Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

At this time we will need a 10.1 patch also for the work mentioned in #5

Please do not just reroll

Also this will need accessibility review once a patch is ready.

Think this falls under WCAG SC 4.1.2

Made MR based on the following feedback from @maxstarkenburg in #accessibility Slack:

1. Remove the ["Expand/Collapse" text] because it's redundant with information a screen reader would already announce via aria-expanded.
2. Depending on who you ask, the button's accessible name (potentially via aria-labelledby) ought to be "[sibling link text]" or "[sibling link text] sub-menu" or "More [sibling link text]".
3. Also, personally, I would recommend not letting the effort to add aria-controls block this issue (perhaps could be made into a separate issue?), since adding aria-expanded and removing the incorrect state would both be easier wins, adding value sooner. aria-controls seems to have limited AT support and/or be semi-"controversial" in its usefulness, see e.g. https://heydonworks.com/article/aria-controls-is-poop/ and https://github.com/w3c/aria/issues/995

Know it's a task but possible to add a simple assertion that checks the aria value. So we can ensure we don't break this.

I re-rolled the patch from #4 to work with Drupal 10.4.x

Adding Needs Tests based on #19, and related issue.

I know nothing about ARIA recommendations, but on #3286466-50: Tabbing order does not satisfy 508 accessibility requirements, @rkoller suggested using aria-pressed instead of aria-expanded:

... But i wonder if aria-expanded is the right choice for a toggle button. I always have problem as a sighted user understanding the current state for toggle buttons that change their label inbetween states. A point that Leonie Watson also illustrated in their talk for smashing magazine: https://youtu.be/OUDV1gqs9GA?t=3222 . She advocated to use aria-pressed instead. That way the button state is either pressed/selected or not and the button label remains the same between states. That way things are completely clear and unambiguous for screen reader users?

Does that apply here? Should we consider it?

@andrewmacpherson: It has been more than 5 years since you self-assigned this issue and last commented. I am un-assigning it now.

@benjfisher

...@rkoller suggested using aria-pressed instead of aria-expanded...
Does that apply here? Should we consider it?

The place in the video that @rkoller linked is regarding a button that toggles light / dark mode. aria-expanded wouldn't apply to that, I believe, because in that use-case there's nothing to expand or show.

Later in the video, there's an example of a show / hide toggle button with aria-expanded. The video creator appears to approve of that use.

There's a separate question of whether to change the "label" (visual display) when the toggle is activated. I can't speak to that. I find changing icons for show / hide widgets (like the orientation of a caret) to be helpful.

@kentr:

Thanks for the reply (here and on the related issue).

I will try to find time to add some tests coverage for this issue. From the comments, that is the only thing holding it back. (The Remaining tasks list "Update JS + templates". I think that is already done, although the current MR changes only one JS file, no templates. If I have that right, can someone strike out that item?)

If it's in scope, I think this related test is passing falsely and needs fixing.

I've observed that the condition that appears to symbolize the appearance of the submenu after clicking will be true even when there's no click.

I checked this in the browser when the submenu is collapsed, and in the test by commenting out the click on the submenu.

I believe it needs a check before the click to confirm that the item is not visible, and then a check after the click to confirm that it has become visible. It looks like the checkVisibility() function might work for this.

It might also be checking the wrong submenu. When testing in the browser with that test code and the nightwatch_testing installation profile, I observe that the click is on the Configuration menu rather than the Reports menu. #toolbar-link-user-admin_index is one of the items that become visible.

@kentr:

I confirmed what you said about toolbarApiTest.js, and I agree it looks wrong. But I do think it is out of scope for this issue. Also, I am not sure what the difference is between that test and toolbarTest.js: what does "Tests of the existing Toolbar JS Api." mean? So let’s leave that for a separate issue.

I added a few lines to toolbarTest.js, checking that the aria-expanded attribute changes as expected. I did not check the aria-labelledby attribute, nor that the inner HTML of the <button> element is empty. I can test either or both of those if anyone thinks it is needed.

I also updated the "User interface changes" section of the issue summary.

I am leaving the status at NW for two reasons:

1. I think the "Proposed resolution" needs to be updated. The current version mentions "Use a constant name for the button:"parent-link-title sub-menu"." That is not what the current MR does. I also added the issue tag for that.
2. There is a CSS rule in toolbar.icons.theme.css (in the toolbar module and in two of the themes) that sets text-indent to a large, negative number for the body of the <button> element. Now that the body is empty, I think we can remove that rule.

