Bug 3733 – Incorrect bidi layout of ETs, ANs, and ENs in some contexts

• Component: Layout and Rendering
• Status: CLOSED FIXED

The testcase for this bug includes various bidi sequences that WebKit lays out in the wrong order, and 
other sequences that should be included in a layout test even though they're currently laid out as 
expected.

Firefox renders everything as expected.

Some of the testcases are based on the KHTML bidi layout test <http://websvn.kde.org/*checkout*/
trunk/tests/khtmltests/i18n/bidi.html?rev=409208>. (It uses directional overrides to display the 
expected layout, and WebKit doesn't handle them correctly, so the KHTML layout test can't be used as a 
WebKit layout test as-is).

Created attachment 2667 [details]
Testcase

Created attachment 2668 [details]
Proposed fix

The proposed patch makes both the testcase and the KHTML layout test render as
expected. It makes 12 of the layout tests fail, because it splits a few runs,
but they render the same with it. It is possible, alternatively, to not split
those runs and in fact merge many other currently-split runs (bidi.cpp:922),
but then over 100 layout tests will fail and will have to be checked.

I haven't tested the patch with "compact runs". I don't know what they are nor
how to make them appear.

The proposed fix also addresses bug 3725.

[Joost de Valk (AlthA)]

related to http://bugzilla.opendarwin.org/show_bug.cgi?id=3725 as seen in the comments.

[Darin Adler]

I like the idea of not splitting runs -- I'd love to see that patch landed after this one, even though it would 
affect a lot of layout tests.

[Darin Adler]

Comment on attachment 2668 [details]
Proposed fix

With a sizable patch like this, it would be nice if the new code fit our coding
style guidelines <http://webkit.opendarwin.org/coding/coding-style.html>.

I don't think it's good style to leave behind commented-out lines of code.

That having been said, as long as the layout test results are still good, then
it seems fine to land this.

Unfortunately I don't know the concepts well enough to review this for real,
but I don't think any of the reviewers do at the moment.

I'd still very much like to see the version of this that makes fewer runs
landed eventually.

Created attachment 2685 [details]
Proposed fix

Improved coding style.

(In reply to comment #6)

> I'd still very much like to see the version of this that makes fewer runs
> landed eventually.

Going over those 130 or so failed layout tests could be easier if there was a tool that rendered any given 
layout test into a PNG. Even if the expected renderings weren't committed to the source tree (as they do in 
KHTML), it would still be possible to "flash" between the old and new renderings and verify that the 
differences, if any, are acceptable (when runs are merged, sometimes type moves a little horizontally, 
probably due to rounding).

[Darin Adler]

Comment on attachment 2685 [details]
Proposed fix

There are still some "if(" in here with no space, but looks ready to land to
me, I guess.

r=me

[Geoffrey Garen]

This patch "breaks" the following layout tests:

css1/basic/containment
css1/box_properties/float_on_text_elements
css1/font_properties/font	
css1/formatting_model/inline_elements
css1/pseudo/anchor	expected
editing/style/relative-font-size-change-001
editing/style/relative-font-size-change-004
fast/block/basic/018
fast/invalid/nestedh3s
fast/js/date-parse-test
fast/lists/003
fast/selectors/166
fast/text/basic/004

Is this intended, or review- ?

(In reply to comment #10)
> This patch "breaks" the following layout tests:
> 
> css1/basic/containment
> css1/box_properties/float_on_text_elements
> css1/font_properties/font       
> css1/formatting_model/inline_elements
> css1/pseudo/anchor      expected
> editing/style/relative-font-size-change-001
> editing/style/relative-font-size-change-004
> fast/block/basic/018
> fast/invalid/nestedh3s
> fast/js/date-parse-test
> fast/lists/003
> fast/selectors/166
> fast/text/basic/004
> 
> Is this intended, or review- ?

fast/js/date-parse-test wasn't broken by this patch (see bug 3762). As for the others, see comment #2. 
I think those few split runs are acceptable (in most cases rendering isn't affected at all, and in others 
text moves by fractions of a pixel).

[Dave Hyatt]

Why do we need to split the runs?  Can't the old behavior be preserved?

(In reply to comment #12)
> Why do we need to split the runs?  Can't the old behavior be preserved?

We can avoid the splits, but the most straightforward way to do that (see patched bidi.cpp:922) would also 
fix a ton of other unnecessary splits that currently occur.

I can try to replicate the old behavior (it will probably require an extra field in bidi.status just for that), but 
why is it important?

To elaborate on comment #13, old behavior was to split whenever there's EN followed by L (such as in 
"over 15px"). However, if the EN was at the beginning of the line (or immediately followed an L, such as in 
"PPC640e") then the EN would change to an L and the split wouldn't happen. It is only under those rare 
circumstances that the patch behaves differently, because the patch prevents the changing of EN into L. 
The reason for not changing EN into L is that in L EN ET AN you need to know that the EN really was an EN 
and not an L,  since it affects the ET differently.

[Dave Hyatt]

Ok, sounds good.  Can we get a followup bug filed on the work to fix these splits + the other unnecessary 
ones?

(In reply to comment #15)
> Ok, sounds good.  Can we get a followup bug filed on the work to fix these splits + the other 
unnecessary 
> ones?
> 

Done. bug 3838