-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update URLPopover role and focus return #61313
base: trunk
Are you sure you want to change the base?
Conversation
The behavior was implemented in #50903. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -3 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
Flaky tests detected in c6f1d5d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8921260980
|
Thanks for finding that, that's quite recent, so it surprises me. I don't think there was any inconsistency though. If anything it creates more inconistency. Try the following:
Same with other blocks that have link/url popovers. Another inconsistency is that you can focus the button within the social link block (focusing the button inside the block is different to focusing the wrapper of the block), hit delete and nothing happens. |
I think that's a bit different because in social icons we can only have links and therefore we have to open the link popover. I think it's more comparable to pressing backspace in an empty button, which deletes the button block.. I have no strong opinions on this one, but personally I feel it fine..
I agree that this is not ideal, but I think we should fix this, instead of removing the other behavior. |
I disagree, but that's ok. I think I'll remove it from this PR to unblock it and we can discuss it on an issue. To go into more detail, I don't agree that it's the same as the button block, text content is different to a url. An input within a dialog is also very different to rich text content. When the user inserts a button block, the caret is by default within the text content and in many blocks deleting all text content results in the removal of the block, so it's an expected behavior. For URLs in both the button and social link block, the user has to carry out a specific action to open the link dialog, clicking on a button. At this point the user understands they're in a dialog, so they would expect it to behave just like any other dialog, with all their input constrained. In no other dialog does deleting all the text remove the block. |
I agree with @talldan here. Backspacing inside a dialog should not remove a block. I'm not even sure how you make a case for good UX on something like that... |
…y times in URLPopover" This reverts commit 78a8521.
I've split out that change into another PR, so hopefully it fully unblocks this one - #61344. |
A couple questions:
|
@afercia It tests well for me.
Unless https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-modal Thanks. |
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.
@talldan Giving this an approve. Do these blocks have any type of E2E tests currently? If so, might be worth adding some basic focus tests.
@alexstine I'm not sure that's correct. Actually, the default value of aria-modal is false. Of course, when aria-modal is entirely omitted, there's no value and that should be treated as I'm guessing the fact a dialog content is restricted anyways by some screen readers when they find a By checking the Accessibility tab in the Chrome dev tools, it clearly shows the value of the modal property is ARIA Attributes:
Computer Properties
Screenshot: As such, I still think we should make a decision on whether to add an explicit aria-modal attribute or not.
One more question: should this semantics be added also to other popovers to insert links e.g. the LinkControl component? |
@afercia Yeah, it's a fair point. It looks like NVDA anyway treats https://a11ysupport.io/tech/aria/aria-modal_attribute#age-of-results I say this gets merged, doesn't look like this would hurt anything. Just not sure it really does anything at this point. Thanks. |
Yes, That is the reason why the Modal component does not use Regardless my question is still: Do we want this Popover to be 'modal'? I still think that making the link editing experience is important so if |
What?
Fixes a few a11y things for the social links block (that I noticed when working on it recently) and media placeholders
Social Links:
URLPopover
, which didn't have a role (I think it should be role="dialog") and the opener button also didn't havearia-haspopup
specified, so I've added it with the value 'dialog'.Media Placeholder (e.g. on the image block):
URLPopover
with no role, and there's noaria-haspopup
on the opener button. This is fixed.Testing Instructions
(use a screenreader)
Social links
Media placeholder