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

Add a header menu to switch between edit and select tool #18624

Merged
merged 11 commits into from Nov 28, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Nov 20, 2019

closes #17088
closes #17847
closes #18583

First attempt at making the select/edit tool more visible in the UI.

This PR does:

  • Adds a dropdown into the header to switch between select and edit tools.
  • Remove the explicit switch to the edit tool as soon as you click the canvas.
  • Enables the edit tool by default.
  • Enables clickthrough in select tool.

Capture d’écran 2019-11-20 à 10 20 11 AM

@youknowriad youknowriad self-assigned this Nov 20, 2019
@youknowriad youknowriad requested review from mtias and jasmussen Nov 20, 2019
@youknowriad youknowriad changed the title Add a header menu to switch between edit and select tools Add a header menu to switch between edit and select tool Nov 20, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 20, 2019

Can you include some visuals to make it easier to share feedback, please? :)

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 20, 2019

I couldn't resist to add it:

Screen Shot 2019-11-20 at 10 18 33

I have some feedback to share:

  • the label in the tooltip should follow the pattern and be shortened to Navigation tool
  • it doesn't behave like a menu, you can compare with the More Menu:
    • you can't use arrow-down key to open the menu
    • you can use arrow-up and arrow-down to switch between menu items
  • there is also one usability challenge, the shortcut for Select mode is esc, however when focus is in the dropdown menu – esc will close the menu, it might be confusing as some user might assume that this will change the mode, I don't have a solution for it
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 20, 2019

it doesn't behave like a menu, you can compare with the More Menu:

I actually used DropdownMenu first, but there's an inconsistency in styling between Dropdown + IcconButton and DropdownMenu that let me to fallback to Dropdown In addition to that, AFAIK we can add "help text" like the sentence at the end in DropdownMenu at the moment.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 20, 2019

Some more visuals for reference:
Screen Shot 2019-11-20 at 10 26 02
Screen Shot 2019-11-20 at 10 27 08
Screen Shot 2019-11-20 at 10 26 45

I don't think we are consistent at all so the part about the label would be nice to clarify in general, in addition we should decide whether we should include the heading in 🤷‍♂

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 20, 2019

It seems to work but would require some CSS tweaks:

diff --git a/packages/edit-post/src/components/header/more-menu/index.js b/packages/edit-post/src/components/header/more-menu/index.js
index 1f781b513..f03b8aa69 100644
--- a/packages/edit-post/src/components/header/more-menu/index.js
+++ b/packages/edit-post/src/components/header/more-menu/index.js
@@ -36,6 +36,9 @@ const MoreMenu = () => (
                                <MenuGroup>
                                        <OptionsMenuItem />
                                </MenuGroup>
+                               <div className="block-editor-navigation-mode-selector__help">
+                                       { __( 'Tools offer different block interactions to optimize block selection & editing tasks' ) }
+                               </div>
                        </>
                ) }
        </DropdownMenu>

It also would work with div inside MenuGroup.

Screen Shot 2019-11-20 at 10 40 04

It now makes me wonder whether we could extend the API of MenuGroup to support such help text.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 21, 2019

Updated this based on the feedback, it should be better now.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 22, 2019

Finally looking at this. Even as just a visualization of Navigation Mode, this feels like a step in the right direction. I would encourage anyone to test this PR out in practice.

Can you try this icon for the select tool instead?

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M6.5 1v21.5l6-6.5H21L6.5 1zm5.1 13l-3.1 3.4V5.9l7.8 8.1h-4.7z" /></svg>

Can you change the tooltip to just say "Tools"?

This also, in practice, feels like a great place to re-introduce the parent-first/clickthrough interface. I think I might either push some hover effect changes directly to this branch, or if you prefer, in a followup PR, to better help ambiguate the two modes and clarify the clickthrough effect better.

