Make social icon navigation one arrow keypress #64883
Conversation
|
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. |
jeryj
added
the
[Block] Social
|
Size Change: -99 B (-0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8076181. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10703879170
|
|
@jeryj Make sure this doesn't break other interactions such as multiselect. I tried to globally remove this |
|
@jeryj Thanks for this PR. Quickly tested and it looks like a good first step. Some considerations: The most important point is that arrows navigation wouldn't work well for Windows screen readers unless we make sure the screen readere switches to 'focus mode'. Otherwise, arrows are reserved for internal usage in screen readers 'browse more'. IN other cases, the editor uses an ARIA role to do this for example the Secondly: while it makes totally sense to use a list on the front end, why we're using a list in the editor? It doesn't make much sense to use a list and then override its semantics by using The |
There was a problem hiding this comment.
To me this feels like an improvement over what we have currently (i.e. two key presses to navigate between link items).
The only caveat is if this breaks multi-select like @alexstine suggested. I wasn't sure how best to verify that one. I was trying SHIFT + ARROWs to select...
|
@afercia The only solution here is using role application for the editor canvas but it's a huge lift and requires tons of testing. As I've stated in the passed, role document is my fault because I attempted to fix a legacy JAWS bug. That was a lesson learned on my part. We used role group before but even that was bad and would not fix this issue. Removing the role all together from Someone will have to take this on one of these days and then fix the endless E2E tests. Furthermore, we'd need real user testing of every block and situation to ensure we're not making the editor worse than it currently is. I do not think we can remove the ability for the block wrapper to gain focus though, this totally removes the ability to easily delete or insert a block after the current position. Admittedly, it is a little known feature to keyboard users. |
|
Thanks for the catch, @alexstine! I was assuming this fix was too good to be true :)
Yeah, you're right. Thanks for catching this before I went too far down the path!
@afercia I believe to make sure CSS styles match so that the editor is an accurate representation of the frontend site. Some potential solutions:
I'll play around with them and see if there's something we can do to make this better. |
|
@jeryj What if you just add the block props to button and adjust the CSS styling? Although, I am wondering why |
Technically, it's possible to make any element behave like a button but that would mean replicating all the semantics and interaction of a native button, including making sure the button can be acitvated on spacebar keyup. It's so overkill that I wouldn't recommend it. |
jeryj
force-pushed
the
fix/double-arrow-social-icons
branch
2 times, most recently
from
bb6c8be to
c8cf43f
Compare
|
I ended up moving the |
|
To my understanding, by moving the props from the
|
@afercia My suspicion from the code is that the |
|
@jeryj thanks for your feedback. I have no strong opinions on whether to consider the social icons in the editor 'blocks' or 'buttons', the most important thing is that there is a focus style whatever its shape is 🙂 |
getdave
force-pushed
the
fix/double-arrow-social-icons
branch
from
f49c692 to
49c47e2
Compare
|
I rebased here because my recent changes to the UX in Core weren't reflected but it seems I made a mistake and reverted @jeryj's progress. He will have his local copy which he can use to update this when he's back. My apologies - it was early in the morning 🤦 |
jeryj
force-pushed
the
fix/double-arrow-social-icons
branch
2 times, most recently
from
f49c692 to
eb95d1b
Compare
|
It happens! Rebased and force pushed :) |
More reviews and testing I believe. |
getdave
added
[Type] Enhancement
|
Anyone willing to give it an approving review? |
|
To my understanding, the markup on the front end doesn't change and that's great. Before: After: Since the button native role is now overridden by For screen reader users this makes ery difficult to understand how to interact with the social icons nested blocks and at this point I'm not sure moving all the props to the button is the best option. A few screenshots to better illustrate: Before: The LI element is the block and it's announced as such:
Navigating with Control+Option+Right arrow to the button, the button is announced as a button with a dialog pop-up, which is what I would expect:
After: The LI element is still announced as block, probably because it now contains the block:
Navigating with Control+Option+Right arrow to the button, the button is not announced as button any longer. The only thin that is announced is the aria-label
Basically, while moving the props to the button solves the keyboard interaction issue, it removes any information on how to interact with the button, which is a problem for screen reader users. The root problem is that each |
|
I guess another option is to remove the |
|
Couple thoughts: 1 2
However, when tabbing into the inner block, the Button block is skipped and focus goes directly to the contenteditable.
|
|
That's super helpful detail @afercia 👍 |
That's what I tried initially, but it breaks using the delete button to delete a block.
I believe if there is a focusable element inside the block, focus is supposed to be transferred to it automatically. Maybe there's a bug there that's breaking this. I'll investigate. |
Looked into this more. The current trunk behavior is technically correct if the
I think this is due to the button being a contenteditable. Also, as a result of this behavior, you can't have a selection on a single button or navigate between buttons easily. For example, To delete it, you have to delete all the text and press delete again. You can't arrow between the button blocks. I would prefer to be able to use the arrow keys to navigate between the button blocks instead of it automatically focusing the contenteditable. Screen.Recording.2024-10-02.at.11.42.42.AM.mov
If we preserve this route of moving the blockProps to the button, the main issue is that the social icon button block is not announced as a button so screen reader users don't have any idea that the element is interactive. If I can get the button to announce as a button instead of role=document, I think we could continue with the current route? Would that address the issue? |
jeryj
force-pushed
the
fix/double-arrow-social-icons
branch
from
8aeebee to
411c567
Compare
|
I added the role=button as an override, and the semantics seem much better. Screen.Recording.2024-10-02.at.11.56.34.AM.mov |
|
The block itself still really isn't focusable though. You think users will figure out they can press delete on the button to remove? The whole concept of blocks in Gutenberg drives me up a wall if I'm being honest. Block and block content, I have no idea how we ended up in this world where some have wrappers, some do not. |
Can you explain this more? It seems focusable in my testing. It's announcing "Block: Bandcamp, dialog pop up, button" and seems to have all the keybindings of a block. |
|
@afercia - Do you want to give this another round of testing before merging? I know it's a lot to go over. |
There was a problem hiding this comment.
I tested this and found the interactions to be much better. Combined with the changes to the inserter this whole block now feels a lot easier to use 🎉
I tried to break this by changing colors, applying block styles and various things but I couldn't see any regressions.
Not sure if you wanted to add a quick set of tests here to ensure there are no future regressions to the focus handling?
Overall, great job 🙇
|
@jeryj thanks for the ping. Keyboard interaction is way better now, thank you!
Genuine question, I don't remember: why not 'omit' the Nitpick: Basically the |
By moving the blockProps for the social icon from the <li> to the <button>, we get the appropriate behavior of being able to select and create social icons with a clean keyboard behavior.
…tent with other focus styles
@afercia Thanks for reviewing this again! If we don't explicitly add the
Thank you for that catch! Updated 👍🏻 |
jeryj
force-pushed
the
fix/double-arrow-social-icons
branch
from
411c567 to
64ca1c9
Compare
Fixes #64937
What?
Moves the
blockPropsfor the social icon from the<li>to the<button>.Why?
<li>and one to the nested<button>.<li>did not have an onClick to open the popover but the<button>did, meaning you can only press enter to open the popover when on the ` but you can't tell which one you're focused on.How?
By moving the
blockPropsfor the social icon from the<li>to the<button>, we get the appropriate behavior of being able to select and create social icons with a clean keyboard behavior. The caveat is to preserve the CSS, we needed to add the.wp-block-social-linkto both theliand thebutton. This allows us the least amount of changes to the CSS, preventing a lot of potential CSS conflicts and styling issues for sites.Testing Instructions
Check for visual regressions on both the frontend and editor
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-08-30.at.10.27.19.AM.mov