Update URLPopover role and focus return #61313
Conversation
talldan
added
[Type] Bug
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: +98 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 79b16c1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9558372990
|
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... |
|
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. |
@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 |
talldan
force-pushed
the
fix/url-popover-role-and-focus-return
branch
from
b64b824 to
79b16c1
Compare
|
Apologies for not following up on this one for a while. It's been a busy time. I have more time for code contributions now, so following up on some older PRs.
While tab navigation is constrained, I don't think other parts of the UI are made inert. It seems like both I personally felt that an important aspect of the PR is for the user to be aware the opener button will send focus to a popover context with constrained focus and that there affordances like escape to close the dialog, as without it I imagine it can be quite confusing. There aren't too many implementations like this across the codebase fortunately, I think the LinkControl and the color controls in the inspector are two that I can see. For a lot of these popovers, they seem to sit in a grey area where they're closer semantically to menus, but not quite 😬 |
Not sure I follow. Disclaimer: It could be because of the 33 Celsius (91 Fahrenheit) temeprature right now in my city. |
😅 In the MDN article it says "If a dialog is not modal — there is no inert background and focus isn't confined to the dialog — either include aria-modal="false" or omit the attribute altogether." In the case of URLPopover, focus is confined but the background isn't inert (at least it didn't seem to be when I checked), so perhaps it doesn't meet the criteria for Perhaps it's bad wording in the MDN article, the actual spec doesn't seem to clarify exactly what |
|
Thanks @talldan for clarifying. Yes that wording could be improved.
So basically there's an inconsistent user experience for keyboard users and screen reader users. That is why I asked if we want to make these popovers be really 'modal' for everyone and provide a consistent experience. |
|
I think we're on the same page, and I have the same question. I can do some research into what it would take to make them have modal like behavior. I also wonder now whether dropdown menus should also be modal too. |
|
@talldan @afercia Let's go ahead and add
Worth noting, inert is not required by this attribute in the 1.2 version.
I do not think the word "should" is equal to the word "required". Docs: https://www.w3.org/TR/wai-aria-1.2/#aria-modal EDIT: If we do decide to mark the content inert, let's simply copy how the modal component does this already. Thanks. |
Ideally, it would be nice to go a little further. |
…y times in URLPopover" This reverts commit 78a8521.
talldan
force-pushed
the
fix/url-popover-role-and-focus-return
branch
from
79b16c1 to
4483baa
Compare
|
Thank you for the ping! We are indeed in the process of rewriting a bunch of components (using And bonus — here's an article that I find as a useful reference when discussing popovers, dialogs and modality: https://hidde.blog/dialog-modal-popover-differences/ |
|
I've added |
* Return focus from URLPopover in social link block * Fix media-placeholder focus return and aria-haspopup * Label the URL Popover * Adjust labelling * Revert "Avoid deleting the block when pressing delete key one too many times in URLPopover" This reverts commit 78a8521. * Add `aria-modal="true"` to URLPopover ---- Co-authored-by: talldan <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: alexstine <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: ciampo <[email protected]>
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-haspopupspecified, so I've added it with the value 'dialog'.Media Placeholder (e.g. on the image block):
URLPopoverwith no role, and there's noaria-haspopupon the opener button. This is fixed.Testing Instructions
(use a screenreader)
Social links
Media placeholder