Blocks: Select ALL on Ctrl+A outside editable DOM elements by youknowriad · Pull Request #1211 · WordPress/gutenberg

youknowriad · 2017-06-16T11:56:24Z

closes #3

So basically, I'm listening to all Ctrl+A and Command+A events and if the active element is not editable (is not an input, textarea, select or contenteditable), I trigger a global multi selection.

aduth

Noting that this takes over select-all for the entire screen, which may be what we're expecting. Try Cmd-A on any page (including this GitHub page) to see the effect I'm illustrating.

aduth · 2017-06-16T13:51:30Z

 */
 import { __ } from 'i18n';
 import { Component, findDOMNode } from 'element';
+import { LETTER_A, SMALL_LETTER_A } from 'utils/keycodes';


Two things:

Maybe LOWER_LETTER_A (lowercase, uppercase distinction)

Do we need to capture uppercase A? Cmd-Shift-A is not typically a valid combination for Select All behavior.

Actually, I'm going to use only LETTER_A because there's no difference between the two in keydown events. (The keycode is the same)

aduth · 2017-06-16T13:55:01Z


+	onKeyDown( event ) {
+		const { uids } = this.props;
+		const isEditable = [ 'textarea', 'input', 'select' ].indexOf( document.activeElement.tagName.toLowerCase() ) !== -1


Starting to see these conditions pop up in a few PRs with minor variations. Seems prone to edge cases or errors. Tested utilities might be more appropriate.

Wondering if there's another way to test here:

If there's no activeElement (or is the default, document.body I think)

If there's no focused blocks

The idea was to enable Ctrl+A even if we're focusing a button or any focusable but not editable. We could restrict to document.body of course.

cc @jasmussen

I'm still a bit hesitant about this logic. Do you think this should be in a utility class, e.g. isActiveElementEditable, or isEditableElement( document.activeElement ) ?

I'd found that in some browsers, window.getSelection() has a type property against which we could perhaps test === 'caret' for reasonable accuracy, but unfortunately not all browsers define this property:

https://developer.mozilla.org/en-US/docs/Web/API/Selection/type

jasmussen · 2017-06-16T14:17:41Z

This is really really really nice! Thanks for working on it.

⌘ A inside a block selects all there, ⌘ A outside, selects all blocks.

Noting that this takes over select-all for the entire screen, which may be what we're expecting. Try Cmd-A on any page (including this GitHub page) to see the effect I'm illustrating.

The idea was to enable Ctrl+A even if we're focusing a button or any focusable but not editable. We could restrict to document.body of course.

I could go either way on this one. On the one hand, having it be unrestricted (i.e. ⌘ A no matter what has focus, unless of course it's inside a textarea selects all blocks) is similar to many desktop apps, even ones that have served in part as inspiration for some of our efforts, like Sketch. On the other hand, it's a non-standard web-page behavior. So which are we more, an app, or a web-page? I'm leaning towards the former, i.e. keeping the behavior as is.

One thing though — it would be nice (not super important, can be separate PR, low priority), if clicking anywhere outside the selected blocks deselected. Right now you can press the white area outside of the blocks to deselect. It'd be nice if you could also click the empty space under the sidebars.

But I'd like a sanity check. @melchoyce @folletto any thoughts? Here's a video where I talk over: https://cloudup.com/csoAyszQ-Bf

melchoyce · 2017-06-17T07:10:49Z

The interaction feels nice! I like the top bar with the number of blocks selected and "delete." Only weird thing IMO is copying the blocks, copies the HTML. I expected it to just copy the text of the blocks so I could paste it into another text editor.

youknowriad · 2017-06-19T10:26:05Z

One thing though — it would be nice (not super important, can be separate PR, low priority), if clicking anywhere outside the selected blocks deselected. Right now you can press the white area outside of the blocks to deselect. It'd be nice if you could also click the empty space under the sidebars.

This has implications, because we want to keep selection if we click the inserter and post settings. I think the "deselection" behaviour should be addressed separately, it's more complex than it seems.

Should we merge here?

jasmussen · 2017-06-19T10:27:39Z

I'm okay with merging, if there are no code objections.

ellatrix · 2017-06-19T10:57:40Z

 export const RIGHT = 39;
 export const DOWN = 40;
 export const DELETE = 46;
+export const LETTER_A = 65;


Where we can, would it be better to calculate for clarity? Also maybe it's better to call it CHAR_A (broader collection)?

export const CHAR_A = 'A'.charCodeAt(0);

Here is some useful info on using event.keyCode: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode. Could we use event.key instead? https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

I recall discussions about compatibility issues for event.key in some browsers. cc @aduth
And we're using keyCode everywhere now.

Related: #1215 (comment)

Not sure what the issues are with IE11 and "non-standard key identifiers", also only available in latest iOS version, which violates documented browser support.

folletto · 2017-06-19T11:55:40Z

Nice one! 👍

I +1 Mel's comment. ;)

youknowriad · 2017-06-19T15:28:41Z

Only weird thing IMO is copying the blocks, copies the HTML. I expected it to just copy the text of the blocks so I could paste it into another text editor.

Yes, Copy/Paste need more ❤️ and is tracked here #943

aduth · 2017-06-19T15:57:52Z

+			|| document.activeElement.isContentEditable;
+		if (
+			! isEditable &&
+			( event.ctrlKey || event.metaKey ) &&


A bit unexpected that Ctrl-A on Mac will select all (not default behavior), but trying to distinguish by OS seems to devolve into absurdity quite quickly, so I'm not sure we care to spend the effort.

Is event.metaKey defined by other OSs? If not, we could look at that.

On Windows, it's the Windows key (⊞), and conversely ⊞+a is not a valid "select all" combination.

I suppose in windows, it won't trigger because it triggers the "start" menu?

trying to distinguish by OS seems to devolve into absurdity quite quickly, so I'm not sure we care to spend the effort

😆

Ok let's get this merged then :). An approval?

aduth · 2017-06-19T16:05:47Z


+	onKeyDown( event ) {
+		const { uids } = this.props;
+		const isEditable = [ 'textarea', 'input', 'select' ].indexOf( document.activeElement.tagName.toLowerCase() ) !== -1


I'm still a bit hesitant about this logic. Do you think this should be in a utility class, e.g. isActiveElementEditable, or isEditableElement( document.activeElement ) ?

I'd found that in some browsers, window.getSelection() has a type property against which we could perhaps test === 'caret' for reasonable accuracy, but unfortunately not all browsers define this property:

https://developer.mozilla.org/en-US/docs/Web/API/Selection/type

Blocks: Select ALL on Ctrl+A outside editable dom elements

00cf106

youknowriad self-assigned this Jun 16, 2017

youknowriad requested review from ellatrix and jasmussen June 16, 2017 11:56

aduth reviewed Jun 16, 2017

View reviewed changes

Blocks: Use only LETTER_A keycode to trigger select All

3888e2e

ellatrix reviewed Jun 19, 2017

View reviewed changes

Framework: Rename LETTER_A CHAR_A

d03ee07

aduth reviewed Jun 19, 2017

View reviewed changes

Framework: Extract isEditableElement utility

c75afd1

ellatrix mentioned this pull request Jun 21, 2017

Simple text interactions #179

Closed

20 tasks

aduth approved these changes Jun 21, 2017

View reviewed changes

youknowriad merged commit 5c24018 into master Jun 22, 2017

youknowriad deleted the try/select-all branch June 22, 2017 10:39

youknowriad mentioned this pull request Jun 22, 2017

Tests: Drop .only to run all unit tests #1370

Merged

aduth mentioned this pull request Jul 19, 2017

Try: Add component for handling keyboard events #1944

Merged

Conversation

youknowriad commented Jun 16, 2017

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Jun 16, 2017

Uh oh!

melchoyce commented Jun 17, 2017

Uh oh!

youknowriad commented Jun 19, 2017

Uh oh!

jasmussen commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

folletto commented Jun 19, 2017

Uh oh!

youknowriad commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

youknowriad commented Jun 19, 2017 •

edited

Loading

youknowriad Jun 20, 2017 •

edited

Loading