- 🇺🇸United States CarlyGerard
I tested the up to date branch on an 11.1.x install, this works as expected for me--the aria-describedby value matches the ID of the description text.
- 🇫🇷France dydave
Yes indeed Charles (@charles belov):
Both patches should not be applied at the same time, see my comment at:
#3286466-64: Tabbing order does not satisfy 508 accessibility requirements →Otherwise, your feedback on this particular patch is very helpful and constructive as usual, thanks a lot!
No problem: We could consider changing the font color to a darker #000000.
[added] Actually, thinking about it, that's one more reason to use the browser's default focus indicator rather than coming up with your own.
So essentially, we would be reverting or resetting all styles for the focus of the menu items?
Do you think that could be a better option overall?Once again, all your suggestions and advice are more than appreciated! 🙏
Thanks in advance! - 🇫🇷France dydave
Thanks so much Charles (@charles belov) for the feedback once again!
Please note my comment on Focus indication clashes with active tab indication. There is code in patch 138 which conflicts with the patch 149 for that issue.
Yes indeed: these are different issues currently under development, so they are very likely to have conflicts and there is not much we could do to fix that.
Ideally, we would like to consider these issues separately, or potentially group one in the other...But you shouldn't be applying both patches at the same time to test both issues, there are great chances it won't work.
If we take out the problem with the styles of the focus:
What would be your general feedback on this patch: In particular, all the work that was put into changing the tabbed navigation and the display, etc... ?
There are a lot of things that could be tested in this issue, putting aside any other known issues that could be addressed separately.
Could we perhaps try focusing on what we already have in this patch?
I personally think it is a great improvement:
I would personally say the module is much better with this patch than without.Please let us know if you encounter more issues while testing or if you would have any questions or concerns on any aspects of this issue, we would surely be glad to help!
Thanks in advance! 🙂 - First commit to issue fork.
- 🇺🇸United States dcam
umm but then we get duplication, I'm asking for other opinions
It looks like you were right to me. I don't see any duplication. I knew there had to be a way. Thank you for showing me.
I updated the IS and CR again.
- 🇺🇸United States charles belov San Francisco, CA, US
Please note my comment on Focus indication clashes with active tab indication 🐛 Focus indication clashes with active tab indication Active . There is code in patch 138 which conflicts with the patch 149 for that issue.
- 🇺🇸United States charles belov San Francisco, CA, US
Thank you for the clarification. I might have to file an issue against core then.
Anyway, yes, it passes. I must say a 4.5:1 contrast of background to text, while compliant, is a bit hard for me to read. If you could also darken the text on focus to #000000, that might help. That said, it's compliant. Unfortunately, there's no way to lighten the green background without breaking 3:1 contrast with the white.
There does seem to be a conflict with patch 138. For the following steps:
Steps:
1. Go to https://simplytest.me
2. Enter admin_toolbar for the project
3. Select 3.x-dev
4. Open the advanced options
5. Keep Drupal core 11.2-dev as the core version
6. Paste patch https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/149.patch
7. Click add additional patch
8. Paste patch https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/138.patchExpected result: I get both the improvements in tabbing sequence provided by patch 138 and the contrast improvements provided by patch 149
Actual result: There is no evidence that admin toolbar is active. However, if I go to Extend, it shows that the admin toolbar module is in fact active.
And for the following steps:
Steps:
1. Go to https://simplytest.me
2. Enter admin_toolbar for the project
3. Select 3.x-dev
4. Open the advanced options
5. Keep Drupal core 11.2-dev as the core version
6. Paste patch https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/138.patch
7. Click add additional patch
8. Paste patch https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/149.patchExpected result: I get both the improvements in tabbing sequence provided by patch 138 and the contrast improvements provided by patch 149
Actual result: I get the improvements in tabbing sequence provided by patch 138 but not the contrast improvements provided by patch 149
- 🇮🇳India Kanchan Bhogade
Hi
have manually tested MR 598 on Drupal 10.3 version with Gin theme
The MR is applied Successfully...Vertical tab height is fixed for the dark mode.
The border color for light mode also darker.Attaching screenshot for reference
RTBC+1
Keeping in need review for code verification
- @kunalgautam opened merge request.
- 🇮🇳India kkalashnikov Ghaziabad, India
kunalgautam → made their first commit to this issue’s fork.
- Issue created by @the_g_bomb
- 🇫🇷France nod_ Lille
umm but then we get duplication, I think it's fine, details is not really a heavily customized template
- 🇫🇷France nod_ Lille
We should be able to do that from twig only, no php changes needed to
form.inc
- 🇺🇸United States dcam
Although @nod_ said "can we add a wrapping div with only the corresponding id for aria? we already have that info in the template..." I couldn't figure out how to get individual attributes out of an existing Attribute object, at least not cleanly. Given that, I figured that this was probably best done in the preprocess function and assigned to a new variable. If I've missed something, then let me know.
I updated the IS and CR with information about this change.
- 🇩🇪Germany rkoller Nürnberg, Germany
Thank you @dev.drupal.ln & @batigolix for picking up and working on the issue, and apologies for the late reply, i am quite behind on going through all the a11y issues across issue queues that received attention recently. :( Todays a11y meeting was a timely and necessary reminder. So I went ahead and have taken a look at MR93 - a few observations:
1) The styling of the filter field needs some work - in general for Claro the design should orient to the Drupal Design System, for Gin I am not aware of any Figma file. Problem with the current state of the MR, the border has a color contrast of 1.2:1 rgb(235, 235, 235)/rgb(255, 255, 255) in Claro and Gin which is not in line with WCAG 2.2 SC1.4.11.
claro
gin
For Claro the border color should be
var(--input-border-color)
and on hovervar(--input--hover-border-color)
, the border should be solid and the sizevar(--input-border-size)
. The filter field in Claro has a height of 48px (the min-height property for that iscalc(((var(--input-padding-vertical) + var(--input-border-size)) * 2) + var(--input-line-height))
).
For Gin the border color should bevar(--gin-border-color-form-element)
and on hovervar(--gin-color-text)
(checked the filter field onadmin/content
for the variable on hover), the border should be solid, the size is 1px, and the corners should be rounded.
The filter field in Gin has a height of 40px (the min-height property hascalc(var(--input-padding-vertical) * 2 + var(--input-line-height))
. There might be more styling details for Claro and Gin to mind, the variables provided for Gin are only for the light mode, they might vary for the dark mode.2) The input field is missing a label (WCAG 2.2 SC 3.3.2)
3) The filter field is missing an outline when being in focus (WCAG 2.2. SC 2.4.7 & SC 1.4.11)
4) If you are entering a term to filter for into the filter field the list underneath is immediately updated accordingly. This would happen to non-sighted users unexpectedly, which is not in line with WCAG 2.2. 3.2.2. In project browser ( 🐛 Improve the accessibility of the search field Active ) we’ve applied the following approach, we’ve added a search button (for the token module the button could be labeled Filter analogous), and the user can either press enter within the filter field or tab to the filter button and then filter. That way the user is in control.
5) If a user wants to clear the input of the filter field they have to use the back space button. If someone assumes pressing the ESC key would clear the field that closes the token dialog instead. It might be worth to consider to again apply the same approach that is used for project browser and currently proposed for the field ui ✨ Add a clear button to the fields ui Active , that way the behavior would become consistent across Core.
6)
If you click on a token in the list, it will populate the last field that had focus. With this change, the last field will become the new token filter field.
this behavior is problematic. That way a keyboard user has no way to accomplish their key task, inserting a token into a field in the background of the token dialog.
7) the hierarchical list becomes hard to read after you have filtered for a string. the treetable is already hard to comprehend in the unfiltered form, but if you enter something into the filter field (for example user), you dont have a top level element as the first element but one that is from the second or third level (the exact level is impossible to assess)
8) Looking at the treeTable problem, that reminds me to the more fundamental question, about how to proceed - there is 📌 Token UI 2.0 Active about Token UI 2.0. It is the question what would be the best approach for the Token UI going forward.
- Should there be a fundamentally different UI and all other issues postponed until a consensus is reached on Token UI 2.0?
- Should there be a fundamentally different UI but until a consensus is reached there should be smaller issues like this one improving the status quo.
- Or should we proceed in the current direction and iterate in small steps with issues like this?
Uncertain what the best approach might be. In regard of the ideation for Token UI 2.0, I’ve suggested in today’s a11y meeting to raise the topic in one of the future weekly UX meeting on friday.
- 🇫🇷France dydave
@charles belov: Was your comment at #9 meant for this issue or rather ✨ Tabbing order does not satisfy 508 accessibility requirements Active ?
It's a bit confusing...
- 🇫🇷France dydave
Thank you so much Charles (@Charles Belov) for taking the time to test again this issue and for all the advice and suggested changes.
Thanks a lot for translating this issue into actionable technical changes.
Based on your suggestions above at #7, I created the initial merge request MR !149 above at #8 with the following changes:
- Focus background color: #33a39d
- Focus and non-focus text color: #2d2d2d
Could you please try testing these changes and let us know if we missed or misunderstood anything in your last reply?
- Active is bold and underlined. I do find it odd that it's shown as a link (underlined), since it's active and I can't tab to it. That might be a separate issue.
OK, no problem 🙂
Let's try to maybe focus on this color contrast change first, then we could always take a closer look at the active menu items styles.Moving issue to Needs review for now, as an attempt to collect more comments, reviews and feedback on MR !149.
Feel free to let us know if you encounter any issues while testing the patch or have more suggested changes, we would surely be glad to help.
Thanks in advance! - 🇺🇸United States charles belov San Francisco, CA, US
Actually, I'm seeing inconsistent behavior as I navigate through the admin toolbar menu.
Focus on specific menu and submenu items are indicated with the green highlight.
Focus on Manage, Shortcuts, the user name, and Announcements are indicated by a thin underline.
Focus on Edit appears to use the browser default.
It would be good to unify the focus indicator so users don't have to hunt it out and perceive changes to it as they navigate through the menus. I would expect the focus indicator to be consistent through the entire menu navigation.
It took me a while to figure out the following: Light gray in a menu item indicates that there a menu subordinate to it. That is, Content, Structure, Configuration, and Reports are gray while Appearance, Extend, People, and Help are not. Additionally, in the Content and other such menus, if an item in that menu has a submenu, there is a chevron indicating the presence of more items, so the light gray there is cosmetic and does not require contrast remediation.
However, the menus for Content, Structure, Configuration, and Reports do not have a chevron, so the light gray currently cannot be considered cosmetic. Either the contrast between gray and white needs to be improved to 3:1 (while retaining a 4.5:1 contrast between text and background) or a down chevron needs to be added to Content, Structure, Configuration, and Reports to indicate the presence of an additional menu.
- @dydave opened merge request.
- 🇺🇸United States dcam
Per @nod_ in Slack:
can we add a wrapping div with only the corresponding id for aria? we already have that info in the template that would avoid changing the structure of description... I would much rather do that. We already have an extra div for errors so that wouldn't be strange to have one for description
I'm working on it.
- 🇫🇷France dydave
Thanks a lot Charles (@charles belov) for taking the time to look at this issue, it's greatly appreciated! 🙏
Indeed: the patch in the MR was outdated and had conflicts with the latest DEV changes.
I have just rebased the merge request and it should apply fine now to the 3.x version of the module.
Could you please try testing the patch again?
I've just given it another round of tests and it really looks great 👍
Please, let us know if you encounter more issues trying to test the patch or with the module in general, we would surely be glad to help.
Thanks in advance for your feedback! - 🇺🇸United States charles belov San Francisco, CA, US
Testing only admin_toolbar 3.x-dev version and Drupal 11.2 (without the other patch, which is a separate issue):
- Focus is shown differently from Active.
- Active is bold and underlined. I do find it odd that it's shown as a link (underlined), since it's active and I can't tab to it. That might be a separate issue.
- Focus is shown as a light green background. Styled as #abe4e0, it has only a 1.4:1 contrast with the white background, so is insufficient to use as an indicator to distinguish from unfocused. Using #33a39d for the focus background would provide 3.05:1 contrast with the unfocused background, which slightly exceeds the 3:1 contrast requirement for non-text contrast.
- But if we change the focus background to #33a39d, then the text color of #3d3d3d fails with text contrast of 3.55:1. We need to darken the text to #2d2d2d to get a contrast ratio of the required 4.5:1.tl;dr: Use the following styling:
Focus background color: #33a39d
Focus text color: #2d2d2d
Non-focus background color: #ffffff
Non-focus text color: Can remain at #3d3d3d or be changed to #2d2d2d to be consistent with the text color for focus. - 🇺🇸United States charles belov San Francisco, CA, US
After attempting on simplytest.me to apply patch https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/138.patch to admin_toolbar 3.x-dev version and Drupal 11.2, I'm getting an error:
This may be the error:
Fatal error: Cannot redeclare function admin_toolbar_update_8005() (previously declared in /var/lib/tugboat/stm/web/modules/contrib/admin_toolbar/admin_toolbar.install:84) in /var/lib/tugboat/stm/web/modules/contrib/admin_toolbar/admin_toolbar.install on line 98
[warning] Drush command terminated abnormally. - 🇫🇷France mably
@funstenolf looks like we have a small problem with our current implementation: if the title is empty we shouldn't generate a carousel_title_id if not provided externally.
Will try to fix that.
-
mably →
committed c74b3c53 on 1.0.x authored by
funstenolf →
Issue #3524173 by funstenolf, mably: Missing aria-describedby to improve...
-
mably →
committed c74b3c53 on 1.0.x authored by
funstenolf →
- @funstenolf opened merge request.
- 🇮🇳India sandip
I have made the suggested changes and moving this issue to RTBC.
- 🇺🇸United States kentr Durango, CO
RE #69
Looking into Automatic Dark Mode further, my take is that the feature is just buggy. AFAICT, these are issues for it. It also appears to still be in a dev stage.
The only small nitpick i might have and i just noticed last night, in forced color mode the unchecked star border is yellow and the checked star is filled yellow. while in none forced color mode the checked star is blue which is in line with the forced color mode, since the color blue stands for links. but the unchecked star border is black / gray.
...
so i wonder shouldnt the star in the unchecked state also have a blue border for consistency and semantics?This is a good point. I was trying to follow @ckrina's colors in #16. Does this change need designer approval?
- 🇳🇿New Zealand ericgsmith
Is this a duplicate of 🐛 Links widget uses checkbox widget library Active - that issue seems to go further than just reverting the class change.
- 🇫🇷France mably
Not sure we want to hard code the
aria-describedby
to the view's title, but let's see what we can do. - Issue created by @funstenolf
- 🇩🇪Germany marcus_johansson
We redid it as part of 🐛 AI CKEditor preview icon looks funny Active that has been merged into 1.1.x-dev release and soon 1.1.x-beta2. I think if you agree, we could close this issue?
Hello @snehal-chibde,
Thank you for review. I’ve updated the MR. I’ve fixed the height of the content to match the height of all vertical tabs in dark mode for better consistency.
As mentioned in first bullet point in "Proposed resolution" I increased the contrast of --gin-border-color. However, I couldn’t directly adjust the contrast of --vertical-tabs-border-color as this variable is not defined in the Gin theme for light mode. I am adding the screenshots for both light and dark mode.Please have a look and let me know your thoughts.
- 🇮🇳India snehal-chibde
hello @utkarsh_kumar_singh, even i have tried this locally, I do not see any changes.
Added screenshots for reference. - 🇩🇪Germany rkoller Nürnberg, Germany
Followup for #68 🐛 Claro Shortcuts star fails WCAG Use of Color and Non-text contrast Needs work . The reason why the two star states were barely visible was my usage of the "enable automatic dark mode" setting in the edge rendering tab. Even though it uses also
prefers-color-scheme: dark
it is still behaving off. @kentr and I had a discussion and some further digging over in: https://drupal.slack.com/archives/C2ANFUGGG/p1746978093222529We still dont know the exact reason why the "enable automatic dark mode" setting is behaving differently output wise, but the following pen @kentr created illustrates the behaviro pretty well between the two approaches: https://codepen.io/kentr/full/XJJyerz (...and narrows down the cause to the
color-scheme
property as the key factor) .enable automatic dark mode
on the left andprefers-color-scheme: dark
on the right.the
color-scheme
property is not used in core so far (only gin uses it in a few places). therefore the suggestion for the way forward would be two fold, to open up a followup issue adding something like:root { color-scheme: light dark; }
to core in general as suggested in the almanac on css tricks ( https://css-tricks.com/almanac/properties/c/color-scheme/). that way all elements of the page/site get opted into both color schemes making those elements theme-able in the forced color mode. and for testing avoid "enable automatic dark mode" nevertheless, instead use the following two settings (here in microsoft edge)
And the star looks good with
prefers-color-scheme: dark
as well as all the available page color themes available. The only small nitpick i might have and i just noticed last night, in forced color mode the unchecked star border is yellow and the checked star is filled yellow. while in none forced color mode the checked star is blue which is in line with the forced color mode, since the color blue stands for links. but the unchecked star border is black / gray.so i wonder shouldnt the star in the unchecked state also have a blue border for consistency and semantics?
- 🇮🇳India abhishek@kumar
ARIA Implementation:
// Example of potential ARIA announcement approach const announce = (message) => { const polite = document.getElementById('tabledrag-aria-live'); polite.textContent = message; }; announce(Drupal.t('Row moved to new position'));
Weight Input Replacement:
<!-- Current --> <select class="tabledrag-weight"> <option value="-15">-15</option> <!-- ... --> <option value="15">15</option> </select> <!-- Proposed --> <input type="number" class="tabledrag-weight" min="-50" max="50" step="1">
- 🇩🇰Denmark ressa Copenhagen
Would an option to disable the "Show row weights" / "Hide row weights" toggle button, either completely or on a role-basis via Permissions fit in this issue as well, or maybe it is already handled in another issue, or need a new issue?
- 🇫🇷France nod_ Lille
small feedback, unnecessary nesting. You can RTBC once it's fixed
- First commit to issue fork.
- 🇫🇷France nod_ Lille
Committed and pushed 2f17d640524 to 11.x and dd94411ba01 to 11.1.x and 21bc1da3e70 to 10.5.x. Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States smustgrave
Believe all feedback has been resolved on this one. Test coverage is failing in 2 spots https://git.drupalcode.org/issue/drupal-2847425/-/jobs/4682940
IS seems to line up to me.
LGTM
- 🇳🇿New Zealand quietone
There hasn't been a response to the query in the last comment. Since there is no clear indication that this is needed, I am closing this issue.
If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.
- 🇩🇪Germany rkoller Nürnberg, Germany
hm that is odd, i've tested MR11498 in Edge with high contrast mode on macos sequoia, the star in the screenshot in #58 🐛 Claro Shortcuts star fails WCAG Use of Color and Non-text contrast Needs work looks okay to me, but in my own testing the star is barely visible in each of the two states.
unchecked:
checked:
- 🇺🇸United States kentr Durango, CO
@smustgrave I think the version here is different than what @ckrina was thumbing up in #6.
There were several iterations. The latest before I worked on it had the "concentric" stars. I didn't use that version because of @ckrina's comment in #16 🐛 Claro Shortcuts star fails WCAG Use of Color and Non-text contrast Needs work :
Once zoomed, I do like the star inside the star emulating the radio styles, but at a smaller size (like the one we're using) it can look blurry or too "crowded". It would also introduce changes on the SVG depending on the size because a 2px spacing might be too much in smaller icons vs too small on the bigger ones, and I'd try to avoid that if possible. I think the contrast with the blue filled vs non-filled grey when it's not saved should be enough:
My version is closest to @ckrina's screenshots in #16. I added the little "+" and "x" in the corners of the hover versions.
Changed back to NR just in case.
- 🇳🇱Netherlands batigolix Utrecht
I made the necessary CSS changes so that the same solution as Olivero is being used
- @batigolix opened merge request.
- 🇳🇱Netherlands batigolix Utrecht
Claro is facing the same problem: 🐛 Breadcrumb separators are focusable with the voiceover cursor and get also announced Active
The solution in that ticket is to follow what Olivero does.