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

Support Navigation and Edit Mode #16500

Merged
merged 29 commits into from Aug 2, 2019

Conversation

@youknowriad
Copy link
Contributor

commented Jul 10, 2019

closes #11581
This is #5709 take 2

The idea is:

  • using "tabs" navigate between blocks
  • hitting "enter" you enter the "edit" mode (the way it works today)
  • hitting "escape" to get back to navigation mode.

The original PR was abandoned because the keyboard event to enter navigation mode was not caught properly in all screen readers. This is fixed in this PR by using a focused button (the block name/breadcrumb) when in navigation mode.

The breadcrumb still needs to be designed properly (because it's now a button) but the idea is here.
I also removed the breadcrumb "animation" when you hover a block because the button was not showing right after you hit "escape".

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Hoooly guacamole, it's happening! Here's a quick GIF:

navigation mode

Initial keyboard usage impressions feel great. Fast, fluid, and it feels kind of intuitive too.

Would it be possible to allow the Delete button to delete the focused block? I would love to be able to, in any block, press Escape and then Delete to delete that block.

The focus style should probably also be rather different from both the hover and selected states. It seems like such a focus has more in common with either a multiselected block, or a focused textfield:

Screenshot 2019-07-10 at 13 58 12

Screenshot 2019-07-10 at 13 56 41

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

This is very cool! In my tests, it only appears to work if you start navigating only with the keyboard. Is that right? Or is there another way to activate it? Once I hit enter to edit a block, I can't figure out how to get back into navigation mode. 🤔

Would it be possible to allow the Delete button to delete the focused block? I would love to be able to, in any block, press Escape and then Delete to delete that block.

+1 to this. That would be awesome.

The focus style should probably also be rather different from both the hover and selected states. It seems like such a focus has more in common with either a multiselected block, or a focused textfield:

I wonder if something like this is more appropriate?

Frame 2

It's the same left border we use, but with a blue "active" state. I've also adjusted treatment of the breadcrumb so that it appears like a standard toolbar button.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@kjellr

This is very cool! In my tests, it only appears to work if you start navigating only with the keyboard. Is that right?

you can hit "Escape" when you're in edit mode to enable "navigation" mode.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Would it be possible to allow the Delete button to delete the focused block? I would love to be able to, in any block, press Escape and then Delete to delete that block.

This is done.

I also added the is-navigation className to be able to target that state by using .is-selected.is-navigation. I'd appreciate if you have some time to tweak the design accordingly. Designers eyes are the best :P

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

I also added the is-navigation className to be able to target that state by using .is-selected.is-navigation. I'd appreciate if you have some time to tweak the design accordingly. Designers eyes are the best :P

Great! I'll jump in and take a look later today.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

How will it be clear what mode you're in? Modes are generally pretty confusing.

hitting "enter" you enter the "edit" mode (the way it works today)

Enter ("return" on a lot of keyboards) traditionally creates a new line. Changing that seems a bit strange.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

I added some quick styles to match the comp above:

navigation-mode

@ellatrix

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

What if you can also use Tab to escape the mode? As soon as you cross the block boundary, you're back in block navigation mode.

Regarding Enter, one solution could be to also include the insertion points when tabbing through blocks. That way it's easy to navigate around blocks and insert between blocks. Sure it's twice as much to tab through, but way better than it is now.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

In a lot of situations Enter does something different. When you click a button for instance and I feel this is a very similar situation so I don't think in "navigation" mode "Enter" is expected to create a new line. In "edit" mode that's the case though.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Noting that the "Escape" key now conflicts with the previous expected behavior (show the toolbar). I think the new behavior is better but I thought I'd mention cc @aduth (It's breaking some e2e tests now)

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

It's the same left border we use, but with a blue "active" state. I've also adjusted treatment of the breadcrumb so that it appears like a standard toolbar button.

I wonder if we should just show the same blue all around? This is a focus state, so I feel like it should ambiguate more from the selected state than otherwise.

Here are some other applications that use what can best be described as a navigation mode. MacOS Finder:

finder

It even has the same enter/escape behavior.

Slack:

slack

Worth noting that in slack, you just use the arrowkeys to navigate, and tab to enter buttons of each message, so it isn't a 1:1 comparison. But the visual style distinctly feels "focusy".

@youknowriad youknowriad force-pushed the add/navigation-mode-take-2 branch 2 times, most recently from 736415f to 465a2a9 Jul 11, 2019

youknowriad and others added some commits Jul 24, 2019

Better param documentation
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Fix typo
Co-Authored-By: Ella van Durpe <wp@iseulde.com>
Fix typo
Co-Authored-By: Ella van Durpe <wp@iseulde.com>

@youknowriad youknowriad force-pushed the add/navigation-mode-take-2 branch from a36f9f4 to ad94bd7 Jul 26, 2019

