Skip to content
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

Try to make the inline toolbars navigable toolbars and make Alt+F10 work #6302

Closed
wants to merge 2 commits into from

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Apr 20, 2018

Work in progress, don't merge.

Inline toolbars should work like the block toolbars:

  • Alt+F10 should move focus from the editable field to the inline toolbar
  • arrows navigation should work in the inline toolbars
  • Escape should move focus back to the editable field

This was quickly discussed in #5840 together with a couple other issues. Was thinking a bit at this and seems to me the most logical thing would be wrapping the inline toolbar within a NavigableToolbar component. And it works, in a way. It's amazing to see how reusing a component in a bit unexpected way works because the component is abstracted enough that it just works 🙂

However, playing with this highlighted an issue with focusToolbar() because it assumes there's just one toolbar. Actually, there may be up to three toolbars:

screen shot 2018-04-20 at 08 43 43

Also on current master, when pressing Alt+F10 focusToolbar() returns two toolbars (depending also on the "fix to top" option) and it works just by coincidence because the last toolbar happens to be the right one. I guess some DOM methods could help getting the right toolbar but that would be fragile and non-Reactish. The main issue I see here is that when pressing Alt+F10, the focus/selection methods used here don't know 1) the selected block 2) the field being edited 3) which toolbar is the one to move focus to.

Considering also some blocks, for example the cover image, are going to be nested blocks, how can we preserve the Alt+F10 functionality and give the application knowledge of which toolbar to focus?

/Cc @aduth @youknowriad

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress labels Apr 20, 2018
@afercia
Copy link
Contributor Author

afercia commented Apr 20, 2018

Latest commit tries to cover the point 3 from #5840 (comment) that is: in some cases the current selection is in another block, so focus gets moved back to a wrong block. Introduced a check for that and if that happens, focus is now moved back to the wrapper of the right block.

Also, while testing the nested cover image PR, see #5452, I've noticed in some cases a JS TypeError happening because nodeType can be null. The thing is, seems to me the window.getSelection() implementation across browsers is not fully conforming. From what I can read in the spec, it should return null when there's no selection. However, at least in Chrome and Firefox, it always returns a selection object so checking for if ( ! selection ) is pointless.

@youknowriad when you have a chance, I'd appreciate your thoughts on the latest commit. You can test it following the steps described in the point 3) on #5840 (comment) Thanks!

@afercia
Copy link
Contributor Author

afercia commented Apr 21, 2018

I'm noticing some inconsistencies in how this component logic works, for example with blocks that don't have editable areas:

screen shot 2018-04-21 at 15 20 44

Testing with Chrome (similar results with Firefox):

  • click using the mouse close to the text "Drag image..."
  • press Alt+F10 to move to the toolbar, press Escape to move focus back
  • at that point there's a selection object, the focusNode is a text node
  • current code assumes it has to get the text node parentElement (yeah this works but only when starting from an editable area)
  • in this case, the parent node is: <div class="components-placeholder__instructions">Drag image here or add from media library</div> which is not focusable
  • thus, nothing happens and focus stays on the toolbar

This happens also when using the keyboard, for example when starting form the "Media Library" button then press Alt+F10 and follow the steps above.

The main goal should be trying to move focus back to the invoking context so I'd like to propose this fallback:

  • when pressing Escape on the toolbar...
  • check if the selection was in an editable area, if yes: move focus back to the selection
  • otherwise, move focus back to the element that was document.activeElement
  • as last resort, move focus back to the block focusable wrapper

This way the component would avoid to make assumptions on the DOM structure, like in using parentElement.

@afercia
Copy link
Contributor Author

afercia commented Apr 22, 2018

One more case where focusSelection() fails is for example in the Quote block. While the citation is within a <cite> element that is contenteditable and focusable, the quote itself is within a paragraph:
<p>Some quote</p>

which is not focusable while trying to get parentElement assumes it is a focusable element.

@afercia
Copy link
Contributor Author

afercia commented Apr 24, 2018

One more issue related to getSelection() (I'm considering to get rid of it):

  • with Firefox, go to any non-Gutenberg page in WordPress, don’t touch the interface and run this in your console: window.getSelection()
  • anchorNode and focuseNode are null, as expected
  • go in a Gutenberg new post, don’t touch anything and run window.getSelection(): null as expected
  • go in an existing Gutenberg post, run window.getSelection()
  • I get a script element as anchorNode / focusNode 😬
  • doesn’t happen in Chrome

We've reproduced this internally at @Yoast and have no idea why it's happening.

@aduth
Copy link
Member

aduth commented Sep 13, 2018

Is there still a relevancy / desire to get this in? Anything I can do to help help push it forward?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018
@afercia
Copy link
Contributor Author

afercia commented Oct 12, 2018

Is there still a relevancy / desire to get this in? Anything I can do to help help push it forward?

@aduth sorry for late reply. Yes: it would be great to have inline toolbars have the same keyboard interaction and shortcuts of the block toolbars. I wouldn't know how to solve the focus issue though, and I'm not sure I have the bandwidth to move this PR on. I think you've also faced the same issue in #10529

@afercia
Copy link
Contributor Author

afercia commented Nov 19, 2018

I think this can be closed for now: many things have changed in the meantime. This was a good exploration and can always be reconsidered later.

@afercia afercia closed this Nov 19, 2018
@afercia afercia deleted the try/navigable-toolbar-on-inline-toolbars branch November 19, 2018 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants