-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global Header: Add in styles to fix older browsers. #178
Global Header: Add in styles to fix older browsers. #178
Conversation
6fbc270
to
fcf6262
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visually this looks good (tested on Chrome 87), but the dropdowns can't be accessed by keyboard anymore. I think you can use :focus-within
on the same element as :hover
, but maybe double check the browser support.
Thanks for catching that! I'll take a look. |
Ok, using I tested all the way back to Chrome 68. However, Falkon in my Virtual Machine won't tab into the navigation at all, which is a bit bizarre. It should work... 🤔 If we are okay with these changes, we could merge this PR, and reopen a new ticket with a smaller scope if that is still in fact a problem. It will give people a better ability to test these changes with little risk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working with keyboard nav & screen reader (VO).
I left one comment that would be an improvement, but it's not a blocker since that's also how it works on production.
align-items: normal; | ||
opacity: 0; | ||
transition: opacity 0.1s linear; | ||
visibility: hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this rule necessary? Technically it removes the element from the accessibility tree, so it no longer shows up in VoiceOver's links menu. The links are still accessible on the page, it's just the menu in the rotor that's missing it.
The risks you identified would now apply to all browsers, rather than just the older ones. We'll need to maintain the CSS, so if the nav block markup changes, our CSS won't know about that. Honestly that's probably true for other things on the site too, so it's not a blocker. |
a64abfa
to
8d55a87
Compare
This PR copies in Gutenberg Navigation styles that include the
:where
CSS declaration which causes unusable layouts in older browsers.Fixes #159
Why does Gutenberg use
:where
?Gutenberg uses these conditional css selectors because the Navigation component has a "Open on click" feature:
When that feature is toggled on, the
hover
functionality is not added to the submenus.What are the risks?
There isn't much risk since the CSS is implemented behind the
supports
declaration, however:Screenshots
Environment: Windows 8.1 - Chrome 69
Incidental Changes
@supports
, we need to updatepostcss-preset-env
. The current version is using an olderautoprefixr
version that does not support@supports
.How I tested this fix
style.css
frombuild
folderstyle.css
source via chrome inspector tools with copied source.I also did the same thing using the Falkon browser in a virtual machine.