// and the block is not locked.
const canUseShortcuts = (
isSelected &&
! isLocked &&

This comment has been minimized.

Copy link
@aduth

aduth Jul 26, 2019

Member

What's the reason we want to prevent switching modes when the template is locked? Isn't it still useful to be able to navigate content, even if the arrangement of said content can't be modified?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 29, 2019

Author Contributor

That boolean is not used for mode switching. So it should still be possible.


this.switchOnKeyDown = cond( [
[ matchesProperty( [ 'keyCode' ], ESCAPE ), this.focusSelection ],

This comment has been minimized.

Copy link
@aduth

aduth Jul 26, 2019

Member

This changes / regresses a behavior:

  1. Create paragraph
  2. Press Alt+F10
  3. Press Escape

With top toolbar:

Before: Escape placed caret back into paragraph
After: Nothing happens when Escape is pressed

Without top toolbar:

Before: Escape placed caret back into paragraph
After: Block navigation mode is triggered

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 29, 2019

Author Contributor

It was an intended change. We discussed this with some folks (@ellatrix and someone else I don't remember :P). The idea being that with this navigation mode, the "Escape" to move back to the block doesn't make sense anymore.

Now for the differences between "top" and "docked" toolbar. I'd argue that that's reasonable enough (even if not perfect).

  • Anything by the block triggers navigation mode,
  • Anything outside the block (inspector, top toolbar) doesn't.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

In the case of the toolbar attached to the block, you can just tab back to the place you were. Not sure about the top toolbar though. There tabbing moves to through everything in the editor. Esc returns to the block, but in navigation mode. Maybe that's fine?

}

// Special case when reaching the end of the blocks (navigate to the next tabbable outside of the writing flow)
if ( navigateDown && selectedBlockClientId && ! selectionAfterEndClientId && [ UP, DOWN ].indexOf( keyCode ) === -1 ) {

This comment has been minimized.

Copy link
@aduth

aduth Jul 26, 2019

Member

In truth, I don't really understand what's trying to be done here.

}

componentDidMount() {
window.addEventListener( 'mousedown', this.switchToEditMode );

This comment has been minimized.

Copy link
@aduth

aduth Jul 26, 2019

Member

Might be something we want to use withGlobalEvents, but I'm starting to question its usefulness, particularly with React hooks making this simple.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 29, 2019

Author Contributor

We could have a useGlobalEvent or something like that. I guess the benefit would be to only have a unique event handler?

packages/e2e-test-utils/src/keyboard-mode.js Outdated Show resolved Hide resolved
} from '@wordpress/e2e-test-utils';

describe( 'adding blocks', () => {
beforeEach( async () => {
await createNewPost();
await switchToEditMode();

This comment has been minimized.

Copy link
@aduth

aduth Jul 26, 2019

Member

Should we just make this a global thing in beforeEach ? Otherwise, when would we want this to linger between test cases? If the mode isn't persisted, how could it possibly be that we need to switch to edit mode after createNewPost ?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 29, 2019

Author Contributor

In a lot of situation we "hit" enter for example to create a block at the beginning of tests. This only works in "Edit" mode and by default we're in navigation mode.

Making it global: I thought about it. The thing is that sometimes you want to be in navigation mode as well. I guess edit is more common so it might mike sense to switch the default in tests.

youknowriad and others added some commits Jul 29, 2019

Simplify applied classname
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
Update packages/e2e-test-utils/src/keyboard-mode.js
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Can I have thoughts or a here?

@ellatrix

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I'll review it again in just a bit.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Thanks everyone for working on this, definitely better than version 1 🙂 I'd like this to be extensively tested by the accessibility team. Given the importance of this new interaction flow, I'd like to not be the only person giving accessibility feedback.

Will propose during tomorrow's weekly accessibility meeting. Of course, everyone is welcome to attend the meeting (Note: meetings happen on the WordPress Slack, registration is required).

@ellatrix
Copy link
Member

left a comment

Left some minor comments that could be nice to address, but other than that this looks great.
I can't give much accessibility feedback though.

// not already handled by descendant.
onInsertDefaultBlockAfter();
event.preventDefault();
if ( canUseShortcuts && isEditMode ) {

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 1, 2019

Member

Minor: these nested switch/if statements look a bit odd to me. Could they maybe be combined?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 2, 2019

Author Contributor

No easy way to do it. We could just use if elses?

}

componentDidMount() {
window.addEventListener( 'mousedown', this.switchToEditMode );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 1, 2019

Member

Why does it apply to the whole window? If you click inside a block it makes sense to switch to edit mode. If you click outside a block it makes more sense to stay in navigation mode?

@enriquesanchez

This comment has been minimized.

Copy link

commented Aug 1, 2019

Just tested this (Mac/Firefox 68.0.1) and works like a charm. This is a big improvement IMHO.

🤔 Thought: when in navigation mode, what if we added the ability to move to the sidebar with the right arrow? and then maybe back to the block with the left arrow?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

when in navigation mode, what if we added the ability to move to the sidebar with the right arrow? and then maybe back to the block with the left arrow?

Interesting idea. It would be good to find ways to improve the navigation to that panel. Right arrow to go there seems ok but left arrow to go back might cause issues depending what "control" is focused on the right sidebar. Imagine it's an input for instance.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Worth noting right and left arrow keys wouldn't work with screen readers. Also, when the block is in edit mode and the caret is within an editable field, arrow keys should keep their default behavior (moving through text).

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Ok let's try this, happy to address extra feedback.

@youknowriad youknowriad merged commit 2f757fa into master Aug 2, 2019

1 of 2 checks passed

Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the add/navigation-mode-take-2 branch Aug 2, 2019

@kjellr kjellr added this to the Gutenberg 6.3 milestone Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.