Bug 3862 - Bidi control characters added by the fix to bug 3545 are actually rendered
Summary: Bidi control characters added by the fix to bug 3545 are actually rendered
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-05 06:08 PDT by mitz
Modified: 2005-11-04 00:33 PST (History)
0 users

See Also:


Attachments
Testcase (362 bytes, text/html)
2005-07-05 06:09 PDT, mitz
no flags Details
actual rendering (2.15 KB, image/png)
2005-07-05 06:10 PDT, mitz
no flags Details
expected rendering (1.86 KB, image/png)
2005-07-05 06:11 PDT, mitz
no flags Details
Proposed fix (1.22 KB, patch)
2005-07-05 06:12 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-07-05 06:08:35 PDT
When using a font that contains glyphs for bidi control characters LRO and PDF, each run of visually-
ordered Hebrew is enclosed by those glyphs.

To reproduce: You need a font that contains those glyphs. For the testcase, the version of Arial that 
comes with Windows XP will do (disable all other versions of Arial). Open the testcase.

Expected: Not to see control-character glyphs on either side of the Hebrew text (see screenshot).

Actual: The Hebrew letters are enclosed by the LRO and PDF glyphs.

Analysis: the fix for bug 3545 encloses each run of visually-ordered Hebrew in LRO-PDF, and adjusts 
the from and to members to include the LRO and PDF. It shouldn't. ATSUI will respect the LRO even if it 
occurs before the first character to be rendered.

The patch corrects this by making the from-to range include only the original characters in the run, 
leaving the control characters outside.
Comment 1 mitz 2005-07-05 06:09:24 PDT
Created attachment 2813 [details]
Testcase
Comment 2 mitz 2005-07-05 06:10:40 PDT
Created attachment 2814 [details]
actual rendering
Comment 3 mitz 2005-07-05 06:11:47 PDT
Created attachment 2815 [details]
expected rendering
Comment 4 mitz 2005-07-05 06:12:40 PDT
Created attachment 2816 [details]
Proposed fix
Comment 5 Joost de Valk (AlthA) 2005-07-05 13:01:58 PDT
Confirmed, weird stuff, hebrew is nice tho :P
Comment 6 Dave Hyatt 2005-07-09 14:08:56 PDT
Comment on attachment 2816 [details]
Proposed fix

r=me
Comment 7 Darin Adler 2005-07-10 06:50:10 PDT
Comment on attachment 2816 [details]
Proposed fix

Looks great to me too, although I don't see the need for the cast to (int) in
the expression:

    (int)run->length - 1

since subtracting 1 from the unsigned value is going to make it into a signed
value anyway.
Comment 8 mitz 2005-07-10 07:47:18 PDT
(In reply to comment #7)
> (From update of attachment 2816 [details] [edit])
> Looks great to me too, although I don't see the need for the cast to (int) in
> the expression:
> 
>     (int)run->length - 1
> 
> since subtracting 1 from the unsigned value is going to make it into a signed
> value anyway.
> 

Tell that to the compiler. Without the case, it says "warning: signed and unsigned type in conditional 
expression".
Comment 9 Justin Garcia 2005-07-23 14:02:33 PDT
Landing a fix and adding a manual test, since our layout test font doesn't include glyphs for LRO and PDF.