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

Expand selection on consecutive Meta+A presses #4600

Merged
merged 5 commits into from May 22, 2018
Merged

Conversation

ellatrix
Copy link
Member

Description

See #4369. This PR only adds going straight to selecting all blocks, just like it does now block selection without input focus.

How Has This Been Tested?

Focus an Editable. meta+a should select all contents. Press meta+a once more to select all blocks. Try one by releasing the meta key, and once by holding it. Both ways should work.

Annoying thing here is that Editable and other input fields behave slightly different. For Editable selection happens before the keyDown event (because of TinyMCE intercepting), for the rest it happens after.

@aduth
Copy link
Member

aduth commented Jan 28, 2018

There's a failing E2E test here.

@ellatrix ellatrix force-pushed the try/select-all branch 2 times, most recently from bca8e2f to 7008b57 Compare February 20, 2018 12:09
@ellatrix ellatrix requested a review from a team February 20, 2018 12:15
@jasmussen
Copy link
Contributor

I like this a lot. I think we want this.

One thing that becomes slightly more apparent now that it's easier to select all, is that focus is set on the block ellipsis menu when selecting all blocks, which scrolls you all the way to the top if if selected all on a long document.

I know we have to set focus somewhere, and that particular niggle does not have to be addressed as part of this PR. Perhaps the answer to where we set focus, when multi selecting, can be found in this ticket.

Another thing related to that — when I set the cursor in a paragraph, ⌘A once, then twice, then press Escape to deselect all blocks, I still have the text selection from the paragraph before, but I can't use the arrow-keys anymore because focus is now on the canvas itself, no longer on that block. It feels like the ideal solution would be to set focus back to where it was — but probably the right solution would be to deselect that text so when you press Escape twice, everything is unselected. Would be a nice fix if easy.

One thing that seems to be a regression from master — if I don't have anything selected, ⌘A now selectes everything on the page using the browser selection model, it doesn't select blocks like it did before. Now you have to set focus inside a paragraph or editable and ⌘A twice to select all blocks using the keyboard.

@ellatrix
Copy link
Member Author

Rebased and updated this. @jasmussen Addressed the last two points.

@ellatrix ellatrix requested a review from tofumatt May 15, 2018 22:10
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Totally love this. Approving based on experience, I can't speak to the code other than "looks good".

A future enhancement to look at, perhaps as part of the navigation mode stuff Riad has been looking at, could be to restore the caret to the block it was in, when you press escape. I.e. you're in block 3, ⌘A selects all text in block 3, ⌘A again selects all blocks, escape takes you back to block 3 with the caret.

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.

The code largely looks good and I really like this functionality! 👍

I'd like a few small docs changes and some documentation/changes for the is function which I feel is a scary/not clear name.

Otherwise looks good! 😄

@@ -201,6 +202,25 @@ class WritingFlow extends Component {
}

if ( ! isNav ) {
const activeElement = document.activeElement;

// Set right before the meta+a combination can be pressed.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "right before", I would write "immediately before". It's clearer and a bit easier to parse (I had to re-read this sentence to grok it.)

event.preventDefault();
}

// Set in case the meta key doesn't get released.
Copy link
Member

Choose a reason for hiding this comment

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

I feel this comment could be expanded to say what is being set. This reads as a sentence fragment and I'm not really sure what it means.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to improve it but unsure if it addressed the issue.

Copy link
Member

Choose a reason for hiding this comment

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

That totally works actually, I get it now 😄

utils/dom.js Outdated
@@ -395,6 +395,40 @@ export function documentHasSelection() {
return range && ! range.collapsed;
}

/**
* Check wether the contents of the element have been fully selected.
Copy link
Member

Choose a reason for hiding this comment

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

wether > whether

utils/dom.js Outdated
*
* @return {boolean} True if fully selected, false if not.
*/
export function isFullySelected( element ) {
Copy link
Member

Choose a reason for hiding this comment

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

I might name this "entirely" rather than "fully" unless there's a history of using "fullySelected". "entirely" just seems a bit clearer and more explicit as to what this means.


export const is = mapValues( modifiers, ( getModifiers ) => {
return ( event, character, _isMac = isMacOS ) => {
const mods = getModifiers( _isMac );
Copy link
Member

Choose a reason for hiding this comment

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

Could we call this modifiers instead of mods? Explicit variable names are nice and we can afford the bytes 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

That would conflict with modifiers higher up.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, duh, sorry about that. 👍

@@ -90,3 +90,19 @@ export const displayShortcut = mapValues( modifiers, ( modifier ) => {
return shortcut.replace( /⌘\+([A-Z0-9])$/g, '⌘$1' );
};
} );

export const is = mapValues( modifiers, ( getModifiers ) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of a confusing function name to me. "is" what? I get it works like is.primary( 'a' ) but that isn't explained in a comment/docblock here and that's a bit of an unexpected API to me. Maybe matchesKeycode or something? Or a doc block here to make this a bit easier to understand without gripping through the codebase for usage (which is what I needed to do).

Copy link
Member Author

Choose a reason for hiding this comment

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

It checks more than just the keyCode, so matchesKeycode feels a bit weird. What we want to do is check if the keydown/keyup event matches the modifiers and letter. Let me think. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe something like matchesShortcut? Or KeyboardEventIsPrimary?

Copy link
Member Author

@ellatrix ellatrix May 17, 2018

Choose a reason for hiding this comment

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

How about isKeyboardEvent.primary( event, 'a' )?

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.

Awesome, thanks for the little tweaks to everything! isKeyboardEvent is perfect 😄

*
* @param {Element} element The element to check.
*
* @return {boolean} True if fully selected, false if not.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just caught this is still "fully selected"–if you wanted to change it to "entirely selected" as well that might be nice for consistency. 😅

@mtias mtias added this to the 3.0 milestone May 22, 2018
@ellatrix ellatrix merged commit 75d3c35 into master May 22, 2018
@ellatrix ellatrix deleted the try/select-all branch May 22, 2018 19:59
@@ -64,6 +64,7 @@ class EditorGlobalKeyboardShortcuts extends Component {
const { hasMultiSelection, clearSelectedBlock } = this.props;
if ( hasMultiSelection ) {
clearSelectedBlock();
window.getSelection().removeAllRanges();
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a side-effect (e.g. store/effects.js) of the CLEAR_SELECTED_BLOCK action?

*
* @type {Object} Keyed map of functions to match events.
*/
export const isKeyboardEvent = mapValues( modifiers, ( getModifiers ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should have unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

This is not the behavior I'd expect from a function named isKeyboardEvent. At first glance, I expected it to return a boolean reflecting whether a given Event implements KeyboardEvent.

*
* @return {boolean} True if entirely selected, false if not.
*/
export function isEntirelySelected( element ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should have unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants