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
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
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).
Comment on attachment 4159 [details] better updated patch Great! r=me
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.
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.