Focus submenu button when clicked #55198
Conversation
SantosGuillamot
added
[Type] Bug
SantosGuillamot
requested review from
luisherranz,
jeryj,
afercia,
joedolson and
alexstine
|
Size Change: +6.71 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
@SantosGuillamot Thanks for the PR.
From what I can tell, this is at least a partial fix. I tested on Windows 10 with NVDA in Google Chrome and Firefox. I tested on Mac Big Sur with Voiceover and Safari. This seems like a solid fix across OS platforms.
I can however not test the click behavior with the mouse, this is only a test done with the keyboard. I would wait for one more approve on this one.
There was a problem hiding this comment.
This is testing well for keyboard interactions. The test steps also work correctly in safari with a mouse click. I noticed one regression from trunk.
- Open the site in Safari.
- Click on the arrow to open the submenu.
- Click on the arrow to close the submenu.
- The submenu stays open, when it should close as it does on Trunk and in Chrome.
|
Thanks a lot for testing this. We are trying to avoid changing the focus programmatically to avoid introducing bugs like this so we are now experimenting with a different fix that uses It seems to work fine, but before committing to it, we want to make sure that that behavior is consistent across all the different Safari versions and devices. We'll test it out with BrowserStack, and we'll let you know as soon as possible. |
|
@SantosGuillamot and I have been testing the Could you please check again? Thank you 🙏 |
|
Flaky tests detected in b92bf04. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6562093203
|
|
I'm not able to test this on Windows because gutenberg.run seems to be failing for this PR. Testing on macOS/Safari both mouse and keyboard interaction are working fine. I tested opening and closing submenus and nested submenus and don't think I missed anything this time 😄 |
There was a problem hiding this comment.
Sorry to keep blocking this :/
The good news: It does work! I can't break it using TT3 theme.
The odd news... the tabindex isn't always consistent in the value in relation to the state. I can get the menu to be open, and not have a tabindex value by:
- Tab to a dropdown chevron
- Press Enter:
tabindex-1 is applied body tag - Press Shift + Tab to go back to the chevron:
tabindexis removed from the body tag
It can also arrive in the "wrong" state with hovering/clicks:
Screen.Recording.2023-10-13.at.8.46.32.AM.mov
It also allows the focus to be placed on the <li> in some instances on Safari and Chrome.
- Tab focus to a link with a dropdown
- Hover the link
- Hover off the link
- Focus ring is wrapped around both the anchor and chevron
document.activeElementreports the<li>
Screen.Recording.2023-10-13.at.8.54.08.AM.mov
Overall, I'm not sure what's going on with it, but it seems inconsistent. I'm really interested in why this fix works. Since I can get the body to not have a tabindex in Safari, I thought the bug of not being able to click the page to close the menu would still be there... But it isn't... This code works to address that.
If we can isolate exactly what is allowing it to work in Safari, then we can use that to provide a fix that will work without potentially introducing odd states like the ones above.
So, sorry for the long text and for keeping this blocked. I'm super curious to find out more!
| @@ -20,6 +20,9 @@ const openMenu = ( store, menuOpenedOn ) => { | |||
| if ( context.core.navigation.type === 'overlay' ) { | |||
| // Add a `has-modal-open` class to the <html> root. | |||
| document.documentElement.classList.add( 'has-modal-open' ); | |||
| } else { | |||
| // Ensure that Safari on iOS/iPadOS trigger the mouseleave events. | |||
| document.body.setAttribute( 'tabindex', '-1' ); | |||
There was a problem hiding this comment.
Do we know why this works? I'm not saying we can't do this, but I'm hesitant to do this since it could have far-reaching effects. I'd love to learn more about it as I didn't know this would trigger mouseleave events for Safari.
There was a problem hiding this comment.
Yes, I don't like this. I would never set this on the <body>.
|
The current implementation is coupled with how the focus is positioned in Chrome, so it's difficult to make it work in Safari because the focus position is totally different. Using The problem is that the ideal solution would be to change the implementation so it doesn't depend on the focus position, but this would require a complete refactoring of the logic, and it might be a bit late in the WP 6.4 release cycle for such a big change. We can keep trying to fix it for WP 6.4 with @jeryj, would there be any chance we could pair program with you to see if we can find the best solution together? |
|
@luisherranz I'll give +1 to just setting focus via the ref. That seemed to work. |
|
Thanks, Alex. I've talked with @jeryj and we'll meet tomorrow to keep iterating this 🙂 |
|
Safari doesn't set focus on buttons, links, or non-text inputs; so |
I played around with this too. I accidentally stumbled upon using both tabindex="-1" in the li element AND using ref.focus() seems to work well. I think it's because the tabindex="-1" n the li prevents Safari from sending focus to the parent navigation wrapper. Combining this with a I added some checks to verify focus on the frontend interactivity tests as well, and added directives to run it in Safari. I spent the last hour debugging why it's flaky though. I believe it's an issue with the test set-up in Safari though. Checking it manually with the same steps consistently works. If we need to remove the Tab keypresses that keep breaking it or write them in a way that tests what we need to while being consistent, I'm fine with that. 🤞🏻 it all works! |
|
Tests pass, but they are flaky, as indicated by #51552. To run the tests locally, you can do: |
|
Thanks a lot for your help and the tests improvements 🙂
I've been testing the latest changes in iPhone and iPad, and unfortunately, it doesn't seem to work there 😞 If I open a submenu, it is not closed by clicking outside. Adding the empty click event listener seems to fix the issue. Going back to Luis' fix everything seems to work fine as well. |
Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change.
Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress.
|
I approved the changes since there wasn't a way for me to dismiss my own review. I think it needs a review from someone else now since I've made code updates. |
|
For reference, this is the implementation that was finally merged:
So this should now be a fully working solution. We still want to investigate an alternative overlay-based solution at some point, though. Thanks to everyone for their help with code and/or reviews 🙂 |
* Focus element manually when open submenu on click * Try using `tabindex="-1"` * Use `tabindex="-1"` also in body when a submenu is opened * Replace tabindex with event listener * Explain the tabindex on <li> * Don't store the element on hover to restore the focus later * Improve explanations * Add tests to cover webkit frontend menu interactions Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari. * Focus clicked button on Safari Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu. * Added the document.addEventListener body click back in Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change. * Remove tab keypresses from webkit menu interaction tests Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress. * Use body click instead for consistency across environments --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
|
I've gone ahead to cherry-pick this change for inclusion in 6.4's RC2: 042dfed |
SiobhyB
removed
the
Backport to WP 6.7 Beta/RC
* Focus submenu button when clicked (#55198) * Focus element manually when open submenu on click * Try using `tabindex="-1"` * Use `tabindex="-1"` also in body when a submenu is opened * Replace tabindex with event listener * Explain the tabindex on <li> * Don't store the element on hover to restore the focus later * Improve explanations * Add tests to cover webkit frontend menu interactions Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari. * Focus clicked button on Safari Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu. * Added the document.addEventListener body click back in Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change. * Remove tab keypresses from webkit menu interaction tests Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress. * Use body click instead for consistency across environments --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Make layout support compatible with enhanced pagination (#55416) * Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name * Update default fullscreen icon for lightbox trigger (#55463) * Make duotone compatible with enhanced pagination (#55415) * Patterns: fix capabilities settings for pattern categories (#55379) Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Revert "Patterns: fix capabilities settings for pattern categories (#55379)" This reverts commit 6f83c92. * Image block: wrap images with hrefs in an A tag (#55470) * This commit sees what happens when we wrap the image element in an A tag in the editor. This is to ensure any inherited styles from the link element, such as border color, apply to the image. * Removing duplicate <a href /> wrapper Adding disabled onClick and aria attribute * Image: Improve focus management in lightbox (#55428) * Improve focus management This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes. * Add delay before focusing when closing lightbox * Put focus back on close button when opening lightbox It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox. * Move focus to the dialog when opening the lightbox. * Fix SVG markup. * Consistent indentation with spaces. * Remove unnecessary tabindex --------- Co-authored-by: Andrea Fercia <a.fercia@gmail.com> * Fix: - Update the title when using enhanced pagination --------- Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com> Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Artemio Morales <artemio.morales@a8c.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
|
I've noticed that |
|
There's a problem with closing the menu when clicking inside the debugger or address bar, stemming from this PR and described in this ticket: https://core.trac.wordpress.org/ticket/60192 |
|
@krokodok, a similar issue was reported recently. I don't think that's a bug; it's how most dialogs/popovers behave. See: #56094 (comment) |
|
Didn't know about that debugging option, good to know! Still I am wondering: Is this standardized best-practive behaviour? Is there a consensus on how such elements should behave? I am finding all different kinds of examples. |
|
From my own personal view I think it is a benefit being able to open a state and then leaving the browser, maybe checking some related stuff on another monitor, and then returning to the browser and finding the exact same state that I left. When the state changes that feels sudden and confusing. As I said, the implementations differ from website to website. Google for example, on their home search page, do it like it is done in WordPress right now, regarding the user profile menu. |
What?
This pull request adds some JavaScript logic to ensure focus the submenu button when it is clicked. It solves this issue: #54991
It also adds a fix for closing the submenu overlay on a click outside the submenu on iOS.
Why?
It seems that Safari doesn't place the focus on the button element but on a parent
div, which causes some issues with the directives assuming the focus is on the button. We can see the difference between Chrome and Safari:Chrome
Comparing.trunk.fix_navigation-submenu-on-safari.WordPress_gutenberg.-.10.October.2023.mp4
Safari
test64.-.10.October.2023.mp4
How?
I added some JavaScript logic that, when the submenu button is clicked, it focuses it. It'd be great if someone from the accessibility team could double-check if this approach is correct or if it will break something I am unaware of. cc: @afercia, @alexstine , @jeryj , @joedolson
Testing Instructions
activeElementis the arrow button.On iOS: