Bug 3599 - Incorrect layout of explicit embedding levels
Summary: Incorrect layout of explicit embedding levels
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-06-17 15:47 PDT by mitz
Modified: 2005-11-04 00:31 PST (History)
1 user (show)

See Also:


Attachments
testcase (1.02 KB, text/html)
2005-06-17 15:49 PDT, mitz
no flags Details
Proposed fix (1.36 KB, patch)
2005-06-17 15:59 PDT, mitz
no flags Details | Formatted Diff | Diff
Proposed fix (774 bytes, patch)
2005-06-22 14:51 PDT, mitz
darin: review+
Details | Formatted Diff | Diff
Testcase (1.12 KB, text/html)
2005-06-23 05:58 PDT, mitz
no flags Details
Testcase (1.12 KB, text/html)
2005-06-24 12:36 PDT, mitz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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 
PDF).
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]
testcase
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
circumstances.
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]
Testcase

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

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