Bug 54724 – Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary/DocumentBoundary) sets incorrect start/end of selection for RTL

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin (Ben) Kalman]

Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary/DocumentBoundary) sets incorrect start/end of selection for RTL

[Benjamin (Ben) Kalman]

Created attachment 82925 [details]
Patch

[Benjamin (Ben) Kalman]

This bug is impossible to reproduce on any current build of chrome or safari because it was only exposed after 54534.

However, to reproduce from tip (>= 78799) open up the test case from that bug (https://bug-54534-attachments.webkit.org/attachment.cgi?id=82596), go to the rightmost side of the text box, press command+shift+left (select to beginning of line) *twice*.  The second time should cancel the selection.  This is the bug.

[Ryosuke Niwa]

Comment on attachment 82925 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary/DocumentBoundary) sets incorrect start/end of selection for RTL
> +        https://bugs.webkit.org/show_bug.cgi?id=54724
> +
> +        Set the end of the selection if the direction is left within an RTL container, and likewise for the start.

I don't think there is sufficient explanation as to how incorrect it is.  You should also explain what caused the issue and how you fixed it.  r- for insufficient descriptions.  Also, since the bug title is really long, you might want to split it into two lines.

[Benjamin (Ben) Kalman]

Well, http://trac.webkit.org/changeset/78799 fixed a bug where the selection was extending in the wrong direction for lines in an RTL container.  That bug would have masked the existence of this one (i.e. they would have appeared to be the same bug), since extending the selection when there is already a selection hits an additional (buggy) codepath.

Namely that on mac the selection is always extended, but this "always extend" code wasn't taking into account the direction (LTR/RTL) of the container.  So in RTL when extending right, it should actually set the start of the selection not the end (since start/end is reversed with respect to left/right).  And likewise when extending left, it should actually set the end.

I'll update the changelog to explain this.

[Benjamin (Ben) Kalman]

Created attachment 83115 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 83115 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This is achieved by extending from the start of selection when going left, or extending from the end when going
> +        right (as opposed from always extending from the extent).

I don't think this comment is a bit misleading since selection is always extended logically and we never "go left" or "go right" when extending the selection.  I think what you mean is that when the selection is extended by the left arrow key and the right arrow key.  Please revise this before landing the patch.

[Benjamin (Ben) Kalman]

Created attachment 83267 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 83267 [details]
Patch for landing

Clearing flags on attachment: 83267

Committed r79290: <http://trac.webkit.org/changeset/79290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.