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

Change keyboard shortcut for remove block to Cmd+Shift+X / Ctrl+Shift+X #9190

Merged
merged 5 commits into from
Aug 28, 2018

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 21, 2018

Description

Closes #9036

  • Change the remove block shortcut to Cmd+Shift+X (Mac) or Ctrl+Shift+X (Others)

How has this been tested?

  • Tested manually on Mac OS - Chrome, Firefox, Safari, Opera (using a British keyboard layout)
  • Tested manually on Windows - Chrome, Firefox, Safari, IE11, Edge (again, British keyboard layout)

Screenshots

Block Settings Menu
screen shot 2018-08-27 at 6 11 05 pm

Keyboard Shortcut Help
screen shot 2018-08-27 at 6 11 22 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan added the [Type] Bug An existing feature does not function as intended label Aug 21, 2018
@talldan talldan added this to the 3.7 milestone Aug 21, 2018
@talldan talldan self-assigned this Aug 21, 2018
@talldan talldan requested a review from a team August 21, 2018 10:10
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My immediate thought is this is a less nice shortcut for the majority of desktop users (Windows + Mac). I'd almost like it to be Linux-only but I get that's not the tradition in Gutenberg/WordPress. It's also a lot of extra code. 😄

So it works for me!

@@ -15,6 +15,10 @@ class KeyboardShortcuts extends Component {
super( ...arguments );

this.bindKeyTarget = this.bindKeyTarget.bind( this );

// Firefox uses keycode 173 for '-', while other browsers use 189.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for the note.

Copy link
Contributor

@aldavigdis aldavigdis Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may have spotted an i18n issue here, using Icelandic keyboard layout on MacOS. Both [ö] and [-] results in keycode 173 in Firefox and 189 in Chrome.

Perhaps sticking with alphabetical keys is a good idea?

@talldan
Copy link
Contributor Author

talldan commented Aug 21, 2018

❤️ Thanks for the speedy review! Would be great to get a test on Linux if anyone has one available.

@jasmussen
Copy link
Contributor

One followup now that we are touching keyboard shortcuts. I noticed a pattern on the Mac for keyboard shortcuts, that we might want to adopt. Here's Google Chrome on the Mac:

chrome_mac

Note how instead of Alt, they write , instead of Shift it's . There are also no plusses between the key indicators. Can we do the same? If you actually glance down at a Mac keyboard, those same icons are used on the keys, in the case of Shift it doesn't even actually have the word "Shift" on the keyboard. I think it's a far more Mac like way of doing things and I think we should adopt it.

Note that this would be a Mac only thing, on Windows we'd still have to spell out a lot of the keys — Chrome does this as well:

chrome_windows

@tofumatt
Copy link
Member

@jasmussen I tried that approach awhile back, but we actually favour using the keyboard names over the Mac symbols unless it's Command. See the discussion here: #4411 (review)

@jasmussen
Copy link
Contributor

Could that discussion be revisited? Thanks for the link but I couldn't exactly extract from the history of that PR exactly why it was preferable to do the other.

Revisiting this now that keyboard shortcuts are in such a great state in master, it feels like we could go the extra mile and tailor Gutenberg more to the operating system itself. This is the stock Finder:

screen shot 2018-08-21 at 15 08 34

Note how they even go further and use ^ to imply Ctrl, which frankly I'm totally fine with. From a certain point of view this feels less like a "what we think is best" and more of a "let's match the operating system" discussion.

@tofumatt
Copy link
Member

The issue is that plenty of keyboards don't actually include the symbol–@iseulde mentioned her keyboard doesn't have those symbols and it was somewhat confusing to see them. My US Logitech keyboard doesn't ship with the symbols either:
img_1522

And looking at @sarahmonster's British Apple Keyboard (Magic Keyboard 2, so the newest one they make), it doesn't have the symbols:
img_1523

Using those symbols saves space (though only for Mac users, so we can't rely on that space-saving cross-platform) but it's certainly less clear. Sure the symbols are more idiomatic, but I think they're less usable and it's an idiom I'm fine eschewing.

@ZebulanStanphill
Copy link
Member

While the macOS key symbols are not used on all keyboards, they are used throughout the OS (like in Finder, as @jasmussen pointed out), so I think using the symbols on macOS is fine, since users would already have been exposed to the symbols in the context menus of the OS, as well as common apps like Chrome, and they do make the editor fit in a bit more with the operating system and save some space.

On a more related-to-the-PR note, thank you, @talldan! I am definitely looking forward to being able to use the Remove Block shortcut on my computer.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 21, 2018

I tested this branch, and it is working for me in both Firefox and Chromium on Antergos Linux. 👍

@tofumatt
Copy link
Member

I think this is good to merge as-is; we can have a separate discussion about keyboard symbols if @jasmussen wants to open one… though as mentioned I believe he is, like me, well-meaning but in this particular instance: ultimately wrong 😆

@jasmussen
Copy link
Contributor

Hah! You're the one to claim to know better than Apple what Mac users want. 🔥

So I disagree with you, but in a friendly loving way. I also don't want to block improvements like this, but I still think the discussion is worth having.

The devil is in the details, and if Mac users see one keyboard combo in their OS and browser, and another in their blogging software, I'm not sure we're benefiting anyone by being "better".

@mtias
Copy link
Member

mtias commented Aug 21, 2018

Consistency with the OS is what tips the balance for me.

@tofumatt
Copy link
Member

Well, I WANT to agree with you, and your arguments have convinced me. 👍🏻

But yeah, a separate issue for that would be great. You can assign it to me as penance if you like 😅

@aldavigdis
Copy link
Contributor

aldavigdis commented Aug 22, 2018

i18n issue:

Using e.key for non-alphanumeric keys results in the keyboard shortcut belonging to different physical keys; depending on they keymap in use.

For example, - is located in the bottom row in the Danish and German keymaps, while it is in the top row in English and Icelandic keymaps.

Finally, this may be minor and a bug in MacOS, but pressing Ö and - using the Icelandic keymap in MacOS results in the same key code (189 in WebKit, 173 in FF).

This could cause some issues with users switching keyboard layouts frequently or others supporting people with different layout then they are used to themselves. How about using a an alphabetical key like X, which is generally located on the same physical key regardless of keymap?

@talldan
Copy link
Contributor Author

talldan commented Aug 22, 2018

@aldavigdis - thanks for letting us know about that. That's very unusual!

Is there a use case for Ctrl+Shift+Ö? if not, I think it's ok to merge this and have both Ctrl+Shift+Ö and Ctrl+Shift+- do the same thing.

We're using mousetrap for mouse events, which uses event.keyCode or event.which (whichever is defined):
https://github.com/ccampbell/mousetrap/blob/78bae4e98226b64d3c7ba7b9956f63e4f8c965ec/mousetrap.js#L214-L220

So this shouldn't cause an issue when using different mappings. For example, on the Italian keyboard, '-' is next to the right shift, but the browser still sends through 189, despite it being a forward slash on my British keyboard:

KeyboardEvent {
  code: 'Slash',
  keyCode: 189,
  key: '-',
  // ...
}

@aldavigdis
Copy link
Contributor

aldavigdis commented Aug 22, 2018

What I'm emphasising is I think we should try to use alphanumeric buttons as much as possible instead of symbols because they are generally consistent across keyboard layouts.

@tofumatt
Copy link
Member

tofumatt commented Aug 22, 2018 via email

@aldavigdis
Copy link
Contributor

aldavigdis commented Aug 22, 2018

@talldan As someone who's a heavy user of keyboard shortcuts, I tend to switch to the en_US layout when working with certain software as some keys like { don't even exist on many non-English layouts.

Word processing or something like WordPress that runs in the browser does not fall into the category of software where I may be expected to switch layouts.

There are generally consistency issues bound to using non-alphanumeric keyboard shortcuts as well as they are often either bound to the physical key, so - and + are located right next to each other as in the US keymap — or if bound to the character, all over the place — or perhaps nowhere at all.

@aldavigdis
Copy link
Contributor

aldavigdis commented Aug 22, 2018

@tofumatt even across QWERTY, QWERTZ and AZERTY, a normal user can generally find an alphanumeric key naturally — and X is usually located on the same spot consistently across layouts.

@talldan
Copy link
Contributor Author

talldan commented Aug 23, 2018

I'm definitely up for defining some rules about keyboard shortcuts as discussed in the meeting last night. Off the top of my head, and in a general order of significance:

  • Don't use a combination that results in an outputted character (e.g. in a paragraph block)
  • Don't clash with OS level shortcuts
  • Don't use a shortcut reserved for future functionality
  • Prefer not to clash with browser shortcuts (especially commonly used)
  • Prefer shortcuts available for a variety of keyboard layouts
  • Prefer shortcuts that have similar behaviour elsewhere in WordPress (e.g. classic editor)
  • Prefer shortcuts that have similar behaviour in other applications
  • Prefer shortcuts with fewer modifier keys
  • Prefer a shortcut that makes some sort of semantic sense.

I think the trouble is that there aren't actually many shortcuts freely available after going through all of those rules.

I'm definitely open to changing it if there's a good suggestion.

@afercia afercia changed the title Change keyboard shortcut for remove block to crl+shift+- Change keyboard shortcut for remove block to ctrl+shift+- Aug 26, 2018
@afercia
Copy link
Contributor

afercia commented Aug 26, 2018

What I'm emphasising is I think we should try to use alphanumeric buttons as much as possible instead of symbols because they are generally consistent across keyboard layouts.

I'd strongly recommend to take @aldavigdis recommendation into account. For the records, it was already noted in #8316 (comment)

Also, there's a problem in the way some non-alphanumeric keys are announced by screen readers, as they usually don't read out "punctuation" like , or ; or -. Will open a separate issue.

@ZebulanStanphill
Copy link
Member

If Ctrl+Shift+- can not be used due to accessibility issues, I would be completely happy with something like Ctrl+Shift+X. I mean, anything is better than having a keyboard shortcut that kills your GUI session. 😛

I also think the ability to remap keyboard shortcuts (#3218) would be a great addition to cover more obscure conflicts and let users choose shortcuts that work better for them, but would not work for other users.

@talldan
Copy link
Contributor Author

talldan commented Aug 27, 2018

Cmd+Shift+X / Ctrl+Shift+X does seem like a reasonable option. I couldn't spot any particular issues with it. Any objections to that?

@jasmussen
Copy link
Contributor

Go for it. Better to ship fast and iterate than hold off and maybe not ship ❤️

@talldan
Copy link
Contributor Author

talldan commented Aug 27, 2018

Ok - changed now. I'll do some testing tomorrow and maybe we can merge then.

@ZebulanStanphill
Copy link
Member

Nice! This PR should probably be renamed.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested whether the shortcuts conflict with anything, but the code still looks fresh and groovy 👍

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just be sure to update the newly-minted keyboard shortcuts section in the reference doc 😄

@talldan talldan force-pushed the update/remove-block-shortcut branch from 1e6f3f7 to 486ac46 Compare August 28, 2018 04:43
@talldan talldan changed the title Change keyboard shortcut for remove block to ctrl+shift+- Change keyboard shortcut for remove block to Cmd+Shift+X / Ctrl+Shift+X Aug 28, 2018
@talldan
Copy link
Contributor Author

talldan commented Aug 28, 2018

Cool - I just did some testing. All worked well. Renamed the ticket and pushed a commit that updates the faqs.

Thanks for the shortcut suggestion @ZebulanStanphill 🎉

@talldan talldan merged commit 32b1f92 into master Aug 28, 2018
@talldan talldan deleted the update/remove-block-shortcut branch August 28, 2018 05:02
@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 28, 2018

@talldan Actually, it was @aldavigdis' idea to use X:

How about using a an alphabetical key like X, which is generally located on the same physical key regardless of keymap?

Also... I forgot to actually test the keyboard shortcut. Fortunately, I just did, and it does not conflict with my browser or my OS.

Instead, it conflicts with WordPress. This is what that keyboard shortcut does:
image

Oops.

@ZebulanStanphill
Copy link
Member

Actually, nevermind. It is not a WordPress behavior. I think it might be a Firefox one. Let me check what happens in Chromium.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 28, 2018

Yeah, it is a Firefox behavior. Ctrl+Shift+X toggles RTL mode in Firefox.

@talldan
Copy link
Contributor Author

talldan commented Aug 28, 2018

Oh no! Wonder why I couldn't reproduce it before. Perhaps because I was only testing with a block selected.

edit- I must've also been testing on a site that doesn't support rtl, so was seeing no effect.

It doesn't help that this is an undocumented shortcut either.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 28, 2018

@talldan How about trying Ctrl+Shift+Alt+X? That does not appear to do anything for me in Firefox on Linux.

@talldan
Copy link
Contributor Author

talldan commented Aug 28, 2018

Probably a good time to share this spreadsheet I've been working on so that we don't have to have these conversations:
https://docs.google.com/spreadsheets/d/1nK1frKawxV7aboWOJbbslbIqBGoLY7gqKvfwqENj2yE/edit?usp=sharing

I plan to try and move this out of google docs form and on to github at some point soon.

talldan added a commit that referenced this pull request Aug 28, 2018
@talldan
Copy link
Contributor Author

talldan commented Aug 28, 2018

Ok - I'll revert the merge since the behaviour is not desirable (e.g. if a user forgets to select a block, they're left with rtl text direction)

How about trying Ctrl+Shift+Alt+X? That does not appear to do anything for me in Firefox on Linux.

My feeling is that three modifiers makes it so complex it's not worth having. There are other alternatives in the spreadsheet, but they're not very nice semantically.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 28, 2018

Personally, I would not mind Ctrl+Shift+Alt+X since all the keys are pretty close to each other (on US keyboards, anyway), and it at least does not cause a display server restart.

Looking at your spreadsheet, it looks like Ctrl+Alt+X is also available. Perhaps that would be a better choice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Block keyboard shortcut causes display server on Linux to shut down or reboot
8 participants