and your proposed solution on the MR you've created with #222330 for the navigation block title color would have a color contrast of 1.1:1 against the background (rgb(42,42,45)) of the navigation sidebar in gin.
for core and the navigation component there is already 🐛 Navigation block titles have a too low color contrast Active so 🐛 Increase the color contrast of the navigation block title Active is a duplicate of that. but i've opened the issue in here because you have a dark mode for gin that does not exist for core. and the shade of grey that is used for the navigation block title in gin is the same in light and dark mode. it is right to fix the color contrast for the navigation block title in core as well. but still the darker grey that will be picked as the solution for 🐛 Navigation block titles have a too low color contrast Active might then not work with the dark background in gin (and being mindful about gin would be out of scope for 🐛 Navigation block titles have a too low color contrast Active ). it might work but still out of scope and why i think it is reasonable having a dedicated issue.
benjifisher → credited rkoller → .
i am taking a look now, was swamped the last few weeks with finishing the draft for gin.
took an initial stab trying to improve the issue summary making it more actionable and instructional.
the_g_bomb → credited rkoller → .
i agree probably it might be not only about the color contrast but also about the font size, a detail we've touched before in yesterdays discussion. and i've updated the issue summary and added a pointer what the navigation block title is in the screenshot.
just as an fyi, the images are not focusable, only the three elements on the image, the checkbox, the edit button and the remove button.
@rahuly072 your theme settings are ok, but you also have to active the forced colors mode. your screenshots show that you are just in dark mode. you have to activate forced colors mode either in your operating system in case you are on windows or in chromium based browsers https://devtoolstips.org/tips/en/emulate-forced-colors/
rkoller → created an issue. See original summary → .
rkoller → created an issue.
rkoller → created an issue.
rkoller → created an issue.
rkoller → created an issue. See original summary → .
rkoller → created an issue.
rkoller → created an issue.
rkoller → created an issue.
removed 🐛 Invalid ARIA attributes Active from the table in the issue summary, added it as a child of the newly created 🌱 [Meta] Improve the accessibility of the Gin theme Active
rkoller → created an issue. See original summary → .
jurgenhaas → credited rkoller → .
The draft of the MR is a work in progress, reflecting proposed changes we've discussed over the course of several ux meetings. The MR is not complete and ready for review yet - we plan to finalize the MR in the coming weeks. We've considered it helpful at this point being able to see all the changes in context. A few remarks:
- So far i've only added the discussed changes for the available field group options, but all those changes also rely on changes on the h1. At the moment the first two steps are just called
Add fields
across all field types and field groups. ✨ Use modals in field creation and field edit flow Needs work has to get in first for that. After that the proposed changes for the h1 on second step could be added as well. - While making the changes for the MR I've noticed that the changes on the field type/field group labels also entail necessary changes in help texts and tests. But those changes should happen after a final consensus was reached about the micro copy. Leave it here as a reminder.
- It has to be noted that at this point the description for every option available for the reference field group has an identical description defined in
EntityReferenceItem.php
. That way it is impossible to provide some sort of guidance to the user what the actual difference between the different reference options actually is and which option to pick in which scenario. So we probably need a separate issue to add the ability to provide individual descriptions for the different reference options. - The
file upload
field group currently has no description yet and is still using the pattern starting with "field to..." we've removed on the description for every other field type and field group. But the entire question how to deal with the media field type is out of the scope for this issue and complicates everything. @benjifisher already opened 📌 Add media reference to the same field type group as file and image Active a few weeks back - the general discussion and work should happen there.
benjifisher → credited rkoller → .
added 🐛 Navigation block titles have a too low color contrast Active
Updated the title and issue summary and added screenshots
the_g_bomb → credited rkoller → .
I've simply pushed another commit now reverting the changes of the commit on the MR from the 27th of january invalidating the changes in the pipeline.yml (4th february) that got in with the rebase... tests are green now and the test only fails properly now as well: https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4243209 ... so the only thing left to do is still another pair of eyes reviewing the MR.
@oily since you are kindly following along. i would have one question. if you see the comments here in this thread, between #44 and #45 i've committed and tested the changes that got into
📌
Change the pipline service from with-chrome to with-selenium-chrome
Active
and then committed and went back to the previous state with *with-chrome
... now
📌
Change the pipline service from with-chrome to with-selenium-chrome
Active
got fixed, so i went ahead and rebased this MR to get the changes in. problem is. if you take a look at https://git.drupalcode.org/project/drupal/-/merge_requests/10631/diffs#n... , there are changes for the pipeline.yml. it somehow the changes introduced in the other issue and that got added by rebase are getting invalidated and the service changed to *with-chrome
. looking at https://git.drupalcode.org/project/drupal/-/merge_requests/10631/commits... is it possible that on a rebase all the commits on a MR are committed again after the rebased core? and that is the reason the change gets reverted again back to *with-chrome
with ba444f84??? and my question is how to fix that?
i've retested with the latest changes, and the problem in #97 ✨ Use modals in field creation and field edit flow Needs work with the focus not moving back to the component that revealed the dialog in safari still persists.
there is another potential rule i just ran into today, but i am not sure if the alt_text_validation module would be able to validate against it, editoria11y might be able to. but the thing i ran it was that the site used the same string the title / header of a card component is using for the alt text. in that particular case that header wasnt even marked up with an h-tag but with a p-tag instead, which might make the validation even trickier. here is an example of the html source. the title/header of the card:
<p class="h1 featured-card__title">Books - Fanzines - Magazines</p>
the image on the card:
<img class="featured-card__image lazyloaded" src="//bisaufsmesser.com/cdn/shop/collections/books_420x.jpg?v=1612535878" alt="Books - Fanzines - Magazines" srcset="" sizes="(min-width: 960px) 450px, (min-width: 720px) 50vw, 100vw" style="max-height: 247.5px;">
if you want to take a look at the different card examples in context, the img src contains the link to the domain.
another rule could be that alt texts should not contain copy right information. i saw it repeatedly that editors used the alt text simply for the copy right information crediting who took the photo not what the photo was actually about.
benjifisher → credited rkoller → .
One detail to note, i've tested the latest state of the MR and the point i've raised in #20 ✨ Use modals in field creation and field edit flow Needs work still applies. when the dialog modal is closed by pressing esc or the close button the focus isnt returning to the "create a new field" button, but instead to the top of the DOM, so the keyboard user has to navigate back to the previous position one left off. the expected and desired behavior is that the focus moves back to the component which revealed the content/dialog.
Adding 📌 Update to jQuery 1.14.1 and use the newly added option for dialog modal headings Active as related issue. The issue is about changing the element wrapping the dialog modals title from a span to an h1, which is relevant and important for this issue as well.
the_g_bomb → credited rkoller → .
the only pressing problem i see is the color contrast of the checkmark (rgb(85, 86, 91)) in dark mode against the background (rgb(59, 59, 63)) (only quickly used the color picker), the resulting contrast is 1.5:1, but it should have at least 3:1. other points would be styling and aesthetics related and the consistency in between light and dark mode and so on. but that could be moved to follow up issues.
jurgenhaas → credited rkoller → .
jurgenhaas → credited rkoller → .
isn't line 89 defining the with-selenium-chrome test? but what the problem was was that test-only changes used with-chrome instead. and this MR changes using with-selenium-chrome for the test-only tests instead of with-chrome.
@oily the fail was a false fail. after some more investigation and some advice from @catch it turns out the tests-only pipeline service definition has to be changed from with-chrome to with-selenium-chrome... the last two commits tested that on this issue. made the change on this MR, the test-only test then correctly failed with
There was 1 failure:
1) Drupal\FunctionalJavascriptTests\Ajax\DialogTest::testDialog
Failed asserting that null is not null.
/builds/issue/drupal-3485202/core/tests/Drupal/FunctionalJavascriptTests/Ajax/DialogTest.php:64
FAILURES!
Tests: 2, Assertions: 14, Failures: 1.
(see https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4155477) . but per the recommendation of @catch i've created a new issue that makes that one line change. 📌 Change the pipline service from with-chrome to with-selenium-chrome Active . when this issue is in then we can rebase this issue and can come to a proper finish.
i've tested in the latest state of 2.0.x and the problem illustrated in the issue summary still applies.
The test-only test is failing as it should.
yeah but if you take a look at the output as noted in #41 you'll notice the two failures were:
Caused by
WebDriver\Exception\CurlExec: Could not resolve host: selenium
that is an infrastructure specific error not an error based on what is actually tested against with this MR?
@oily since you were following along 🐛 Functional Javascript test false postive and missing browser output Active as well. i ran the test-only test now after the rebase and the job failed: https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4151589 . but to me it looks like the testing "infrastructure" aka selenium failed but not the actual test?! do i read that correctly? i "thought" the changes from 🐛 Functional Javascript test false postive and missing browser output Active were contained in that rebase?
I've removed the fail-on-skip as well as the debug flag from test-only.sh and also ran a rebase after 🐛 Functional Javascript test false postive and missing browser output Active went in. fingers crossed that the test-only tests aren't skipped anymore and properly fail now.
jurgenhaas → credited rkoller → .
benjifisher → credited rkoller → .
we can also solve those problems within the scope of the bulk action ui issue and close this issue. would also be fine with me. but just thought scope should be kept tight and this issue was an followup to ✨ Communicated the status/state of a module in one of the upper corners of a module card Needs work
pameeela → credited rkoller → .