Block Editor List View: use anchor elements instead of buttons #35655
Conversation
|
Size Change: +54 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
ramonjd
added
[Package] Block editor
|
Need to eventually update selectors for the E2E tests to pass. |
ramonjd
changed the title
| // We need to clear any HTML drag data to prevent `pasteHandler` from firing | ||
| // inside the `useOnBlockDrop` hook. | ||
| const onDragStartHandler = ( event ) => { | ||
| event.dataTransfer.clearData(); |
There was a problem hiding this comment.
The href attribute is triggering the browser's native drag + copy link. (More information on the browser's drag operations)
It's populating the DataTransfer object with text/html, which is triggering a paste command in onHTMLDrop used by useBlockDrop
This effect is causing the e2e failure.
Clearing the data using event.dataTransfer.clearData(); seems to address this.
Are there better ways to handle this event?
Maybe useOnBlockDrop could take a prop that explicitly defines/overwrites the onDrop functionality so we wouldn't have to create extra functions in our components.
ramonjd
force-pushed
the
try/block-editor-use-anchor-links-in-list-view
branch
from
f69afaf to
23f812f
Compare
| @@ -155,7 +155,7 @@ describe( 'Navigating the block hierarchy', () => { | |||
| // Return to first block. | |||
| await openListViewSidebar(); | |||
| await page.keyboard.press( 'ArrowUp' ); | |||
| await page.keyboard.press( 'Space' ); | |||
| await page.keyboard.press( 'Enter' ); | |||
There was a problem hiding this comment.
We need to change the key here because:
Buttons are expected to be triggered using the Space or Enter key, while links are expected to be triggered using the Enter key.
See also: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a
ramonjd
changed the title
packages/block-editor/src/components/list-view/block-select-button.js
Outdated
Show resolved
Hide resolved
ramonjd
force-pushed
the
try/block-editor-use-anchor-links-in-list-view
branch
from
23f812f to
9918ef3
Compare
There was a problem hiding this comment.
Thanks for exploring this @ramonjd! I noticed a couple of differences with the switch to using an anchor tag, that I wondered might be potentially confusing for users who are used to the current behaviour via the keyboard. Now that space no longer activates the item in the list, it will scroll the list view down a page (and the currently active item is then scrolled out of view within the list):
Also, by default on Mac, Firefox doesn't tab to a tags (unless we update system preferences) so I noticed that the highlighted list item is no longer in the tab sequence.
I'm not sure how frequently either of these actions are used, but if it does introduce an unexpected change, would it be worth us looking into the following?
- Add
tabindex="0"to the currently highlighted list item so that it's ensured to be in the tabbing sequence (in case users currently expect it to behave like a button) — it looks like there are a few cases of this throughout the codebase, so it might not be all that strange to add it in? - Add a
spacekeyboard event handler to ensure that pressingspaceon a list item behaves like it would if it were a button?
Of course, I also understand if part of the switch to the different HTML tag is that we will have different behaviour. I suppose for my own personal preferences, I like the feeling of a button, even though semantically it's more of a link 😃
|
Thanks for taking the time to review @andrewserong Definitely some things we need to address.
Huh, for me it's the opposite. Tabbing works as expected on Firefox (I have Keyboard navigation control focus switched off in MacOS) but in Chrome I can only use the arrow keys 🤷
I think it should be consistent either way, as you hightlight. It doesn't seem acceptable to mark up anchor tags as buttons, when semantically, we want an anchor tag here, so maybe we'll need to look at the tabIndex and events or a combination of both. Anchor tags with
TreeGridCell employs a roving tabIndex, which I haven't looked into yet, but it seems to be assigning our I'll investigate this further. Hardcoding
I did consider this, but hesitated after learning that selecting anchor tags with the space key isn't default behaviour, so I didn't want to confuse things further for folks expecting certain behaviour. 😆 But I was probably overthinking things. There's a line on which to balance somewhere - we just need to find it. cc @diegohaz who will know better than I where that line is 😄 I think there's a sound case to keep default behaviour, especially if removing it causes a negative user experience. It's probably worth experimenting with an |
ramonjd
force-pushed
the
try/block-editor-use-anchor-links-in-list-view
branch
2 times, most recently
from
9f1d68a to
1e91c55
Compare
|
I've rebased to get the latest changes from #35706. Also trying out adding a key event handler for space/enter keys, and hardcoding cc @talldan who appears famously in the history, just in case there are some historical requirements that I've missed. |
…n href. This allows us to remove the custom CSS for the native anchor element.
…ton element behaviour, even though we're switching to an anchor element. Forcing a tabIndex of `0` so the anchor elements are tabbable.
…ace. Reinstate tabIndex
ramonjd
force-pushed
the
try/block-editor-use-anchor-links-in-list-view
branch
from
6beaeb3 to
48e288c
Compare
There was a problem hiding this comment.
@ramonjd thanks for re-instating the tabIndex. This is testing well for me in Chrome and Edge (on Mac), but I noticed in Firefox and Safari, the button (anchor tag) doesn't appear to be accessible via the tab sequence, even though in the markup tabindex="0" is set for it. Also in those browsers (FF and Safari) there appears to be an error thrown in tabbable.js at some point in the tabbing sequence (in findPrevious). Not sure if that's related?
Here's the inspector view where we can see tabIndex is correctly set to 0 for the first element, and a tiny gif with the error message:
| tabIndex is set correctly | error while tabbing in FF |
|---|---|
 |
 |
Also, this change adds the block id to the URL after clicking on it in the list. Is that intentional, or an issue at all?
I think this is due to the selectEditorBlock callback that's passed to onClick using event.stopPropagation() on this line, so the default anchor tag behaviour is taking effect. But, it looks like switching to event.preventDefault() might fix it? (Though I'm not sure if that would have other side-effects, too 🤔)
|
I appreciate the 🦅 👀 again @andrewserong !
I'm aware that Safari has, in the past, required a little nudge to enable tabbing. For me, tabbing to the list works when I enable:
This is what I'm seeing in Firefox. I seem to be able to use it according to the parameters in #35655 (comment)
True. Hmm. I'm not sure about that one. It's the writing flow component, with which I'm not terribly familiar. I'll take a closer look.
I saw this as an intentional consequence of using internal anchor tags, and an effect that is consistent with the Document Outline links.
I feel that there is a desire to have the anchor tag to achieve semantic nirvana, but we also want to preserve all the built-in x-browser behaviour of a button. That might be trickier than we thought, especially given that some users have to enable keyboard navigation in OSX for things to work as they should in Safari and Firefox. |
Having said that, there is the option of accepting the limitation as one set by the operating system and trust that users who rely on tabbing would know how to enable it fully in their settings 🤷♂️ |
Ah, nice, given the consistency there, that sounds good to me 👍
This does appear to be the crux of the issue! I think my only real hesitancy with the change is that semantically it makes better sense as a link, but because it's already been a button, for users in FF on Mac that are used to being able to tab to it, will it feel like a regression given that the OS settings are a little buried?
I wonder if we can gather more feedback from some folks with more specialisation in a11y, to see whether this is an expected issue, or if it'll be a frustration? For the error with |
ramonjd
added
[a11y] Keyboard & Focus
Needs Design Feedback
I'm with you on this one.
Good call. It would be helpful to get some insight on whether this change would benefit accessibility, and, if so, by how much. I'll try to find the right folks to ping. |
|
@ramonjd What is changing here? E.g. where can I go to see this in action? Can you list for me specific steps I should perform to see the change? Happy to provide feedback once I have additional needed context. Thanks. |
|
Thanks a lot for swinging by, @alexstine ! We'd really love some opinions on this one.
In the Block Editor, we can open the Block List by clicking on the
This opens a "tree" that represents all blocks currently in the editor. Clicking on an item in that tree will highlight, and select, the corresponding block in the editor. The primary change this PR tries to introduce is turning that block list tree item from a button element into an anchor element. Before:
After:
The reason why we're trying this is to satisfy the requirements laid out in #11711
Buttons have some default behaviour in browsers, e.g,. they're tabbable, and triggered by Space and Enter. Switching to an anchor tag means that we'd have to either emulate or abandon this behaviour. So we're trying to weigh up the pros and cons in terms of accessibility, semantics and existing behaviour. What do you think? Let me know if you need more context. 🙇 |
|
@ramonjd I am a blind screen reader user and I could not find any obvious regressions or negative impacts from this change. Other members of the Accessibility team might have thoughts about any possible visual changes, but it seems to work no different with NVDA on Windows with Google Chrome. Is there a platform I should be testing on? I am running Windows 10 but also have Mac OSX. |
I appreciate you going through this @alexstine, it's very helpful, thank you! If you have time to spare and could test these changes on Mac OSX, it would be great. Mac OSX is the operating system we're most concerned about given that tabbing of anchor elements has to be enabled in the System settings. The following link explains how to turn that on: https://support.apple.com/en-us/HT204434#fullkeyboard I tried turning on the screen reader in Mac OSX and could navigate around, but I'm not used to using them so my experience might not be the best guide 😄 |
|
Regarding MacOS, I'd observe that people who depend on keyboard navigation are likely to also be people who have already turned on the relevant options in their operating system. This is less likely to impact people who depend on keyboard navigation significantly; mostly people who use it as an optional navigation method. |
Thanks for the thought @joedolson I'd tend to agree. Citing my own experience, the frustration and annoyance at not being able to use keyboard navigation by default fuelled my search after where to adjust my settings on my MacOS machine. 😄 Taking on the OS from the browser doesn't seem like a fair fight to me. |
|
@ramonjd I just tested this on Mac and did not notice any different behavior between trunk and your branch. The only difference was links instead of buttons. I can't say that this PR negatively impacts any of my accessibility testing, so it's a pass from me. Thanks. |
This is a colossal help. Thank you! 🙇♂️ |
@andrewserong I was looking at this and I can reproduce in Chrome and other browsers. As far as I can see, the error is thrown in tabbable.js because the focussed element doesn't appear in the collection of focusable elements. The So when I logged things out like if ( index > -1 ) {
focusables.length = index;
} else {
console.log( 'element index', element, index );
}The element is, more often than not, |
|
@ramonjd That is the accessible navigation for screen readers I believe. Every time you press Tab, the button dynamically updates to the next block with useEffect. I think that's how it works, I've only played with that a little bit. |
There was a problem hiding this comment.
Thanks for your patience, and for investigating the tabbable.js issue @ramonjd! I can replicate it on trunk, too — I think I mustn't have been tabbing far enough forwards and back when I last tested it there, so apologies for missing it!
I've just re-tested this PR in Safari, Chrome, FF, and Edge on Mac, and it's all working well for me when I enable Use keyboard navigation to move focus between controls, and it sounds like it isn't a deal breaker requiring this for Safari and FF as it matches OS behaviour like you've mentioned. I haven't been able to break it in any other ways, either 😀
This LGTM, and we can always follow up on the tabbable.js error if we need to separately, particularly given that it's in a separate package and doesn't affect any user behaviour.
Thanks again for working on this!
Thanks for retesting! 🍺 I'll write up an issue while the memory is still fresh. |
Description
Resolves #11711
Related to #29733 (tracking issue) and has origins in #7142
This PR swaps out the
<Button />element with an<a />. The List View tree items are used to navigate to targets on the same page.For more reading on Links vs Buttons see:
If you want a user to navigate to a new page or to a different target on the same page, use an anchor element .
Oct-22-2021.11-25-50.mp4
One point to consider, suggested in #7142, would be:
How has this been tested?
Check that:
<a />, focus actions are expected to be triggered using the Enter key.Types of changes
Markup changes to the Block List items for accessibility.
Checklist:
*.native.jsfiles for terms that need renaming or removal).