I updated the proposed resolution and added a todo item for the CSS. I believe this addresses the issue summary updates requested in #28.

I don't have time to do a full review, so I'm not striking out any of the todo items.

Adding Needs change record because the policy requires it for changes that affect UI / UX.

The MR pipeline failed. It looks like it's from the JS size change and the value for ScriptBytes in AssetAggregationAcrossPagesTest.php needs updating. Removing the CSS may also require a change there.

At my day job, we use the admin_toolbar module, which overrides how the menu links are rendered. The result is that they do not have id attributes, which makes the changes from this issue ineffective.

I added #3538171: Menu links should have id attribute for the admin_toolbar module, but I am not sure it will be accepted. There may be a reason (performance?) for not including id attributes in the admin_toolbar module. In that case, is there any other value we could use here for the aria-labelledby attribute?

RE #30, I suggest:

1. Don't use aria-labelledby.
2. Revert to having the label component in the button text.
3. Keep the action component out of the button text to resolve the problem described in the IS.
4. Add an assertion in the test to confirm that the button accessible name equals the text of the corresponding link (the link that was used for aria-labelledby).

I think that will also resolve @nod_'s comment in the MR.

I've made some suggestions on the MR for these.

Rationale:

I'm guessing that aria-labelledby was a result of @Max Starkenburg's comment in Slack:

