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.
related to http://bugzilla.opendarwin.org/show_bug.cgi?id=3725 as seen in the comments.
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.
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).
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
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).
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.
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