Revert "Revert "components: Promote VisuallyHidden from ui into full components (#31244)"" #31902
Conversation
sarayourfriend
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Size Change: -558 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
I'm currently investating why VisuallyHidden's styles aren't being applied. This seems like a bug in our
Currently pretty stumped on this one. |
sarayourfriend
added
[Feature] Component System
|
I've stumbled upon something interesting. It appears that |
|
I wonder what the issue is here? Seems like it could be a bug but we're on Emotion 10 so I'm not sure it's worth filing an issue for it. What do you think @youknowriad? Should we prioritize upgrading to Emotion 11 to see if it fixes this issue? I can't find anything about this online at all. |
That sounds like a good plan to me. Do you know how difficult would that be? |
|
#31476 is currently blocked by an issue with webpack (summarized in the PR) |
sarayourfriend
force-pushed
the
revert-31882-revert/visually-hidden-component
branch
from
9470a35
to
c0f26f7
Compare
|
Okay after rebasing I've found that the Emotion 11 upgrade does not fix the issue we're seeing. What's more, I'm able to reproduce the issue in this codesandbox: https://codesandbox.io/s/relaxed-browser-4fe79?file=/src/App.js It might be worth fiddling around with that codesandbox until we can isolate where the issue is coming from. |
sarayourfriend
force-pushed
the
revert-31882-revert/visually-hidden-component
branch
from
c0f26f7
to
58b59d6
Compare
|
Okay, so per emotion-js/emotion#2409 we'll move just using |
I think the correct way to test this is to focus a piece of text using a screen reader. @diegohaz is that correct? I'm worried that adding a story with say, a visually hidden button would be misleading. Are we supposed to visually hide buttons or other similar elements like that? |
sarayourfriend
force-pushed
the
revert-31882-revert/visually-hidden-component
branch
from
651cf43
to
35892e1
Compare
I think it would still make sense — an example that comes to mind for this use case is a "Skip to content" button. The reason why I was proposing to add a story is that we have specific styles that will show the content of gutenberg/packages/components/src/visually-hidden/styles.js Lines 24 to 38 in 35892e1 |
|
Yeah for what it's worth you can test this in the browser by inspecting the element and forcing the
|
My point about adding a story was to illustrate to the users of this component an example for when those <VisuallyHidden as="Button" variant="tertiary">Skip to content</VisuallyHidden>` Before approving, I'd also like to hear @diegohaz 's opinion around the way this component handles focus (see
|
Sorry for the delay. I missed this one in my notifications. I don't think this component should be used as "skip to content" links. I'd only use this to hide things from sighted users (including keyboard users that would benefit from the "skip to content" link) while keeping them visible for screen readers. A common use case for this that we can add as a story is a <p>
In addition to SmartThings' own family of sensors, you can also control hundreds of connected devices from other brands through the SmartThings open platform.{ ' ' }
<a href="#">Learn more<VisuallyHidden> about connected devices</VisuallyHidden></a>
</p>That would be useful so screen reader users scanning links in the site would have more context other than a bunch of "Learn more" links. Regarding "skip to content" links, these are the styles I've seen out there: left: -999px;
position: absolute;
top: auto;
width: 1px;
height: 1px;
overflow: hidden;
z-index: -999;
&:focus,
&:active {
background-color: white;
left: auto;
top: auto;
height: auto;
width: max-content;
overflow: auto;
padding: 1em;
margin: 1em;
text-align: center;
z-index: 999;
} |
|
If we remove the |
Yes! |
sarayourfriend
force-pushed
the
revert-31882-revert/visually-hidden-component
branch
2 times, most recently
from
d95518e
to
5c9628d
Compare
There was a problem hiding this comment.
This looks good to me. Thank you for working on this @sarayourfriend! 🎉
sarayourfriend
force-pushed
the
revert-31882-revert/visually-hidden-component
branch
from
411a324
to
cfe4ba2
Compare
|
I'm not a big fan of this refactoring. Do we even need a component context system for visually hidden components? The whole point of Related CSS for WP-Admin: and |
Reverts #31882