Bug 7433 - REGRESSION (r12789): Second RTL text run on a line cannot be selected
Summary: REGRESSION (r12789): Second RTL text run on a line cannot be selected
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2006-02-23 14:56 PST by mitz
Modified: 2006-03-01 09:34 PST (History)
2 users (show)

See Also:


Attachments
Test case (380 bytes, text/html)
2006-02-23 14:57 PST, mitz
no flags Details
Fix the regression (7.10 KB, patch)
2006-02-26 12:10 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-02-23 14:56:54 PST
Clicking or dragging to select the second run and subsequent runs of RTL text on a line fails. See the test case for an example where you cannot drag or double-click to select the rightmost two words of Hebrew text, since they are separated from the first two words by a double space (causing them to be in a different text run).

Using nightly builds I tracked it down to somewhere between r12786 and r12791, and r12789 looks like the most likely candidate to be the cause.
Comment 1 mitz 2006-02-23 14:57:40 PST
Created attachment 6687 [details]
Test case
Comment 2 mitz 2006-02-24 07:48:50 PST
This bug is a result of Position::inRenderedText()'s assumption that RenderText's inline boxes are ordered by their start offset. This assumption is false in many bidi contexts.

The same logic is used in at least two other places: Position::isRenderedCharacter() and RenderText::inlineBox().
Comment 3 mitz 2006-02-26 12:10:37 PST
Created attachment 6746 [details]
Fix the regression

This fixes inRenderedText() (which is responsible for this bug) and isRenderedCharacter(), but not other places where logical ordering of the text boxes is wrongly assumed. Those seem to be in editing code and require separate bugs and test cases.
Comment 4 Darin Adler 2006-02-26 15:40:20 PST
Comment on attachment 6746 [details]
Fix the regression

These fixes seem to leave in an optimization for renderers that don't contain reversed text. But couldn't we just remove the optimization entirely?
Comment 5 David Harrison 2006-02-26 17:16:12 PST
I would think so.  Before my patch, Position::inRenderedContent had this optimization but VisiblePosition::isCandidate() did not.
Comment 6 Darin Adler 2006-02-27 08:20:53 PST
Comment on attachment 6746 [details]
Fix the regression

Lets get this fix landed -- we can ponder whether to remove the regression altogether later.

r=me
Comment 7 Darin Adler 2006-02-27 10:58:28 PST
Comment on attachment 6746 [details]
Fix the regression

I meant to say "we can ponder whether to remove the *optimization* altogether later".
Comment 8 mitz 2006-03-01 09:34:36 PST
Verified in r13060 nightly