Adjust clashing shortcuts used for character input #14681

ellatrix · 2019-03-28T12:17:33Z

Description

Fixes #13118. Replaces the shift+alt combination with the "standard" WordPress access combination.

Also adds a check to KeyboardShortcuts to prevent this mistake from happening in the future.

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.

draganescu · 2019-03-28T14:18:03Z

This is great! It globally replaces Shift+Alt with Ctrl+Alt (on OSX). This is also the basic behaviour (Ctrl+Alt) that the HTML accesskey has:

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/accesskey

I tested and it worked as described, but I am not aware if this has other implications.

draganescu · 2019-03-29T09:20:52Z

@afercia @mcsf would you think is is actually a better solution than solving one individual shortcut? It also addresses @talldan 's suggestion to update all shortcuts. 'accesskey' seems a viable way to handle generic shortcuts that use one letter.

afercia · 2019-03-29T09:54:17Z

It has already been pointed out that the "standard" WordPress access combination is a problem, as it conflicts with native shortcuts used on macOS for VoiceOver. See for example:

Keyboard shortcuts that use the access modifier don't work with VoiceOver
#11154

Help shortcut conflicts with VoiceOver Help shortcut (Mac)
https://core.trac.wordpress.org/ticket/39271

So this is a known issue, and I'm not sure why we'd want to keep using access.

In #11154 I've also linked to a useful resource by the W3C: in the new WAI-ARIA Authoring Practices 1.1 there's a section dedicated to keyboard shortcuts. It's more about what not to do, see: https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_shortcuts_assigning

There's a list of combinations to avoid and, among others, it's clearly stated to avoid:

macOS only: Control+Option + any combination of other keys.

Also:

It globally replaces Shift+Alt with Ctrl+Alt (on OSX)

This actually means all these shortcuts are not available to VoiceOver users so I'm afraid I can't support this change as it creates an accessibility barrier.

ellatrix · 2019-03-29T10:07:23Z

@afercia I'm very well aware that access has its own problems, and that we should move away from that combination as well. For now, this is the quickest solution. The shift+alt combination needs to go away as soon as possible.

Which VoiceOver commands will clash with these two?

Other options:

ctrl+option+↑ and ctrl+option+↓
ctrl+option+tab and ctrl+option+shift+tab

afercia · 2019-03-29T11:03:14Z

@ellatrix thanks. Can you please clarify globally replaces Shift+Alt with Ctrl+Alt? At a first reading, I got it was going to use access everywhere. Looking at the code, seems to me it's changing only the navigate region shortcuts.

Anyways, here's a list of the VoiceOver commands, where VO stands for Control+Alt:
https://www.apple.com/voiceover/info/guide/_1131.html

Other options:

ctrl+option+↑ and ctrl+option+↓

ctrl+option+tab and ctrl+option+shift+tab

VO + Down and Up arrows are widely used for navigation and to change settings;
VO + Tab is used to "Tell VoiceOver to ignore the next key combination"

talldan · 2019-04-01T08:36:02Z

@ellatrix This looks good. There's one extra place to update the documented shortcut:
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/faq.md#editor-shortcuts

talldan · 2019-04-01T08:45:29Z

Looking at the code, seems to me it's changing only the navigate region shortcuts.

@afercia - I can help answer that. My understanding is that the two navigate regions shortcuts are the only ones bound in Gutenberg that are using shift+alt on mac os, which is where the problem is.

draganescu

Hi @ellatrix there is this doc file that should be updated once the shortcuts are changed,
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/faq.md#editor-shortcuts

Do you have the time to update it or may I edit your PR?

Otherwise with the follow-up confirmation from @talldan I think we're good to go!

talldan · 2019-04-11T09:50:53Z

@draganescu yep, I'm happy. It might be worth just pushing the changes to that file in a commit and then merging. I'm sure @ellatrix won't mind.

packages/components/src/keyboard-shortcuts/index.js

…duction dead code

draganescu · 2019-04-12T12:24:04Z

rebased, updated the docs and inlined the script based on @aduth suggestion

ellatrix · 2019-04-12T15:12:07Z

Thanks for the updates @draganescu!

* Adjust clashing shortcuts used for character input * Update modal * adds docs correction to correspond, adds inlined script to remove production dead code

aduth · 2019-04-25T12:48:35Z