One thing worth doing, though — it's important that it's easy to return to "edit mode" trivially, even when using the mouse. Right now the only way to do that is to press Enter on a selected block, or selecting the edit tool. Can we add so this is also done when you click a second time on a selected block? I.e. this flow should work:

  1. Press Escape to enter navigation/selection mode
  2. Click a block to select it
  3. If the block you click is at the deepest nesting level, clicking it again should exit back to edit mode. That is, if you click a Paragraph twice inside a Cover, you go back to editing it.
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 22, 2019

I think I might either push some hover effect changes directly to this branch, or if you prefer, in a followup PR

I'm fine with both.

If the block you click is at the deepest nesting level, clicking it again should exit back to edit mode. That is, if you click a Paragraph twice inside a Cover, you go back to editing it.

I agree with this, I'll see how feasible it is.

@youknowriad youknowriad force-pushed the try/select-edit-tool branch from e276aae to faad95e Nov 25, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 25, 2019

If the block you click is at the deepest nesting level, clicking it again should exit back to edit mode. That is, if you click a Paragraph twice inside a Cover, you go back to editing it.

@jasmussen Turns out this is trickier than I originally thought because if the block has children blocks, it doesn't mean all its content is made of inner blocks, one big part of it might be edited in the block itself (example Media & Text or Cover blocks) which make me think this is probably not the best approach.

I do think it's weird that you're able to edit text... in navigation mode though, so it makes me think, maybe the fix here is not to make the "edit mode" more easy to trigger but the other way around, making it harder to trigger without an explicit button somewhere in the UI (or by changing the tool).

So basically, when you're in a leaf node an the navigation tool is enabled, you won't able to click the block to edit its content, unless you explicitely choose to go to the edit tool.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

I added a message for screen reader users using our existing a11y speak helper. Anything left here?

type: 'SET_NAVIGATION_MODE',
isNavigationMode,
};

if ( isNavigationMode ) {

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 27, 2019

Member

Are you sure that it should be announced every time someone switches modes? When editing individual blocks with the keyboard it might be a common pattern to switch between modes.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 27, 2019

Member

For example when I enable VoiceOver (⌘F5 in MacOS, works best in Safari), I see the following message when the browser has focus:

Screenshot 2019-11-26 at 13 53 56

I see it now. We can give it a try. Someone should battle test it though :)

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 27, 2019

Contributor

It is my understanding that yes, we do need something like this. We received helpful information from a user in #17088 (comment), who had also created #18583 which was related to the switch.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 27, 2019

Member

Right, my concern is mostly about the number of details included in the message. Anyway, let's collect feedback and refine only if necessary.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 27, 2019

there is also one usability challenge, the shortcut for Select mode is esc, however when focus is in the dropdown menu – esc will close the menu, it might be confusing as some user might assume that this will change the mode, I don't have a solution for it

This is still an issue. Actually, it's wrong for both tools as they use very popular keyboard shortcuts esc and enter. The question is whether we should show those keyboard shortcuts in the menu knowing that folks can't use them in this context as advertised?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

The question is whether we should show those keyboard shortcuts in the menu knowing that folks can't use them in this context as advertised?

That's a good question, we could potentially add new global shortcuts or remove the shortcuts from there now. What do you think @jasmussen

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

The question is whether we should show those keyboard shortcuts in the menu knowing that folks can't use them in this context as advertised?

Changing one or both of the Esc and Enter shortcuts is an option. Adding additional shortcuts is another. I would suggest that this could/should be explored and discussed separately, as this very pull request addressed an issue already shipping in master, and that it's worth getting that benefit in soon.

This is still an issue. Actually, it's wrong for both tools as they use very popular keyboard shortcuts esc and enter. The question is whether we should show those keyboard shortcuts in the menu knowing that folks can't use them in this context as advertised?

I'm unsure how to best address this, and even whether we should address it. It is a question of local vs. global shortcuts. Take ⌘B for "bold", for example, this is a local shortcut in that it only works when the cursor is in text. This is similar to the current navigatin mode behavior, as it requires a block to be selected first. And then there is the duality of Escape also meaning "close menu" or "clear selection" (when you've selected text).

