Bug 54724 - Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary/DocumentBoundary) sets incorrect start/end of selection for RTL
Summary: Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Benjamin (Ben) Kalman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-17 22:54 PST by Benjamin (Ben) Kalman
Modified: 2011-02-21 22:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (8.35 KB, patch)
2011-02-17 22:59 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (8.76 KB, patch)
2011-02-20 18:57 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch for landing (8.78 KB, patch)
2011-02-21 21:36 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin (Ben) Kalman 2011-02-17 22:54:55 PST
Extending selection by a boundary granularity (LineBoundary/ParagraphBoundary/DocumentBoundary) sets incorrect start/end of selection for RTL
Comment 1 Benjamin (Ben) Kalman 2011-02-17 22:59:45 PST
Created attachment 82925 [details]
Patch
Comment 2 Benjamin (Ben) Kalman 2011-02-17 23:04:18 PST
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.
Comment 3 Ryosuke Niwa 2011-02-18 04:06:55 PST
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.
Comment 4 Benjamin (Ben) Kalman 2011-02-20 18:48:55 PST
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.
Comment 5 Benjamin (Ben) Kalman 2011-02-20 18:57:15 PST
Created attachment 83115 [details]
Patch
Comment 6 Ryosuke Niwa 2011-02-21 00:25:55 PST
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.
Comment 7 Benjamin (Ben) Kalman 2011-02-21 21:36:04 PST
Created attachment 83267 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2011-02-21 22:24:52 PST
Comment on attachment 83267 [details]
Patch for landing

Clearing flags on attachment: 83267

Committed r79290: <http://trac.webkit.org/changeset/79290>
Comment 9 WebKit Commit Bot 2011-02-21 22:24:58 PST
All reviewed patches have been landed.  Closing bug.