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

Navigation mode: polish enabling when no block is selected #19298

Merged
merged 3 commits into from Dec 23, 2019

Conversation

@ellatrix
Copy link
Member

ellatrix commented Dec 23, 2019

Description

Polishes #19238: don't enable navigation mode on tab, but rather use the FocusCapture elements to detect incoming keyboard navigation. This fixes an issue where navigation mode is enabled after tabbing outside the block list (e.g. document sidebar) and then clicking on a block.

To do: add e2e tests.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@ellatrix ellatrix requested review from talldan and youknowriad as code owners Dec 23, 2019
@ellatrix ellatrix added the [Type] Bug label Dec 23, 2019
Copy link
Contributor

youknowriad left a comment

So I remember properly, this behavior is related to the "PostTitle" component where sometimes we tab through it and sometimes we don't right?

What if instead of trying to hack around "auto-enabling" the navigation mode, we make sure the "PostTitle" behaves like a block. I guess thisi means when we tab out from it in edit mode, instead of going to the next block, we just skip all the blocks?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

@youknowriad It's related, but it should work from both sides. When I tab from the header to the block list, and no blocks are selected, navigation mode should be enabled. When I tab backwards from the document sidebar, and no blocks are selected, navigation mode should also be enabled. I do agree that the post title should behave as much as possible like a block, or be a block, since it's wrapped in WritingFlow.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

I guess what I don't understand is why we auto-enable Navigation mode. I think explicitness is better in most cases.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

@youknowriad Yes, it would be better if navigation mode were enabled by default. This would make most sense because you Escape from Edit mode to Navigation mode, and Enter Edit mode from Navigation mode. But since you don't land in Edit mode on click, we can't have it on by default I guess. Otherwise, if you load a page and click on a block, you're still in Navigation mode.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

Yes, it would be better if navigation mode were enabled by default.

We had it by default and it broke other stuff (first focus in the post title) and also a lot of confusion from users so we reverted.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

Exactly, so we can't enable it by default, and this seems like the right solution.

May I ask why it is ok to auto-enable Edit mode when using the mouse, but not ok to auto-enable Navigation mode when using the keyboard?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

May I ask why it is ok to auto-enable Edit mode when using the mouse, but not ok to auto-enable Navigation mode when using the keyboard?

Good question :) I guess I'm biased here but the thinking for me at least is that "double click" explicitly means "edit" whlie "tabbing" not necessarily "navigation mode" (since you can also tab in "edit" mode)

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

That's a good point though.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

So the double-click is a bit like tabbing into the block list without any block selected. :)

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 23, 2019

Ok you convinced me :P

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

@youknowriad Thanks for the discussing and review. I'll add an e2e test for this behaviour before merging.

ellatrix added 2 commits Dec 23, 2019
@ellatrix ellatrix requested review from ajitbohra, nerrad and ntwb as code owners Dec 23, 2019
@ellatrix ellatrix merged commit 07153e7 into master Dec 23, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the fix/navigation-mode-enable branch Dec 23, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.