After further reflection (and defaulting to trusting Andrew's judgement on something like this), I'm realizing that it probably indeed made sense to try to remove a description of the state from the button, since I believe most screen readers will already announce the change in the aria-expanded value anyway (e.g. VoiceOver seems to announce "[accessible name], collapsed, button" when tabbing to such a button).

That led me to wonder what the accessible name should be. I sought out "what would Adrian Roselli do for such disclosure widgets?" leading to https://adrianroselli.com/2019/06/link-disclosure-widget-navigation.html#Name, in short, to use the name of the sibling link (via aria-labelledby). I think Andrew's patch was to use "[sibling link text] sub-menu", and looks like WAI's example uses "more [sibling link text]". I think neither of those quite contribute to the same type of verbosity problem Adrian reneged on (which seems to have been to toggle "show [sibling link text]" and "hide [sibling link text]").

Duplicating the link's text in the button should accomplish the same goal of "use the name of the sibling link", but without relying on aria-labelledby.

Sorry, disregard the image attachment in my last comment. It was an accidental holdover from an earlier version of the comment.

I updated the MR, removing the CSS rule that is no longer needed if the button has no text (inner HTML). I remembered (barely) to update the .pcss.css files along with the .css files. I updated toolbar.icons.theme.css in the toolbar module and the claro theme.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css. I added a copy of toolbar.menu.js (before any changes from this issue) to the theme, and updated the libraries-override section in its .info.yml file.

Testing: I used drush generate theme to create a sub-theme of stable9. I enabled that and set it as the admin theme. I checked that the old markup was used on admin pages and the new markup was used on non-admin pages.

When I finished that, I saw that @kentr had made some suggestions on the MR, perhaps in response to @nod_'s comment there. I think it will be easier to discuss those suggestions here instead of on the MR. From the issue summary, the current markup (11.x) for the button (when the submenu is collapsed) is

<button class="toolbar-icon toolbar-handle" style=""><span class="action">Extend</span> <span class="label">Structure</span></button>

Using the current MR, that changes to

<button aria-expanded="false" aria-labelledby="toolbar-link-system-admin_structure" class="toolbar-icon toolbar-handle" style=""></button>

I think @kentr proposes changing that to

<button aria-expanded="false" class="toolbar-icon toolbar-handle" style=""><span class="label">Structure</span></button>

That is, remove the aria-labelledby attribute and restore the text inside the button (inner HTML).

I am not an accessibility expert, so I have no opinion on that suggestion. If we can get consensus on what the markup should be, I am happy to update the MR. (Among other things: if we restore the text, then the CSS rule is no longer redundant.)

P.S. I neglected to reload the page, so I wrote this comment before seeing #31 and #32.

I'm not an accessibility expert either.

I think the underlying problem with the button text is that there's also text indicating what the button will do (the "action" component) and that it is the opposite of the "state" of the sub-menu and what's indicated by the icon. From the IS:

the [accessible] name of the button is the opposite of the current state. There's some unnecessary cognitive effort there, to figure out what the current state is.

When the sub-menu is collapsed, the icon shows that, but the text in the button says "Extend [sub-menu]".

Using aria-labelledby to avoid repeating "[sub-menu]" in the button text would be convenient if it worked, but maybe it's not feasible.

Using the aria-expanded state instead of words like Extend/Collapse in the button name is definitely the right thing to do.

Regarding what the button names should be, we've found in user testing of a main navigation with a similar design that there are some assistive technologies (e.g. ZoomText) that will convey the name but not the role so both the link and button would simply be called "Structure." Therefore, I think including "Menu" at the end of the button names would help differentiate the links and buttons and better convey the button's purpose. Since aria-labelledby can reference multiple ids, " Menu" can be added after the text taken from the link.

However, a reason to use visually hidden text within the buttons to name them is client-side language translation is more likely to be accurate. If the button for "Site settings" uses aria-labelledby to reference the link's text and another element containing just the word " Menu," the name and what a screen reader will say is "Site settings menu." However, if the page is client-side translated to French, the two strings are translated separately so it would say "paramètres du site menu."

If the button instead had the visually hidden text "Site settings menu," the translation would be "menu des paramètres du site" which would be more expected and more easily understood. I think the aria-labelledby version would still be understood, I don't know if there are languages for which the meaning would be lost if the words would not translated all as one string.

I'd add that a reason to not name the buttons using an attribute string like aria-label="Site settings menu" is translation software tends to skip aria-label entirely but does translate both visually hidden text and text hidden from everyone (with the hidden attribute, display: none styles, etc.)

Thanks for the guidance, @cwilcox808. I have updated the MR based on Comment #35. (I hope I got it right.)

I think sentence case (not title case) is the Drupal standard, so I made it "Content menu", "Structure menu", etc, not "Content Menu". Would "submenu" be clearer than "menu", or would that just be an additional, unhelpful syllable?

I am updating the issue description. At the end of the Proposed resolution section, I have this:

Screen reader announcements will be along the lines of "structure menu, expanded, button" and "structure menu, collapsed, button".

Is that right?

The performance test (Drupal\Tests\demo_umami\FunctionalJavascript\AssetAggregationAcrossPagesTest::testFrontAndRecipesPagesEditor()) failed with the message,

Failed asserting that 445441 ( is equal to 445601 or is greater than 445601 ) and ( is equal to 446601 or is less than 446601 ).

I think @kentr already mentioned this on Slack.

This is a case where updating the test is the right thing to do. This MR reduces the size of toolbar.menu.js by 214 bytes. Before that reduction, it was in range. By design, the performance test passes unless the number of bytes changes by more than 500, so I reset it to the current value, 445441.

P.S. It was not in Slack. It was Comment #29 on this issue.

@benjifisher I don't know if "submenu" would be more clear than "menu." Without additional information, I'd stick with the shorter term.

The current text of the Proposed solution looks good.

Instead of using a large negative text-indent to visually hide the text of the buttons, I would use the .visually-hidden utility class (probably on the inner span.label rather than the button itself) or the same properties that class uses. That collection of properties has proven to be more robust, avoiding issues with right-to-left languages, very large viewports or font-sizes, etc..

@cwilcox808:

Thanks for the review!

I did a little research. The text-indent rule was added in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop. Searching that issue, I see one mention of element-invisible, the class that was later renamed to visually-hidden, in Comment #288. But I do not see any argument against using the utility class.

I updated the MR here to use visually-hidden, and I restored the change that removes the obsolete text-indent rule. As I said in #33, the stable9 theme already overrides the CSS file, so I do not have to worry about keeping that theme stable.

... probably on the inner span.label rather than the button itself

We definitely do not want to make the button itself visually-hidden. That would hide the icon.

I am updating the snippets in the issue summary to add the visually-hidden CSS class.

I've checked the latest changes on a test install and they look good to me.

I'd love to have an assertion for the (accessible) name of the button because the choice was very deliberate.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css.

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

Actually two assertions, to show that the name stays constant with the toggling (i.e., to show that we fixed that problem also).

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

That is my understanding.

Any changes to the stable9 theme have the potential to break custom themes based on it, and perhaps cause automated tests to fail. We normally think in terms of the CSS in the theme, but I think it applies to JS, too.

I am removing the tag for an a11y review, thanks to @cwilcox808's comments.

I am setting the status to NW for additional test coverage (Comments #43, #44). I will try to get that done soon, but I have a 1-week vacation coming up. If I do not find time in the next two days, then it will be at least another week before I do it.

It took two tries, but I added the assertions for the button text, before and after toggling.

Unless I totally misunderstand the nth-child selector, my mistake was to look for the second item in the admin menu after installing the Standard profile. That is Structure. I am not sure which profile the Nightwatch tests use, but it seems that the second item is Configuration.

Back to NR.

I think checking the name with .getAccessibleName() instead of checking the button text would be more robust.

I understand the goal to be presenting a usable name for AT. The name is currently in the button text, but it could get overridden in the future by aria-label or aria-labelledby. Checking with .getAccessibleName() should catch that.

Since @benjifisher said he'll be away, I'll make that change and let it go through review.

@kentr:

I am sorry to be quiet for so long. I have been dealing with some serious medical issues. (If you want the details, then I can point you to a Slack thread.)

There are a couple of reasons I am nervous about using .getAccessibleName().

First of all, I am having trouble running the Nightwatch test locally. (See my Comment #46: "It took two tries, ...".) So I cannot tweak and evaluate as I would like.

Second, I searched the codebase and this seems like the first use of that test method. Maybe there is a reason it is not being used? Or maybe it should be used widely, in which case it might be a better idea to have a big issue for updating lots of tests instead of starting small with the test for this issue. In other words, I see a lot of value in keeping a single test self-consistent. Even if .getAccessibleName() is a better way to test, it might not be worth the inconsistency.

Third, I think .getAccessibleName() relies on the translated message. The usual practice in automated tests is not to use translated text unless you are testing translations. But see my first point above: I may be wrong about this.

There is also a meta-reason for not making this change. Until you made that test change, you preserved your role as a reviewer on this issue. I think we can restore that if we revert your commit. Then you have the authority to declare this issue RTBC. If we have both committed code to the MR, then we have to find someone else to review it.

I had an interesting discussion with @larowlan a while ago: https://drupal.slack.com/archives/C223PR743/p1753146459028029. I started by asking for some general advice on Cypress tests for the current project at my day job. Here is a bit of that thread:

A day or two ago, I was looking at some Nightwatch tests in the tooolbar module. They seem to use CSS selectors and nothing else.

-- @benjifisher

and

yeah so do a lot of our JavascriptFunctionalTests in core unfortunately

-- @larowlan

I was referring to the tests for this issue, of course.

Anyway, my point is that there is a lot of room for improvement in our current test suite. Maybe .getAccessibleName() is part of it, maybe not. But this is what I am thinking in my second point, where I suggest making broad changes to the tests instead of an isolated change to the test for this issue.

@benjifisher:

I'm sorry to hear about your health issues.

I reverted the commit. Now the pipeline is failing with what appears to be #3539366: Default DB transaction isolation set to read-committed breaks InstallerIsolationLevelExistingSettingsTest test 😔. I'll update the fork in the Gitlab UI to see if that fixes the pipeline.

I see myself more as a collaborator than a sole reviewer. I apologize if I botched the issue queue workflow or etiquette. I believe that this is a better way to test, though, so I won't mark it as RTBC. Folks with more authority can make the call.

Second, I searched the codebase and this seems like the first use of that test method. Maybe there is a reason it is not being used? Or maybe it should be used widely, in which case it might be a better idea to have a big issue for updating lots of tests instead of starting small with the test for this issue. In other words, I see a lot of value in keeping a single test self-consistent. Even if .getAccessibleName() is a better way to test, it might not be worth the inconsistency.

My thought is that there's already a lot of inconsistency in tests. I don't love that either, but I haven't found detailed coding standards for test implementation, and broad changes are harder to accomplish. I wish there were coding standards on this issue. It's difficult, though, because PHPUnit doesn't have this functionality yet AFAIK.

I see your point about consistency within a test, but I see this as a (slightly) different problem than locating the element because it's an explicit assertion that the accessible name is what we want it to be. If the markup changes to use aria-label or aria-labelledby, then checking the text content could lead to a falsely-passing test:

1. Markup changes to use aria-labelledby that resolves to "overriding, incorrect label", but text content doesn't change.
2. Test passes because it checks the text content.
3. Actual users perceive "overriding, incorrect label" because it overrides the text.

Third, I think .getAccessibleName() relies on the translated message. The usual practice in automated tests is not to use translated text unless you are testing translations. But see my first point above: I may be wrong about this.

I don't understand what you mean regarding the translated value. My understanding is that .getAccessibleName() computes the accessible name based on the page HTML, as the browser would:

Returns the computed WAI-ARIA label of an element.

Anyway, my point is that there is a lot of room for improvement in our current test suite. Maybe .getAccessibleName() is part of it, maybe not. But this is what I am thinking in my second point, where I suggest making broad changes to the tests instead of an isolated change to the test for this issue.

I agree that there is a lot of room for improvement and with @larowlan's comments about using findByRole, findByLabelText, and findByText.

If this is a better testing method, then we'll at least have one test that's better. Maybe it will bring awareness.

As an aside, Playwright agrees with Testing Library and @larowlan:

We recommend prioritizing role locators to locate elements, as it is the closest way to how users and assistive technology perceive the page.

and recommends including the accessible name (rather than the text content) as part of finding by role:

When locating by role, you should usually pass the accessible name as well, so that the locator pinpoints the exact element.

Now the pipeline is failing ...

I will see if I can fix it. Maybe Tuesday.

I hope you will reach out to @larowlan and/or @quietone on Slack and discuss the possibility of large-scale changes to the tests.

Returns the computed WAI-ARIA label of an element.

If we translate the text, either using $this->t() in PHP code or Drupal.t() in JS, won't that be the translated text? Again, I was unable to try this out since I could not get the tests to run locally. I suppose I could push a temporary, do-not-merge commit and let GitLab CI do the test.

Thanks for reverting your commit.

I see myself more as a collaborator than a sole reviewer.

That's a bummer. Then we still need to find someone to review the issue.

Or maybe this is the time for you to take on the Reviewer role, which is very important. I think you are familiar with the code and test changes on this issue. Why not review it?

RTBC means Reviewed and Tested By the Community, not Ready To Be Committed. (It used to stand for that.) You are a member of the community. Your review represents your best effort to improve, then approve the issue. Do your best to ask the questions that a core committer would ask. Your best effort is good enough. If there is part of the MR that you are unsure of, be open. Tell them that you are not sure, and they will focus their attention in the right place. Some of my early reviews admitted that I had not looked closely at the tests, since I did not feel qualified. That's OK. Open Source Software is built on radical honesty and radical modesty.

When the core committer reviews the issue and asks for changes, you will learn something. Your next review will be closer to the goal of asking all the questions the committer would ask, which saves them the trouble of asking those questions.

@benjifisher: First of all, I really do hope that your commenting means that your health issues have resolved, or are resolving.

The pipeline is passing now.

Or maybe this is the time for you to take on the Reviewer role, which is very important. I think you are familiar with the code and test changes on this issue. Why not review it?

RTBC means Reviewed and Tested By the Community, not Ready To Be Committed. (It used to stand for that.) You are a member of the community. Your review represents your best effort to improve, then approve the issue.

I guess I'd say that I have reviewed it and that I've made my best effort to improve it by suggesting changes. I'm not ready to approve it, though, because I feel strongly about using .getAccessibleName(). I would write the test that way and then leave it up to maintainers or a vote in the review process. I can leave a comment on the MR, though.

If we translate the text, either using $this->t() in PHP code or Drupal.t() in JS, won't that be the translated text?

Wouldn't the inner text content that is found and checked by .textEquals() also be affected by this? The Nightwatch page says "Check if an element's inner text equals the expected text.". I don't understand how the text that it finds and checks could be different than what is output from $this->t() or Drupal.t().

Another way to put my argument about .getAccessibleName() is that I see the goal as making sure that the control is perceivable to the user in the way that we specify.

Checking the text content alone doesn't do that reliably because there are multiple methods for naming an element, and the final result is computed by the UA.

To really accomplish the goal, I think we'd need to also check that there's nothing overriding the text content (check for absence of aria-label, aria-labelledby, etc.). It's complex and depends on the role (reference).

.getAccessibleName() should do this for us so that we don't have to reinvent the wheel. It's also in the WebDriver spec, which Nightwatch may use (I couldn't tell from looking at the Nightwatch code).

[edited to fix typo]

My understanding is the point of doing End-to-End tests using tools like Nightwatch is to have tests running in real browsers so that they're closer to the actual experience of users. I would expect such tests to run after Drupal translation methods have completed.

.getAccessibleName() is within that spirit, the user doesn't care how a control comes to have its name, they only care that it has the name they expect. Testing where a control's name comes from is a different kind of test that's also worth doing when there's a good reason to care where the name comes from (and I think there is).

Briefly looking, I don't see anything that exposes where a name comes from. If a <button> is supposed to be named by its content, a test could check for the absence of aria-label or aria-labelledby on the element. A button can also have a <label>, which supersedes its content as its name, but it's rarely used and it's harder to test.

@kentr:

Thanks for asking about my health. At this point, I am working three days a week and getting back to Drupal contribution. In a few months, I should be close to my "old normal".

If you are willing to take on the Reviewer role, then I will defer to you. I reverted your revert. Of course, the core committer will have the last word.

Referring to translated text:

Wouldn't the inner text content that is found and checked by .textEquals() also be affected by this?

Good point.

@cwilcox, thanks for weighing in.

I am adding a little more detail to the Proposed resolution section of the issue summary.

There is a test failure for core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php. It looks unrelated, and the test passes locally, so I am re-running the test.

The pipeline passed.

I mentioned the discussion of .getAccessibleName() in Comments #47, #49 to #54 in the Proposed resolution section.

@kentr: FYI, this attention to the issue summary is one way I try to make it easier for the core committer to do the final review.

I went over the points in the proposed resolution. Everything looks good except #5, "Make sure that these changes do not affect the stable9 theme". It may need some adjustment.

Details below.

1. Use aria-expanded to convey the current open/closed state. [✅ OK - Spot checked.]
2. For the button accessible name that is announced by screen readers: use the visible text link whose sub-menu is toggled by the button (aka, the button's "sibling link") followed by "menu": for example, "Content menu" or "Structure menu". [✅ OK - Spot checked.]
3. Use the visually-hidden CSS class instead of a custom text-indent: -9999px rule targeting the button. (Maybe out of scope.) [✅ OK - Here and here. also checked the element in browser inspector.]
4. Add test coverage for the aria-expanded attribute. [✅ OK - When running test-only changes, test failed due to missing aria-expanded.]
5. Make sure that these changes do not affect the stable9 theme. [⚠️ UNCERTAIN - See below]
6. Update the claro theme to match (removing the text-indent rule). [✅ OK - Here and here. also checked the element in browser inspector.]
7. Update a performance test, since the size of one JS file is a little smaller. [✅ OK - Verified by running test-only changes.]
8. After some discussion (Comments #47, #49 to #54) we agreed to use .getAccessibleName() in the Nightwatch test. [✅ OK - Double-checked by temporarily altering the expected value in the test.]

Regarding #5 (stable9 theme)

I checked manually in stable9 against 11.x@f795f242f12 by un-hiding stable9 and setting it as the default theme.

For me, the text-indent seems to be missing on non-admin pages, causing the button text to be visible. It should be hidden, right?

Here are a couple of screenshots with 11.x on the left and the MR on the right.

Steps to reproduce with the MR

1. Do a standard installation.
2. In stable9.info.yml, comment out hidden: true.
3. Log in as admin.
4. Set the default theme to Stable 9.
5. Go to a non-admin page like the front page.

I think that rule is coming from core/themes/claro/css/theme/toolbar.icons.theme.css.

I didn't try it, but the MR might need to copy any Claro toolbar CSS into another location.

Note: There was also a console error related to contextual.js, but I saw that also in 11.x (so, it's unrelated to the MR).

@benjifisher

FYI, this attention to the issue summary is one way I try to make it easier for the core committer to do the final review.

I can totally see that.

[edited to add missing "OK" for some items]

@kentr:

I confess: I often have to remind myself that the T in RTBC stands for "tested". Thanks for the detailed test results on this issue.

When I follow your steps to reproduce (STR), I see the same result on admin and non-admin pages: I see the text, and my browser tools do not show any setting for text-indent.

In Step 4, if I also set stable9 as the admin theme, then I get the expected result. I no longer see the text (admin page or not) and my browser tools show text-indent: -9999px on the <button> element. Based on that, I am setting the status back to NR.

It seems you are right, and some of the Claro styles are leaking (or bleeding) into the toolbar. That could be a known bug, like #2987665: Toolbar styling is easily disrupted by theme CSS.. I thought there was an issue like #3511280: Front-end theme styles can bleed into Navigation for the toolbar, not Navigation, but maybe I was wrong. Since it seems like the admin theme is bleeding into the toolbar, this might be a new bug instead of an old one.

@benjifisher:

I might misunderstand what stable9 is used for, but my question is whether there will be a problem (i.e. a BC break) in this scenario:

• Admin theme is Claro.
• Default theme is based on stable9.

Claro appears to completely remove other toolbar CSS from non-admin pages.

The changes might indirectly affect themes based on stable9 because of Claro's theme overrides.

If I comment out the line with unset(), the stable9 stylesheet (stable9/css/toolbar/toolbar.icons.theme.css) reappears in the page source and overrides the Claro stylesheet. The problem then goes away.

I don't know if that causes other problems, though.

I did a little digging. Looks like the Starter Kit theme is based on stable9.

I set the default theme to Starter Kit like I did with stable9 and got this:

If a new theme is created with the starter kit, will that theme have this problem initially?

If #58 & #61 are considered a problem, an easy fix might be to restore the text-indent in the Claro CSS but keep visually-hidden on the element.

Other than being redundant, would restoring text-indent harm anything?

I think you are right: we have to fix this. I am setting the status back to NW.

I do not mind a little redundancy, but my first thought is that visually-hidden should be enough. I will look into why it is not, but it might be a few days before I get to it.

@kentr:

Thanks for pointing me to claro_system_module_invoked_library_info_alter() in Comment #60. That function was added in #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future (odd title). The idea is that Claro (the back-end theme) should be able to style the toolbar even on front-end pages. Personally, I think a better idea would have been to provide a mechanism for the site admin to choose whether the front-end theme or the back-end theme (a.k.a. the default theme or the admin theme) gets to style the toolbar on front-end pages.

I do see the value in making the toolbar consistent on all pages.

Anyway, I just added a few lines to that function so that Claro insists on using toolbar.menu.js from the toolbar module (just as it insists on using various CSS files from Claro). It takes just a few lines, but I could not get it right the first time I tried, about a week ago.

I am adding a few lines to the issue description and setting the status back to NR.

It looks as though git.drupalcode.org is in the middle of a 2-hour maintenance window, so I cannot check the pipeline.

From the manual testing I can confirm the aria-expanded is present and updated when opening/closing. Also tested on various sections, like opening 1 and then another updated all them correctly.

Sucks stable9 has to do it the old way but get it.

Putting in NW for the change record tag.

I added a draft change record.

That is one of the best change records I've read yet! Kudos

I have no feedback.

That is one of the best change records I've read yet!

Thanks. I copied most of it from the issue summary here, changing "after this issue" to "after this change". The bit at the end about overriding the override is something I had to figure out for my day job, so I copied the YAML snippet that I added to our theme's .info.yml file.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Merge conflicts

#3579899: Remove remaining claro.theme functions removed claro.theme. In particular, it replaced claro_system_module_invoked_library_info_alter() in that file with Drupal\toolbar\Hook\ToolbarThemeHooks::library_info_alter(). To resolve the modify/delete conflict, I copied the change for this issue from claro.theme to ToolbarThemeHooks.php.

Testing

See the issue summary (User interface changes section) for the expected "old markup" and "new markup" (a.k.a. before and after). The stable9 theme, and any child themes, should use the old markup. Other themes should use the new markup. But there is a twist: if Claro is the admin theme, then it overrides the toolbar markup for the front-end theme. Therefore, I tested with several combinations of default and admin themes (a.k.a. front-end and back-end themes):

Explicit steps:

1. Install Drupal with the Standard profile.
2. Uninstall navigation and install toolbar.
3. Check the markup on the home page and an admin page.
4. Enable the Default Admin theme and set it as the admin theme.
5. Repeat Step 3.
6. Set hidden: false in stable9.info.yml.
7. Set Stable 9 as the default theme.
8. Repeat Step 3.
9. Set Claro as the admin theme.
10. Repeat Step 3.

Back to RTBC, since this was "just" a merge.

I skimmed the very detailed change record and updated the who this impacts. Also removed the version from headers as this information should be in the version and branch field.