Bug 213671 - Unable to select multiple lines of vertical text correctly
Summary: Unable to select multiple lines of vertical text correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-26 18:06 PDT by Megan Gardner
Modified: 2020-08-01 01:02 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.11 KB, patch)
2020-06-26 18:13 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2020-07-31 14:36 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (3.40 KB, patch)
2020-07-31 15:27 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (3.40 KB, patch)
2020-08-01 00:34 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-06-26 18:06:56 PDT
Unable to select multiple lines of vertial text correclty.
Comment 1 Megan Gardner 2020-06-26 18:13:55 PDT
Created attachment 402937 [details]
Patch
Comment 2 Megan Gardner 2020-06-26 18:14:19 PDT
<rdar://problem/53753636>
Comment 3 Wenson Hsieh 2020-06-27 12:01:07 PDT
Comment on attachment 402937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402937&action=review

It would also be nice to add a new layout test for this, as well.

> Source/WebKit/ChangeLog:3
> +        Unable to select multiple lines of vertial text correclty.

Nits -
  vertial => vertical
  correclty => correctly

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1501
> +    if (existingSelection.rootEditableElement()->computedStyle()->isVerticalWritingMode()) {

Wouldn't this only work when the selection is in editable content? I think this will also crash when trying to move selection handles in non-editable text on iOS :P

We probably want to check the RenderStyle of the container node of the base VisiblePosition instead.
Comment 4 Megan Gardner 2020-07-31 14:36:37 PDT
Created attachment 405741 [details]
Patch
Comment 5 Darin Adler 2020-07-31 14:43:51 PDT
Comment on attachment 405741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405741&action=review

> Source/WebKit/ChangeLog:14
> +        In order to make for a better text selection experience, we pulled the selection position 
> +        down to be on the last line selectable, rather than snap the selection to a single position.
> +        This made for a better selection experience on small text, but we failed to take
> +        vertical text into account, and a user is locked into only selecting vertical text that ends below the
> +        other anchor point of the selection. We should have the same behavior for vertical text, but correctly 
> +        calculated for X instead of Y.

Can we create a regression test?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1497
> +    WebCore::Node *node = selectionStart.deepEquivalent().containerNode();

auto node =

or

    WebCore::Node* node =
Comment 6 Megan Gardner 2020-07-31 15:15:41 PDT
Comment on attachment 405741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405741&action=review

>> Source/WebKit/ChangeLog:14
>> +        calculated for X instead of Y.
> 
> Can we create a regression test?

The selection test infrastructure is currently broken by changes in UIKit. There are plans to change how those tests are written, and I will add a test with those changes.
Comment 7 Megan Gardner 2020-07-31 15:27:32 PDT
Created attachment 405748 [details]
Patch
Comment 8 Megan Gardner 2020-08-01 00:34:09 PDT
Created attachment 405772 [details]
Patch for landing
Comment 9 EWS 2020-08-01 01:02:09 PDT
Committed r265174: <https://trac.webkit.org/changeset/265174>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405772 [details].