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: fix buffer for the triggering of multi-select #14448

Merged
merged 3 commits into from
Mar 15, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 15, 2019

Description

Fixes #12322.

Fixes the buffer for calculating whether, based on the text selection, multi-selection should be triggered or not. This buffer was originally added with only a collapsed selection in mind, so calculating the line height was a simple matter of taking the height of the selection rectangle.

Solution: use getComputedStyle to get the line height from the browser.

Why is this buffer needed in the first place? Depending on the browser, there may be a difference between the height of the selection rectangles and the line height. The selection rectangle may not reach all the way to the line edge. This is why we add a buffer that is the line height divided by two.

How has this been tested?

See #12322. Put the caret at the start of a paragraph that is at least 4 lines long. Press shift+arrowDown and ensure multi-block selection only starts after the last line has been selected.

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.

@ellatrix ellatrix added [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Mar 15, 2019
@ellatrix ellatrix added this to the 5.3 (Gutenberg) milestone Mar 15, 2019
@ellatrix ellatrix requested a review from afercia March 15, 2019 09:14
@ellatrix
Copy link
Member Author

I'll see if I can add a test case.

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.

The implementation is consistent with what I would expect to be an improvement based on the hypothesis in #12322 (comment) . The need for buffer I still find to be unfortunate, though would be happier to see an improvement than delay on it further.

The end-to-end test is great.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Fixes the issue and the code is fine. Thanks for the e2e test!

@ellatrix
Copy link
Member Author

Thanks for the reviews!

@aduth Yeah the buffer is unfortunate, though I see no better way to address the issue. :/

@ellatrix ellatrix merged commit 3848167 into master Mar 15, 2019
@ellatrix ellatrix deleted the try/multi-select-trigger branch March 15, 2019 13:08
@aduth
Copy link
Member

aduth commented Mar 15, 2019

@aduth Yeah the buffer is unfortunate, though I see no better way to address the issue. :/

Right, if I recall correctly, the issue is that the selection bounding box doesn't align neatly with the edges of the input it's in? I remember exploring this and finding it exceedingly difficult to measure that difference accurately, even going so far as to explore possible effects of various text metrics characteristics.

youknowriad pushed a commit that referenced this pull request Mar 20, 2019
)

* Input Interaction: fix buffer for the triggering of multi-select

* Add inline comment

* Add e2e test
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
)

* Input Interaction: fix buffer for the triggering of multi-select

* Add inline comment

* Add e2e test
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... Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] DOM /packages/dom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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