packages/components/src/keyboard-shortcuts/index.js

 		forEach( this.props.shortcuts, ( callback, key ) => {
+			if ( process.env.NODE_ENV === 'development' ) {
+				const keys = key.split( '+' );
+				const modifiers = new Set( keys.filter( ( value ) => value.length > 1 ) );


Won't this capture much more than just modifiers, like escape, or space, or tab ? While ultimately it has no impact one way or the other since the subsequent lines only care to check for alt or shift specifically, it begs the question at that point why to filter at all (vs. hasAlt = keys.includes( 'alt' )).

aduth · 2019-04-25T12:53:07Z

packages/components/src/keyboard-shortcuts/index.js

+					( modifiers.size === 1 && hasAlt ) ||
+					( modifiers.size === 2 && hasAlt && hasShift )
+				) {
+					throw new Error( `Cannot bind ${ key }. Alt and Shift+Alt modifiers are reserved for character input.` );


I guess it's expected this should crash the application (to make the problem exceedingly obvious)? As there's been some prior conversation at #13405 and alternative approaches at #14151 to address these sorts of "wrong usage" violations, it seems we might consider having a unified approach to when and how they should be introduced. Personally I'd favor ESLint rules since they can be captured at build-time than at runtime, but I could grant that (a) there's more effort involved in creating the rules (though not always) and (b) this assumes that the developer has opted into using ESLint and the custom rules, which is not a given.

ellatrix requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners March 28, 2019 12:17

ellatrix added the [Type] Bug An existing feature does not function as intended label Mar 28, 2019

ellatrix added this to the 5.4 (Gutenberg) milestone Mar 28, 2019

ellatrix mentioned this pull request Mar 28, 2019

CZ quotation marks in Gutenberg don’t work #13118

Closed

afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Mar 29, 2019

noisysocks modified the milestones: 5.4 (Gutenberg), 5.5 (Gutenberg) Apr 1, 2019

draganescu approved these changes Apr 11, 2019

View reviewed changes

aduth reviewed Apr 11, 2019

View reviewed changes

packages/components/src/keyboard-shortcuts/index.js Outdated Show resolved Hide resolved

ellatrix requested review from mkaz, oandregal and notnownikki as code owners April 12, 2019 12:22

ellatrix and others added 3 commits April 12, 2019 15:23

Adjust clashing shortcuts used for character input

97c8a84

Update modal

cd58e41

adds docs correction to correspond, adds inlined script to remove pro…

92aefd3

…duction dead code

draganescu force-pushed the fix/input-shortcuts branch from 2d1a65f to 92aefd3 Compare April 12, 2019 12:23

ellatrix merged commit dd81bcc into master Apr 12, 2019

ellatrix deleted the fix/input-shortcuts branch April 12, 2019 15:12

talldan mentioned this pull request Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

aduth mentioned this pull request Apr 15, 2019

Plugin: Backport bug fixes from Gutenberg 5.5.0 to wp/trunk (WordPress 5.2 RC) #14987

Merged

aduth pushed a commit that referenced this pull request Apr 16, 2019

Adjust clashing shortcuts used for character input (#14681)

709d68c

* Adjust clashing shortcuts used for character input * Update modal * adds docs correction to correspond, adds inlined script to remove production dead code

aduth pushed a commit that referenced this pull request Apr 16, 2019

Adjust clashing shortcuts used for character input (#14681)

f11681c

* Adjust clashing shortcuts used for character input * Update modal * adds docs correction to correspond, adds inlined script to remove production dead code

This was referenced Apr 17, 2019

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

draganescu mentioned this pull request Apr 18, 2019

removes the dev mode access key binding safeguard #15044

Merged

aduth reviewed Apr 25, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust clashing shortcuts used for character input #14681

Adjust clashing shortcuts used for character input #14681

ellatrix commented Mar 28, 2019 •

edited

Loading

draganescu commented Mar 28, 2019

draganescu commented Mar 29, 2019

afercia commented Mar 29, 2019

ellatrix commented Mar 29, 2019

afercia commented Mar 29, 2019

talldan commented Apr 1, 2019

talldan commented Apr 1, 2019

draganescu left a comment

talldan commented Apr 11, 2019

draganescu commented Apr 12, 2019

ellatrix commented Apr 12, 2019

aduth Apr 25, 2019

aduth Apr 25, 2019

Adjust clashing shortcuts used for character input #14681

Adjust clashing shortcuts used for character input #14681

Conversation

ellatrix commented Mar 28, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

draganescu commented Mar 28, 2019

draganescu commented Mar 29, 2019

afercia commented Mar 29, 2019

ellatrix commented Mar 29, 2019

afercia commented Mar 29, 2019

talldan commented Apr 1, 2019

talldan commented Apr 1, 2019

draganescu left a comment

Choose a reason for hiding this comment

talldan commented Apr 11, 2019

draganescu commented Apr 12, 2019

ellatrix commented Apr 12, 2019

aduth Apr 25, 2019

Choose a reason for hiding this comment

aduth Apr 25, 2019

Choose a reason for hiding this comment

ellatrix commented Mar 28, 2019 •

edited

Loading