So should we show the shortcut hint if it doesn't work when you're viewing the shortcut hint? I don't know... in the Keyboard Shortcuts sheet you can see the shortcut hint for bold even if it doesn't work there. But at least that sheet has clear context such as "Global Shortcuts", etc.

Question: can we make escape into a global shortcut? Can we make it both invoke navigation mode AND close the menu? If we can, should we?

Alternately, let's try this:

  • Add the new shortcuts to the Keyboad Shortcuts dialog, under "block shortcuts" or "selection shortcuts".
  • I think we should keep the menu hint, but revisit if it remains a problem.
  • If Esc and Enter remain challenging due to their existing actions, we should revisit those separately.

Sound good?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 27, 2019

This one is the most interesting when focus is on the selected "Select" mode:
Screen Shot 2019-11-27 at 10 57 20

Pressing Enter will only work as expected when you move the focus to the "Edit" item.

When switching to "Edit" mode with the keyboard, you rather don't want Esc to get activated to switch back to "Select" mode but rather the intention is to close the menu:
modes

I would personally remove those shortcuts from this menu and put them in the modal dialog which presents all keyboard shortcuts.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

I would personally remove those shortcuts from this menu and put them in the modal dialog which presents all keyboard shortcuts.

Another option emerges. Instead of writing:

Tools offer different block interactions to optimize block selection & editing tasks.

... we could remove the shortcuts hints from the right side, and instead write the following in the prose:

Tools offer different interactions to optimize block selection & editing tasks.

When editing, press <key>Escape</key> to select it with the Selection tool. Press <key>Enter</key> to go back to editing.

We'd want it also in the block keyboard shortcuts help sheet, but what do you think about the above?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

it doesn't feel great

Capture d’écran 2019-11-27 à 11 56 58 AM

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

Yeah that's a lot. I think it's still a good place. Maybe we can try this:

Tools offer different interactions for block selection & editing. To select, press Escape, to go back to editing, press Enter.

What do you think?

Edit: also removed the "key" element styling there, as it didn't seem helpful in the prose.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

Pushed it, feel free to tweak it as needed.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

I think it's good to try!

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

Approve :P

Copy link
Contributor

jasmussen left a comment

This PR in my opinion provides a solid accessibility improvement to navigation mode shipping in master. It provides a visual indicator, and better clarification on how to switch between the two. It also does not preclude us from making further improvements in the future!

Before you merge, it would be good to have @enriquesanchez just run through the announcement that happens to screenreaders when you switch modes. Other than that, 👍 👍 and thank you.

@gziolo
gziolo approved these changes Nov 27, 2019
Copy link
Member

gziolo left a comment

Nice work @youknowriad. It's a huge challenge to integrate those tools in the UI. I'm not surprised it triggered a lot of discussions. 😃

Code wise, everything looks good and all my comments were addressed 👍

Let's gather the initial feedback and refine the UI bits separately.

@youknowriad youknowriad force-pushed the try/select-edit-tool branch from 93ece0e to 8a255ce Nov 28, 2019
@youknowriad youknowriad force-pushed the try/select-edit-tool branch from 8a255ce to 27bee6b Nov 28, 2019
@youknowriad youknowriad merged commit 2bec0e6 into master Nov 28, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the try/select-edit-tool branch Nov 28, 2019
@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Nov 28, 2019

It's confusing to me that when I'm in selection mode I still see a text cursor (cursor: text) while hovering over paragraph blocks. Could we set cursor: default on non-selected blocks when in selection mode?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 28, 2019

@noisysocks That's a good point 👍

@enriquesanchez

This comment has been minimized.

Copy link
Contributor

enriquesanchez commented Nov 28, 2019

Hey @youknowriad!

I'm just catching up with my notifications and looks like this branch was already merged and deleted. I'm not sure if there's still a way for me to test this, let me know.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 28, 2019

@enriquesanchez It's in master, so you can try master directly and we can follow-up with any problems you find.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 29, 2019

When hovering a nested block in select mode, the left border disappears:

image

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 29, 2019

@ZebulanStanphill this seems like a small design glitch, would you mind creating an issue?

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 29, 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.