Bug 11811 - REGRESSION (r11783): Hebrew text in list boxes is reversed
Summary: REGRESSION (r11783): Hebrew text in list boxes is reversed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Major
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-12-12 08:26 PST by mitz
Modified: 2007-02-05 15:35 PST (History)
0 users

See Also:


Attachments
Test case (215 bytes, text/html)
2006-12-12 08:26 PST, mitz
no flags Details
patch (97.07 KB, patch)
2007-02-02 22:22 PST, Adele Peterson
adele: review-
Details | Formatted Diff | Diff
updated patch (97.89 KB, patch)
2007-02-05 14:25 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
new patch! (188.04 KB, patch)
2007-02-05 15:25 PST, Adele Peterson
mitz: 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-12-12 08:26:07 PST
Plain Hebrew text in list boxes is laid out in reverse, that is, left-to-right. See the attached test case. Notice that if you select Debug > Use ATSUI For All Text and reload, the text is laid out correctly.

This bug is a regression from fixing bug 4844 (SVN doesn't seem to have a revision number for that CVS-era change). It is currently affecting the engine-based list boxes but it also affected the previous implementation.
Comment 1 mitz 2006-12-12 08:26:40 PST
Created attachment 11823 [details]
Test case
Comment 2 mitz 2006-12-12 08:49:30 PST
Bug 11812 is similar. GraphicsContext::drawText is not suitable for drawing arbitrary strings.
Comment 3 David Kilzer (:ddkilzer) 2006-12-12 09:16:22 PST
(In reply to comment #0)
> This bug is a regression from fixing bug 4844 (SVN doesn't seem to have a
> revision number for that CVS-era change).

It's r11783.  (I ran "svn log WebKit/WebCoreSupport" and looked for the bug number in the commit text. :)
Comment 4 mitz 2006-12-12 09:28:16 PST
(In reply to comment #3)
> It's r11783.  (I ran "svn log WebKit/WebCoreSupport" and looked for the bug
> number in the commit text. :)
> 

Thank you!
Comment 5 Stephanie Lewis 2007-01-22 18:37:22 PST
In Radar 4947184
Comment 6 Adele Peterson 2007-02-02 22:22:02 PST
Created attachment 12891 [details]
patch
Comment 7 Adele Peterson 2007-02-03 00:34:52 PST
Comment on attachment 12891 [details]
patch

I'm going to rework this and post a new patch.

Things to address:

-malloc less by using a Vector<UChar>
- don't call bidi algorithm in the directional override case
- If there's one run, and its not reversed, return early
Comment 8 mitz 2007-02-04 05:15:56 PST
Comment on attachment 12891 [details]
patch

+        TextStyle textStyle(0, 0, 0, style()->direction() == RTL, style()->visuallyOrdered() == true);

Since the characters in the buffer are in LTR visual order (that's the result of bidiReorderCharacters), the flags should always be (..., false, true). This will ensure that ATSUI's own bidi algorithm doesn't kick in. The only exception is that if style()->direction() == RTL and style()->unicodeBidi() == Override, in which case you *may* skip bidiReorderCharacters and use (..., true, true). In the other override cases (LTR+Override or VisuallyOrdered) you skip bidiReorderCharacters and use (..., false, true).
Comment 9 Adele Peterson 2007-02-05 14:25:31 PST
Created attachment 12949 [details]
updated patch

I tried to address Mitz's comments with this patch.

There may be a cleaner way to put the direction override logic though.
Comment 10 Adele Peterson 2007-02-05 15:25:51 PST
Created attachment 12955 [details]
new patch!
Comment 11 mitz 2007-02-05 15:33:27 PST
Comment on attachment 12955 [details]
new patch!

r=me with one change discussed on IRC. Anders okayed the CharacterBuffer typedef.
Comment 12 Adele Peterson 2007-02-05 15:35:26 PST
Committed revision 19414.