Add new keyboard shortcuts for block settings menu #8279

talldan · 2018-07-30T08:02:33Z

Description

Related issues - #7433, #71, #2980

Adds some new keyboard shortcuts for the block settings menu:

Cmd+Shift+D / Ctrl+Shift+D - duplicate block
Cmd+shift+, / Ctrl+shift+, - show/hide sidebar

In order to achieve this a number of changes were made:

MenuItems are now used to render block settings buttons (since they display shortcuts in the intended way)
Improved some incorrect aria attributes used on MenuItem. MenuItem now defaults to a role of menuitem and uses aria-checked instead of aria-pressed as per example implementation (https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-2/menubar-2.html)
Move onDuplicate and onRemove action dispatchers from their components to a HOC (withBlockSettingsActions) so that they can be shared by a keyboard shortcut component as well as the buttons themselves.
Remove BlockRemoveButton and BlockDuplicateButton, since MenuItems can now be used directly
Add BlockSettingsKeyboardShortcuts component for binding of shortcuts
Alter the way BlockSettingsMenu is rendered so that keyboard shortcuts are available when typing.
Some changes to RichText keyboard event handling. The component was stopping propagation of some keyboard events unnecessarily.

How has this been tested?

Tested Chrome, Firefox, Safari, Opera on MacOS. Only known keyboard shortcut conflict is Cmd+Shift+D which is used for 'Bookmark all open tabs' in Chrome and Firefox. This is prevented when a block is focused.
Tested Chrome, Firefox, Opera, Edge, IE 11 on Windows 10. Ctrl+Shift+D again conflicts with 'bookmark all tabs' and is prevented when a block is focused.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

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

jasmussen · 2018-07-30T08:58:09Z

Very very cool work! Love to see the shortcuts there. A few quick ones:

Let's do ⌘, instead of ⌘+, — it's a pattern to not show a plus after the ⌘ symbol for shortcuts. However we do probably want, on Windows, for it to say Ctrl+,.
Let's do Del instead of DEL. Should there be a plus? I'm leaning towards no. @karmatosed any thoughts?

Testing the PR the keyboard shortcuts are working well. However ⌘, works anywhere, not just with the block focused — is this desired? @afercia I seem to remember the original purpose of this shortcut was to open the sidebar and set focus there, like a "Skip" link. Do we not need this anymore, and if not, should we remove the block menu item entirely?

If we want ⌘, to work everywhere, we might want to show it as a tooltip on the cog too, like we show ⌘S on Save:

afercia · 2018-07-30T13:49:03Z

It's important to consider that keyboard shortcuts shouldn't ever conflict with native shortcuts. For example, Cmd+d on mac Chrome is used to bookmark a page. Though it's unlikely that users want to bookmark a page while a block is selected, conflicts should be avoided. I'm pretty sure the equivalent shortcuts for Windows conflict with other native functionalities. Have the new shortcuts been tested also on Windows? With a few exceptions, I guess we should always use two modifiers for the shortcuts.

Ideally, Cmd+, should work only when there's a block selected otherwise the sidebar will show "No block selected" when it appears.

I seem to remember the original purpose of this shortcut was to open the sidebar and set focus there, like a "Skip" link. Do we not need this anymore, and if not, should we remove the block menu item entirely?

I'd prefer to not assume users want to move focus on the sidebar. I think having the ability to toggle the sidebar from the block is a good thing, but the ability to "jump" to the sidebar should be offered by other means.

abrightclearweb · 2018-07-30T14:11:37Z

Just to agree with @afercia about Cmd+d; that would be pretty annoying for Chrome users to see bookmarks created unnecessarily.

Cmd+ triggers the font size increase so I'm not sure that will work as a shortcut.

The only one that works on Windows and Mac as expected isCmd+del/ CTRL+Del.

talldan · 2018-07-31T03:30:21Z

Overriding browser behaviour is not desirable and unfortunately we also can't use key combinations that result in a printed character. This rules out a lot of simpler options.

Delete Block - I'm fairly happy with Cmd+Del / Ctrl+Del. Note that when a user is not typing in a block but has block(s) selected, the user can also use the delete key on its own to delete blocks. I personally feel that it's ok to make both options available, it feels fairly intuitive.

