Bug 4898 – Failures in dbaron's bidi ordering tests

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

Multiple failures in http://dbaron.org/css/test/bidi2 http://dbaron.org/css/test/bidi2_html and http://
dbaron.org/css/test/bidi2_charcode (even with the patch for bug 4862 in place).

I have asked dbaron for permission to attach his tests to this bug and use them as layout tests.

To pass the "HTML" test, bug 3245 needs to be fixed.

Created attachment 3833 [details]
Bidirectional Text Test 2 - CSS

Created attachment 3834 [details]
Bidirectional Text Test 2 - Formatting Characters

Created attachment 3835 [details]
Bidirectional Text Test 2 - HTML

Created attachment 3842 [details]
Bidirectional Text Test 2 - CSS

Added BASE tag

Created attachment 3876 [details]
patch for embedding levels in bidi.cpp

This is a diff from (bidi.cpp+the patch for bug 4862). Together with the patch
from bug 4862, it fixes the "Formatting Characters" and "CSS" variants of the
test, but not the "HTML" variant due to bug 3245. It affects two layout tests:
* fast/dom/HTMLElement/bdo test break differently
* fast/text/international/bidi-explicit-embedding fails due to changes in the
render tree but the end result is the same

Created attachment 3877 [details]
patch for embedding levels in bidi.cpp

One of the assertions in the previous patch exposed the fact that
deleteBidiRuns did not reset emptyRun (so in theory you could get a different
layout on second pass). Added a fix for that as part of this patch.

Created attachment 3880 [details]
patch for embedding levels in bidi.cpp

Actually should call embed() (to push a new context) even if the run is empty.

Created attachment 3885 [details]
patch for embedding levels in bidi.cpp

Created attachment 4004 [details]
patch for embedding levels in bidi.cpp

[Darin Adler]

Comment on attachment 4004 [details]
patch for embedding levels in bidi.cpp

Looking this over makes me realize I really don't understand our bidi
algorithm. But on the other hand, it's all about the tests! And since Mitz is
our expert on this sort of thing, I think we should take this patch.

r=me

[Darin Adler]

These tests don't work well as layout tests because they are long tall documents, and our pixel tests only 
dump the visible portion in an 800x600 window.

We should figure out what to do about this, because we'd really like working regression tests for these 
cases, so either the pixel test machinery or the tests themselves need to be fixed.

Created attachment 4051 [details]
Bidirectional Text Test 2 - Formatting Characters

800x600 version

Created attachment 4052 [details]
Bidirectional Text Test 2 - HTML

800x600 version

Created attachment 4053 [details]
Bidirectional Text Test 2 - CSS

800x600 version

Comment on attachment 4004 [details]
patch for embedding levels in bidi.cpp

Pulling the patch since it triggers a problem with zero-length boxes in
RenderText.

Created attachment 4154 [details]
updated patch

Created attachment 4155 [details]
updated expected test results

Created attachment 4156 [details]
updated expected pixel test result

Created attachment 4159 [details]
better updated patch

Comment on attachment 4159 [details]
better updated patch

Same as attachment #4004 [details] except it avoids creating empty runs (which RenderText
doesn't like).

[Darin Adler]

Comment on attachment 4159 [details]
better updated patch

Great! r=me

[Darin Adler]

Working on landing this one now.

Great to see this finally landed! If only bug 5172 didn't exist :-(

I'm not sure why you indented the if statement with "exclude the embedding char itself" when you landed 
this.

[Darin Adler]

Mitz, to answer your question about indentation:

Sources in WebCore need to be viewed with 8-character tabs. I indented with spaces to match the existing 
surrounding indentation with tabs. I presume you have your editor set to 4-character tabs, so in your 
editor it looked like the indentation matched.

If we replace all the 8-character tabs with spaces, the problem will go away.