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

Input Interaction: only consider selection at edge if directed towards it #14450

Merged
merged 2 commits into from
Mar 15, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 15, 2019

Closes #12322

Description

An alternative to #13638. I chose to make the selection direction check inside isVerticalEdge, as we do the same for isHorizontalEdge. Still needs tests.

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.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Generally, this is great.

There's one small imperfection by the logic, and it's OS-specific as well. Per #13638 (comment) , the behavior of Shift+Up in a non-collapsed selection in a line of a text in non-macOS environments is that it should expand to the previous line (or in our case, if in the first line of a paragraph and a block preceded it, triggering multi-selection). Right now, the behavior is that nothing happens. Effectively in this scenario, I think it might be correct to say that it is at the vertical edge, even if the selection is non-collapsed.

That being said, what exists here is a huge improvement all the same, so I'd like to see it merged.

@@ -169,7 +169,18 @@ export function isVerticalEdge( container, isReverse ) {
}

const selection = window.getSelection();

// Only consider the selection at the edge if the direction is towards the
// edge.
Copy link
Member

Choose a reason for hiding this comment

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

Love the dedication to the 80 column 👍

@ellatrix
Copy link
Member Author

@aduth Thanks for the review! I'll have a look into the issue you're describing, maybe we can fix it separately.

@ellatrix ellatrix merged commit 542151d into master Mar 15, 2019
@ellatrix ellatrix deleted the try/multi-select-direction branch March 15, 2019 14:07
@ellatrix
Copy link
Member Author

@aduth Looking at your GIF, it seems like a consecutive shift+arrowUp press would trigger multi-block selection? The behaviour is slightly different, but you can still select blocks?

chrome

@ellatrix
Copy link
Member Author

Or am I missing something that would block multi-block selection after these steps?

@aduth
Copy link
Member

aduth commented Mar 15, 2019

The specific point I'm trying to make could do for a better / different animation, I think.

Essentially: Everywhere except macOS, if an uncollapsed selection exists within a single line, the effect of pressing ArrowUp (with Shift held) should be to expand the selection to the previous line.

In terms of blocks, that may mean triggering a multi-selection. Or, at the very least, it's contradictory to the idea here that a vertical edge traversal can never occur with an uncollapsed selection.

@ellatrix
Copy link
Member Author

@aduth I think I slowly get what you mean. It would be good to have an issue with the steps to reproduce the issue, and what you'd expect to happen vs what happens now in master. After playing on macOS a bit, it does seem strange that the OS completely disregards the selection direction when using the keyboard after having set it with a pointer device.

@aduth
Copy link
Member

aduth commented Mar 15, 2019

I created an issue at #14463 . Noting that it's improved a bit more from what I was observing, as a result of the changes from #14453, but still not quite as expected.

youknowriad pushed a commit that referenced this pull request Mar 20, 2019
…s it (#14450)

* Input Interaction: only consider selection at edge if directed towards it

* Add e2e test
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
…s it (#14450)

* Input Interaction: only consider selection at edge if directed towards it

* Add e2e test
@ellatrix ellatrix added the [Type] Bug An existing feature does not function as intended label Mar 28, 2019
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... [Package] DOM /packages/dom [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text selection with keyboard triggers blocks multi-selection unexpectedly
3 participants