Duplicate Block - Cmd+d does indeed result in a bookmark when no block is selected, but preventDefault is used to prevent that while a block is selected. Avoiding this key combination might be a good idea, since it is used the same way in all browsers. Shift+Cmd+d / Shift+Ctrl+d could be an alternative (in a few text editors this is the shortcut for duplicating a line), but in Chrome (and Firefox) that's also 'Bookmark all tabs'. Is that obscure enough that we'd consider overriding it?

Toggle sidebar - I felt that being able to use the shortcut while no blocks are selected is actually preferable. The sidebar is still present at that point, so why not? One thing I'd change on second glance is that when no blocks are selected, the 'Document' tab should be the selected tab when opening the sidebar using the shortcut.

jasmussen · 2018-07-31T06:34:11Z

Ideally, Cmd+, should work only when there's a block selected otherwise the sidebar will show "No block selected" when it appears.

This is not actually the case in this branch. Since the auto-tab-selection, if you have no blocks selected and invoke the shortcut, you'll simply see the sidebar.

Toggle sidebar - I felt that being able to use the shortcut while no blocks are selected is actually preferable. The sidebar is still present at that point, so why not? One thing I'd change on second glance is that when no blocks are selected, the 'Document' tab should be the selected tab when opening the sidebar using the shortcut.

Not completely sure either way. I agree that in practice this seems to work very nice — and the more shortcuts, the better. Additionally, I agree it would feel weird that a shortcut for a global interface is available only when a block is selected. Though if we keep this shortcut global, I would suggest we add a tooltip also to the button in the editor bar, as noted in #8279 (comment).

The alternative viewpoint is that because the block more menu has an item called "Show Block Settings", the shortcut key for meta+, is shown in context of that specific menu item, and suggests that it is a shortcut to showing the particular block settings tab, not just the sidebar overall as is the case right now. If we were to change the behavior to show that tab, not just toggle the sidebar, we'd probably want an additional shortcut to toggle the sidebar overall, right?

I think that so long as we have the interface where the block settings tab is automatically chosen when you select a block (and the sidebar is open), it makes the most sense to have this shortcut be for toggling the sidebar, not the tab. Essentially, because the tab is not behaving as a tab, but more like a breadcrumb.

talldan · 2018-08-02T10:04:15Z

@jasmussen I've changed the shortcut for duplicate to Cmd+Shift+D.

For the sidebar toggle, I've made the change I've suggested, so that the appropriate tab is opened depending on whether a block is selected.

I also realised Cmd+, clashes with Chrome's open settings shortcut, so I've changed that to Cmd+Shift+,

I'll try to have a thorough test across platforms of the shortcuts tomorrow.

jasmussen · 2018-08-02T10:49:24Z

Love it. This is working great for me! I have nothing more to add here, unless the others on the thread have objections I think this is good for final code review. Nice work!

noisysocks

Love this—great job! 😍

Not so sure about Cmd+Del as the shortcut for deleting a block. It doesn't work for paragraph blocks because Cmd+Del deletes the current line of text in macOS. In most of our non-paragraph blocks, Del already deletes the block when it is selected.
Selecting multiple blocks and then pressing Cmd+Del doesn't seem to work.
Unrelated, but should we just kill Shift+Opt+Cmd+M? It's such an awkward shortcut and I'm not sure that switching to the code editor is a very common or useful function.

noisysocks · 2018-08-03T01:55:40Z

edit-post/components/keyboard-shortcuts/index.js

+		hasBlockSelection: !! select( 'core/editor' ).getBlockSelectionStart(),
+	} ) ),
+	withDispatch( ( dispatch, { hasBlockSelection } ) => ( {
+		switchMode: ( mode ) => {


Minor: Could shorten this to switchMode( mode ) { which has the nice side effect of being a named function (useful for debugging).

You didn't do this 👀

Whoops - now done.

noisysocks · 2018-08-03T01:58:10Z

edit-post/components/keyboard-shortcuts/index.js

+		switchMode: ( mode ) => {
+			dispatch( 'core/edit-post' ).switchEditorMode( mode );
+		},
+		openSidebar: () => {


Minor: Could be openSidebar() {, as above

noisysocks · 2018-08-03T02:02:10Z

edit-post/components/keyboard-shortcuts/index.js

 	}

 	toggleMode() {
 		const { mode, switchMode } = this.props;
 		switchMode( mode === 'visual' ? 'text' : 'visual' );
 	}

+	toggleSidebar( event ) {
+		event.preventDefault();


What does this do?

Added comments to explain it. The only shortcut I know that clashes is the one for duplicate, which overrides 'bookmark all open tabs' in chrome. Also, with the devtools open it repositions the devtools pane (Google override their own shortcut). It's not ideal to override a browser shortcut, but the options for a shortcut here are very limited.

Cmd+D is add bookmark
Cmd+Opt+D is show/hide dock

Other uses of preventDefault don't prevent any known shortcuts, but the user might be using an obscure browser/ plugin/ some kind of custom shortcut, and generally we don't want two things to happen when a user uses a shortcut.

noisysocks · 2018-08-03T02:08:26Z

packages/components/src/menu-item/style.scss

 		fill: $dark-gray-500;
 	}

 	&:hover svg,
 	&:hover svg * {
-		stroke: $dark-gray-900 !important;
-		fill: $dark-gray-900 !important;
+		fill: $dark-gray-900;


These !importants were actually intentional.

Try installing https://wordpress.org/plugins/dropit/, you'll note that it adds plugin icons up there that are colored. The only way we can reliably recolorize those on mouse hover and in particular on active, is by using !important. This is unfortunate, but it is also exactly the kind of edgecase that I believe the modifier was designed for.

Perhaps we should add a comment?

This is how it should look:

This is how it will look if we remove the !important's:

Note that the toggled state still works in the Drop It plugin even if the !important's are removed, but this is because Drop it is built by our own Riad so he was careful to make that work. In other words, he added his own important styles in his plugin. But we can't rely on plugin developers to do this, and given SVGs devs will use for plugin icons commonly rely on inline styles, I see no other way.

Perhaps we should add a comment?

👍

The dropit icons are icon-buttons, whereas this style rule is for menu-items.

I guess there could be similar reasoning for menu-items, so I've added the !important back in for the fill style and added a comment.

I can't add the stroke style, as it makes our dashicons look fuzzy:

That's a great catch. This needs to be scoped better. The only reason those rules exist for the dropdown menu is so as to catch these plugin-added icons:

There doesn't appear to be any specific rule we can target, alas:

Should we make one?

Alternately, perhaps it's okay to remove the !important rules after all, since it's less important that the hover and active styles in the more menu are colorized than it is in the toolbar.

Ah - thanks, I didn't spot them there in menu-item form as well.

I can create a separate issue - it does look like a specific way to target the plugins would be the best trade-off.

noisysocks · 2018-08-03T02:09:32Z

packages/editor/src/components/block-list/block.js

@@ -499,7 +499,7 @@ export class BlockListBlock extends Component {
 					<BlockSettingsMenu
 						clientIds={ clientId }
 						rootClientId={ rootClientId }
-						isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' }
+						isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' || isTypingWithinBlock }


noisysocks · 2018-08-03T02:22:48Z

packages/editor/src/components/block-settings-menu/with-block-settings-actions.js

-)( BlockDuplicateButton );
+] );
+
+export default withBlockSettingsActions;


Can you remind me why we're putting this in its own file, again? Was it just to make block-settings-menu/index.js less large?

I'm wondering if it's unnecessary indirection. withBlockSettingsActions and BlockSettingsMenu are very tightly coupled so it makes sense in my mind that their code remains very near each other.

Yeah, totally agree. Initially, I tried out using withBlockSettingsActions as a HOC for both the BlockSettingsMenu and BlockSettingsKeyboardShortcuts (to solve the issue where shortcuts weren't being bound until the menu was rendered). I ended up changing the condition around when BlockSettingsMenu is rendered to solve that problem.

I agree now that it's only used to wrap one component they can be combined.

Done in 8db7543.

Have also gone one step further and refactored BlockSettingsKeyboardShortcuts into BlockSettingsMenu in f847517, as it wasn't doing anything.

noisysocks · 2018-08-03T02:29:09Z

packages/editor/src/components/block-settings-menu/with-block-settings-actions.js

 			index: getBlockIndex( last( castArray( clientIds ) ), rootClientId ),
 			isLocked: !! getTemplateLock( rootClientId ),
+			blocks,
+			canDuplicate,
+			shortcuts,


Why have withSelect pass shortcuts through as a prop here? Would it make sense for shortcuts to just be a regular top-level variable, since it never changes when the store updates?

Now used as just a regular variable since 8db7543

talldan · 2018-08-03T10:04:21Z

Thanks for the review @noisysocks :)

It doesn't work for paragraph blocks because Cmd+Del deletes the current line of text in macOS.

Hmm, I don't seem to have that behaviour on my Mac OS - Cmd+Del in a paragraph doesn't do anything on master.

Selecting multiple blocks and then pressing Cmd+Del doesn't seem to work.

This also works fine for me. I wonder what's different about your particular setup.

abrightclearweb · 2018-08-03T10:30:50Z

@noisysocks

Unrelated, but should we just kill Shift+Opt+Cmd+M? It's such an awkward shortcut and I'm not sure that switching to the code editor is a very common or useful function.

I'd be reluctant to lose a shortcut for switching between visual and code. Not saying it has to be that key combo but I understand that some users with a visual impairment write posts in HTML. It would be bad if they were denied that choice because the shortcut didn't exist.

afercia · 2018-08-03T12:36:11Z

Maybe Shift+Opt+Cmd+M could be simplified but it's an essential shortcut for pro keyboard users and assistive technologies users.

noisysocks · 2018-08-06T01:08:33Z

@abrightclearweb @afercia: Makes sense, thanks for explaining the motivation behind that shortcut! 🙂

I wonder what's different about your particular setup.

We might be getting confused about what the "Delete" key is. It's my fault. I'm referring to this key, which normal people call "Backspace":

This explains the behaviour I saw, though. I was pressing Cmd+"Backspace" to delete the block because that's normally what the shortcut for deletion is on Mac. Most Mac users don't have a dedicated "Delete" key unless they're using an external keyboard.

I think our best bet is to just cover our bases and allow both primary( 'del' ) and primary( 'backspace' ) to delete the block.

talldan · 2018-08-06T02:38:07Z

Oh yes, that does make sense. I should've thought of that. I'll bind backspace as well. I supposed we're not too concerned about losing the ability to delete an entire line? It doesn't seem particularly useful outside of coding.

noisysocks · 2018-08-06T03:55:01Z

I supposed we're not too concerned about losing the ability to delete an entire line? It doesn't seem particularly useful outside of coding.

Ah. I use Cmd+Backspace all the time when writing 😛

OK, how about we have:

When editing text: Cmd+Opt+Backspace & Cmd+Opt+Del deletes the selected block.
When not editing text: Backspace & Del deletes the selected block. (This shortcut already exists in master.)

jasmussen · 2018-08-06T07:18:34Z

Ah. I use Cmd+Backspace all the time when writing 😛

+1+1 we should be very careful with any conflicting shortcuts.

jasmussen · 2018-08-06T07:21:07Z

Also worth noting, this is how my keyboard looks, with separate backspace and del keys:

So it's a rabbit hole. Reuse existing shortcuts when we can, and be careful when inventing new ones, that seems like the best practice.

talldan · 2018-08-06T09:08:13Z

I've just spotted a new issue. The cmd+del or cmd+shift+del binding isn't triggered in a paragraph block when at the end of a section of text (ie. when there's nothing to delete). The same happens with backspace at the start of a section of text.

Took some time to figure out what it could be. Seems to be this code that's the cause, in particular, line 462:

gutenberg/packages/editor/src/components/rich-text/index.js

Lines 439 to 463 in fb03f0b

    
           if ( 
        
           	( keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) || 
        
           	( keyCode === DELETE && isHorizontalEdge( rootNode, false ) ) 
        
           ) { 
        
           	if ( ! this.props.onMerge && ! this.props.onRemove ) { 
        
           		return; 
        
           	} 
        
           	const forward = keyCode === DELETE; 
        
           	if ( this.props.onMerge ) { 
        
           		this.props.onMerge( forward ); 
        
           	} 
        
           	if ( this.props.onRemove && this.isEmpty() ) { 
        
           		this.props.onRemove( forward ); 
        
           	} 
        
           	event.preventDefault(); 
        
           	// Calling onMerge() or onRemove() will destroy the editor, so it's important 
        
           	// that we stop other handlers (e.g. ones registered by TinyMCE) from 
        
           	// also handling this event. 
        
           	event.stopImmediatePropagation(); 
        
           }

Possibly a bit overzealously, the code there is preventing any other keyboard bindings. I'm fairly certain this was working before, but that code has been there for a while, so maybe not.

¯_(ツ)_/¯

- For Windows that's Ctrl+Alt+Backspace or Ctrl+Alt+Delete

Iron out inconsistent behaviour with use of backspace/delete key when it can result in deletion or merging of blocks. When specific modifiers are pressed the event should not have any effect. Here, the use of preventDefault and stopImmediatePropagation were blocking further binding of keyboard shortcuts and preventing browser/system level keyboard shortcuts.

…s menu

- BlockSettingsMenu was the only consumer of withBlockSettings - shortcuts object is now not injected into props

… add comments

mtias · 2018-08-08T11:40:50Z

I think we need a shortcut for deleting a block, but it's totally fine to split into a separate PR. Thanks a lot for all the work here.

youknowriad · 2018-08-08T11:51:20Z

packages/components/src/menu-item/index.js

@@ -21,7 +21,7 @@ import IconButton from '../icon-button';
 *
 * @return {WPElement} More menu item.
 */
-function MenuItem( { children, className, icon, onClick, shortcut, isSelected = false } ) {
+function MenuItem( { children, className, icon, onClick, shortcut, isSelected, role = 'menuitem', ...props } ) {


Minor: I guess if we remove the role explicit prop and just use role="menuitem" in JSX, it has the exact same effect. Not sure if it's better or if we want to be explicit.

My thinking was that menuitem would usually be the right role, and at least this way there's some out of the box accessibility. Perhaps it's worth adding something to the docs explaining the preferred usage, and also commenting in the code.

youknowriad

Awesome PR, there's probably too much in this PR already. We should try to get this in. The only thing that bothers me is the removal of the duplicate/remove separate components.

youknowriad · 2018-08-08T11:53:09Z

packages/components/src/menu-item/index.js

@@ -40,7 +40,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected =
 				className={ className }
 				icon={ icon }
 				onClick={ onClick }
-				aria-pressed={ isSelected }
+				aria-checked={ isSelected }


Should this be always aria-checked or only when the role is "menuitemcheckbox"?

Yep - I'm relying on an undefined value preventing that attribute from being set on the outputted button element.

Both menuitemcheckbox and menuitemradiobutton are valid roles that can be used with aria-checked when it comes to menuitems.

Yes but please see my last comment. menuitemcheckbox and menuitemradio support is very scarce. It's pointless to use these roles if they're not communicated to users.

Would you say it's better to just use menuitem in that case? How about aria-checked?

youknowriad · 2018-08-08T11:59:56Z

packages/editor/src/components/block-settings-menu/index.js

 								/>
 							) }
-							<BlockDuplicateButton


Why did you decide to remove those components and move them to this one:

I think separating is good to avoid having huge components like this one?

One of the challenges was providing the withDispatch props to the KeyboardShortcuts component. Another is that KeyboardShortcuts has to render regardless of whether the settings menu is open or closed (so that the shortcut is always bound and available while in a block). That meant KeyboardShortcuts couldn't be added to the MenuItems (which would have been ideal).

Because of those constraints KeyboardShortcuts exists in the BlockSettingsMenu, and to avoid duplication I moved the withDispatch functions there as well. After that there was very little left to the individual components, they were just functions that returned MenuItems (with a little bit of boolean logic).

@noisysocks did suggest the logic in the dispatch function could become an effect. I've since found another complication though, that BlockSettingsMenu is also rendered in BlockMultiControls and receives different props from that parent component, so that would also have to be considered.

I thought this seemed to be the best trade-off.

Fair enough 👍

youknowriad · 2018-08-08T12:33:46Z

edit-post/keyboard-shortcuts.js

+		display: displayShortcut.secondary( 'm' ),
+	},
+	toggleSidebar: {
+		raw: rawShortcut.primaryShift( ',' ),


This shortcut is opening a new tab in chrome (Chrome support page)

I wonder is this is another keyboard layout issue, as this key combination has no effect on my keyboard (cmd+shift+,).

Probably, I have a french Keyboard

It looks like there's a , key and you hold shift to output ? on a French keyboard. On a British keyboard layout it's , and you hold shift to output <.

However, on a British keyboard layout, when you press cmd+shift+,, the event handler returns that the , was pressed. On the French keyboard layout it returns ?.

That seems completely inconsistent, and I don't know why that's the case.

afercia · 2018-08-08T16:27:26Z

While the implementation of the roles menuitemcheckbox and menuitemradio is technically correct, actual support is very scarce. JAWS, one of the most advanced screen readers, doesn't support them and it doesn't announce the role. Reference: https://freedomscientific.github.io/VFO-standards-support/aria.html (Last updated 05 June 2018)

NVDA, one of the most advanced screen readers, seems to announce menuitemradio but it doesn't announce menuitemcheckbox at all. It doesn't even say "menu item". Tested with latest Firefox:

With Safari and VoiceOver, I've noticed a degraded experience with focus management when using the standard VoiceOver keystrokes to activate controls: Control-Option-Spacebar.

Also, the various element with a role "menu" have content that is a bit mixed in its nature. In most of the cases they're equivalent to checkboxes or radio buttons. But, if we want to be strict with ARIA, then all the controls within an element with role="menu" should be either menuitem, menuitemcheckbox, or menuitemradio. This is not always the case. For example, the "Copy All Content" button (ClipboardButton) misses a role. It's within a menu but it's not equivalent to a checkbox or radio button. Should it be a menuitem? Is it possible to enforce via code that any item in a menu must have a proper role? Not sure. Also, the span that wraps it should have a role="presentation" to ensure the button is perceived by assistive technologies as first child of the menu.

<div role="menu" aria-orientation="vertical" aria-labelledby="components-menu-group-label-17">
	<span>
		<button type="button" class=" ... ">Copy All Content</button>
	</span>
</div>

Having proper markup with proper ARIA is a bit challenging when it must be done programmatically, and it would probably require an increased level of complexity that should be avoided.

Considering menuitemcheckbox and menuitemradio support is scarce, I wonder why we would want to use them. Also, if we want to be strict with ARIA then we should implement keyboard navigation properly as described in https://www.w3.org/TR/wai-aria-practices/#menu and in the example linked at the beginning of this PR. Navigation should work only with the arrow keys and all the menuitems should have tabindex="-1" to be not tabbable. I'm not sure we really want to do this.

It all depends on what we want to communicate to assistive technologies users. Is it important they get the subtle difference between a menu item radio and a menu item checkbox (assuming they work)? Or maybe the only important thing is they get there's a "group" of controls and, for each control, they get its state? I'm at a point where I'm reconsidering the usage of role="menu" in the first place, but I'd like to ponder a bit on this. Thankfully, this PR standardizes various things and makes future changes easier 👍

Sorry for the long comment but I didn't get this PR was changing various things besides "keyboard shortcuts".

noisysocks

Let's get this in so that it has time to bake for 3.6. We can iterate on the key combinations and correct the ARIA attributes in follow-up PRs.

Great work on this, @talldan! 👏

talldan self-assigned this Jul 30, 2018

talldan requested a review from noisysocks July 30, 2018 08:03

talldan changed the title ~~Add duplicate shortcut to block settings menu~~ Add new keyboard shortcuts for block settings menu Jul 30, 2018

talldan added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Blocks Overall functionality of blocks labels Jul 30, 2018

talldan added this to the 3.5 milestone Jul 30, 2018

talldan requested review from jasmussen and afercia July 30, 2018 08:27

talldan force-pushed the add/keyboard-shortcuts-for-block-settings-menu branch from 26cc6dc to 478ea95 Compare July 31, 2018 09:25

jasmussen mentioned this pull request Jul 31, 2018

Keyboard shortcuts support #71

Closed

35 tasks

talldan force-pushed the add/keyboard-shortcuts-for-block-settings-menu branch from 478ea95 to 82b1b24 Compare August 1, 2018 09:45

noisysocks reviewed Aug 3, 2018

View reviewed changes

talldan force-pushed the add/keyboard-shortcuts-for-block-settings-menu branch from 82b1b24 to 8384ba0 Compare August 6, 2018 10:46

talldan added 15 commits August 8, 2018 16:36

Add modifier key exceptions to prevent deletion or merge of blocks

86b4d02

Make block deletion shortcut Cmd+Opt+Backspace or Cmd+Opt+Delete

0fbdd4e

- For Windows that's Ctrl+Alt+Backspace or Ctrl+Alt+Delete

Make keyboard shortcut for delete Cmd+Alt+Delete

68b9eb7

Abbreviate display shortcut to prevent line wrapping in block setting…

2759893

…s menu

Merge withBlockSettingsActions into BlockSettingsMenu

24fff39

- BlockSettingsMenu was the only consumer of withBlockSettings - shortcuts object is now not injected into props

Remove BlockSettingsKeyboardShortcuts component, which had no logic

98172cb

Use preventDefault to prevent any additional browser/os shortcuts and…

06b72b5

… add comments

Use object method shorthand

fc5a691

Add !important back in, with clarifying comment

8a66776

Use lodash noop over noop arrow function

d4f087c

Destructure dispatching functions

8ea43bf

Refine exceptions to RichText Backspace/Delete handling

783821d

Remove the Remove Block shortcut

c860d5d

Revert changes to RichText back to master version

aad3406

talldan force-pushed the add/keyboard-shortcuts-for-block-settings-menu branch from 03c6123 to aad3406 Compare August 8, 2018 08:44

youknowriad reviewed Aug 8, 2018

View reviewed changes

Fix unit tests (stylelint)

f59bdfb

youknowriad reviewed Aug 8, 2018

View reviewed changes

youknowriad modified the milestones: 3.5, 3.6 Aug 8, 2018

talldan mentioned this pull request Aug 9, 2018

Add "Insert Before" and "Insert After" buttons in the block More menu #7297

Closed

noisysocks approved these changes Aug 10, 2018

View reviewed changes

talldan merged commit 2b4bc5b into master Aug 10, 2018

talldan deleted the add/keyboard-shortcuts-for-block-settings-menu branch August 10, 2018 01:54

talldan mentioned this pull request Aug 10, 2018

Add remove block keyboard shortcut #8805

Merged

4 tasks

afercia mentioned this pull request Nov 2, 2018

MenuItem must use aria-checked only when its role is menuitemcheckbox or menuitemradio #11431

Closed

Add new keyboard shortcuts for block settings menu #8279

Add new keyboard shortcuts for block settings menu #8279

Conversation

talldan commented Jul 30, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

jasmussen commented Jul 30, 2018

afercia commented Jul 30, 2018

abrightclearweb commented Jul 30, 2018

talldan commented Jul 31, 2018 • edited Loading

jasmussen commented Jul 31, 2018

talldan commented Aug 2, 2018

jasmussen commented Aug 2, 2018

noisysocks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan commented Aug 3, 2018

abrightclearweb commented Aug 3, 2018

afercia commented Aug 3, 2018

noisysocks commented Aug 6, 2018

talldan commented Aug 6, 2018

noisysocks commented Aug 6, 2018

jasmussen commented Aug 6, 2018

jasmussen commented Aug 6, 2018

talldan commented Aug 6, 2018 • edited Loading

mtias commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Aug 8, 2018

noisysocks left a comment

Choose a reason for hiding this comment

talldan commented Jul 30, 2018 •

edited

Loading

talldan commented Jul 31, 2018 •

edited

Loading

noisysocks left a comment •

edited

Loading

talldan Aug 7, 2018 •

edited

Loading

talldan Aug 7, 2018 •

edited

Loading

talldan Aug 6, 2018 •

edited

Loading

talldan commented Aug 6, 2018 •

edited

Loading

talldan Aug 9, 2018 •

edited

Loading