report: close drop down menu when focus is lost #10208
Conversation
Summary Problem: The report drop down menu does not close when the escape key is pressed in the devtools. This creates a challenge for customer relying on the keyboard that wish to dismiss the drop down. This also does not align with the experience of the WAI ARIA practice navigation menu button example [1]. Root cause: The escape key event is captured and not propagated by the devtools to toggle the console drawer. Solution: Close the menu when focus moves out of the menu. This better aligns the menu with the Navigation menu button example for which its interaction design is based on [1]. Code Change * Add a menu focus out handler to close the drop down menu * Unfortunetly JSDOM does not support focusout events so unit tests are not possible at this time [2]. [1]: https://www.w3.org/TR/2019/NOTE-wai-aria-practices-1.1-20190207/examples/menu-button/menu-button-links.html [2]: jsdom/jsdom#2708 Related PR 9433
| // The focusout event is not supported in our current version of typescript (3.5.3) | ||
| // https://github.com/microsoft/TypeScript/issues/30716 | ||
| const focusEvent = /** @type {FocusEvent} */ (e); |
There was a problem hiding this comment.
Is there appetite to upgrade typescript?
There was a problem hiding this comment.
I'm always up for upgrading TS, but we may not be at critical mass yet ... @patrickhulce what do you think?
|
I am going to look into adding a unit test for onMenuFocusOut to improve code coverage per codecov results. |
| // The focusout event is not supported in our current version of typescript (3.5.3) | ||
| // https://github.com/microsoft/TypeScript/issues/30716 | ||
| const focusEvent = /** @type {FocusEvent} */ (e); |
There was a problem hiding this comment.
I'm always up for upgrading TS, but we may not be at critical mass yet ... @patrickhulce what do you think?
There was a problem hiding this comment.
Instead of a unit yet, what about a UX test? Could piggy back on the viewer's puppeteer tests.
I have just finished writing a few unit tests and will add this to this PR but I am happy to follow up with UX tests if you think they would be valuable.
I'm curious if this fixes the issue this tool menu is shown in the Print preview. Does it?
I do not know about the Print preview issue, do you have a link to the issue or repro steps? I will take a look.
If you feel that covers the new behavior well enough that is fine. Was just suggesting an alternative in case unit tests weren't possible.
no crbug, just something I noticed. Printing doesn't seem to be a feature anyone uses so no big deal, was just curious. If you choose the Print option from the tool menu and don't see this tool menu in the Print results, then you've fixed it :) |
I gotcha, the unit tests I added are good enough coverage.
Unfortunately this change did not fix Print preview :(
But this looks like a CSS issue, the menu is hidden by @media print {
.lh-tools__dropdown {
/* the drop down is not supported on paper ;) */
display: none;
}I will get a PR out with that fix shortly. |
Done: #10216 |
connorjclark
changed the title
…de review feedback
Summary
Problem
The report drop down menu does not close when the escape key
is pressed in the devtools.
Gif of bug: menu does not close when escape key is pressed and console opens
This creates a challenge for customer relying on the keyboard that wish
to dismiss the drop down.
This also does not align with the experience of the WAI ARIA practice
navigation menu button example 1.
Root cause
The escape key event is captured and not propagated by the
devtools to toggle the console drawer.
Solution
Close the menu when focus moves out of the menu.
This better aligns the menu with the Navigation menu button example for
which its interaction design is based on 1.
Code Change
are not possible 2, instead I have added unit test for the onMenuFocusOut handler.
Gif of bug fixed: menu does close when escape key is pressed and console opens
Gif of menu escape key closing the menu
Gif of menu tabbing closing the menu
Related Issues/PRs
#9433