Bug 3599

Summary: Incorrect layout of explicit embedding levels
Product: WebKit Reporter: mitz
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: CLOSED FIXED    
Severity: Normal CC: bugs.mano
Priority: P2    
Version: 412   
Hardware: Mac   
OS: OS X 10.4   
Description Flags
Proposed fix
Proposed fix
darin: review+
Testcase none

Description mitz 2005-06-17 15:47:08 PDT
Summary: The layout is wrong in many cases of explicit Unicode embedding (either by a <span> with 
direction property or by Unicode embedding characters) occurs.

To Reproduce: See testcase.

Expected/Actual: Testcase includes a description of expected rendering. Firefox renders as expected. 
Note that mirroring behavior is also affected by a separate ATSUI issue with certain fonts. Use Lucida 
Grande for testing.

Analysis: The testcase actually expose three issues in bidi.cpp:
1. bidiReorderLine() not closing the "dangling" run when it sees an explicit embedding character (or 
2. embed() not setting up the new run correctly following an embed/pop.
3. bidiReorderLine ()'s implementation of rule L2 of the Unicode bidi algorithm is broken for lines where 
the embedding levels aren't unimodal (the index variable is advancing too fast).

The proposed patch addresses these issues and fixes all the lines in the testcase. Testcase can be used 
as a new layout test.
Comment 1 mitz 2005-06-17 15:49:17 PDT
Created attachment 2453 [details]
Comment 2 mitz 2005-06-17 15:59:11 PDT
Created attachment 2454 [details]
Proposed fix
Comment 3 mitz 2005-06-18 13:50:29 PDT
Comment on attachment 2454 [details]
Proposed fix

Pulling the patch since it causes Safari to hang under some hard-to-reproduce
Comment 4 mitz 2005-06-21 05:41:27 PDT
I've split the 3rd issue (implementation of rule L2) off into bug 3633 .
Comment 5 mitz 2005-06-22 14:51:12 PDT
Created attachment 2551 [details]
Proposed fix
Comment 6 Darin Adler 2005-06-22 19:45:29 PDT
If the test case requires using Lucida Grande, then we should add CSS to make it do that, instead of 
relying on the default font.
Comment 7 Darin Adler 2005-06-22 19:47:46 PDT
Comment on attachment 2551 [details]
Proposed fix

I'm not really qualified to say whether this code is better or not, since I
don't understand the bidi algorithm nor the details of our implementation of
it. But as long as this fixes this test case and doesn't break any existing
cases, I say r=me.
Comment 8 mitz 2005-06-23 05:58:19 PDT
Created attachment 2582 [details]

Specified the use of Lucida Grande and added an item.
Comment 9 mitz 2005-06-24 12:36:47 PDT
Created attachment 2632 [details]

Modified testcase so that the textual description of the expected layout
(digits in increasing orders) would be correct.