Bug 4898 - Failures in dbaron's bidi ordering tests
Summary: Failures in dbaron's bidi ordering tests
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 3245 4862
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-09 02:15 PDT by mitz
Modified: 2005-10-10 10:56 PDT (History)
0 users

See Also:


Attachments
Bidirectional Text Test 2 - CSS (8.23 KB, text/html)
2005-09-09 15:54 PDT, mitz
no flags Details
Bidirectional Text Test 2 - Formatting Characters (8.13 KB, text/html)
2005-09-09 15:55 PDT, mitz
no flags Details
Bidirectional Text Test 2 - HTML (8.01 KB, text/html)
2005-09-09 15:55 PDT, mitz
no flags Details
Bidirectional Text Test 2 - CSS (8.27 KB, text/html)
2005-09-10 08:39 PDT, mitz
no flags Details
patch for embedding levels in bidi.cpp (13.82 KB, patch)
2005-09-12 08:38 PDT, mitz
no flags Details | Formatted Diff | Diff
patch for embedding levels in bidi.cpp (14.00 KB, patch)
2005-09-12 11:48 PDT, mitz
no flags Details | Formatted Diff | Diff
patch for embedding levels in bidi.cpp (15.02 KB, patch)
2005-09-12 12:52 PDT, mitz
no flags Details | Formatted Diff | Diff
patch for embedding levels in bidi.cpp (15.03 KB, patch)
2005-09-13 00:30 PDT, mitz
no flags Details | Formatted Diff | Diff
patch for embedding levels in bidi.cpp (15.58 KB, patch)
2005-09-22 07:40 PDT, mitz
no flags Details | Formatted Diff | Diff
Bidirectional Text Test 2 - Formatting Characters (6.08 KB, text/html)
2005-09-26 16:02 PDT, mitz
no flags Details
Bidirectional Text Test 2 - HTML (5.96 KB, text/html)
2005-09-26 16:03 PDT, mitz
no flags Details
Bidirectional Text Test 2 - CSS (6.23 KB, text/html)
2005-09-26 16:04 PDT, mitz
no flags Details
updated patch (16.75 KB, patch)
2005-10-02 13:32 PDT, mitz
no flags Details | Formatted Diff | Diff
updated expected test results (11.89 KB, patch)
2005-10-02 13:33 PDT, mitz
no flags Details | Formatted Diff | Diff
updated expected pixel test result (43.35 KB, image/png)
2005-10-02 13:35 PDT, mitz
no flags Details
better updated patch (16.41 KB, patch)
2005-10-02 14:49 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-09-09 02:15:48 PDT
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.
Comment 1 mitz 2005-09-09 15:36:20 PDT
To pass the "HTML" test, bug 3245 needs to be fixed.
Comment 2 mitz 2005-09-09 15:54:54 PDT
Created attachment 3833 [details]
Bidirectional Text Test 2 - CSS
Comment 3 mitz 2005-09-09 15:55:32 PDT
Created attachment 3834 [details]
Bidirectional Text Test 2 - Formatting Characters
Comment 4 mitz 2005-09-09 15:55:57 PDT
Created attachment 3835 [details]
Bidirectional Text Test 2 - HTML
Comment 5 mitz 2005-09-10 08:39:29 PDT
Created attachment 3842 [details]
Bidirectional Text Test 2 - CSS

Added BASE tag
Comment 6 mitz 2005-09-12 08:38:49 PDT
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
Comment 7 mitz 2005-09-12 11:48:12 PDT
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.
Comment 8 mitz 2005-09-12 12:52:38 PDT
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.
Comment 9 mitz 2005-09-13 00:30:35 PDT
Created attachment 3885 [details]
patch for embedding levels in bidi.cpp
Comment 10 mitz 2005-09-22 07:40:56 PDT
Created attachment 4004 [details]
patch for embedding levels in bidi.cpp
Comment 11 Darin Adler 2005-09-23 09:07:55 PDT
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
Comment 12 Darin Adler 2005-09-24 04:56:17 PDT
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.
Comment 13 mitz 2005-09-26 16:02:19 PDT
Created attachment 4051 [details]
Bidirectional Text Test 2 - Formatting Characters

800x600 version
Comment 14 mitz 2005-09-26 16:03:24 PDT
Created attachment 4052 [details]
Bidirectional Text Test 2 - HTML

800x600 version
Comment 15 mitz 2005-09-26 16:04:28 PDT
Created attachment 4053 [details]
Bidirectional Text Test 2 - CSS

800x600 version
Comment 16 mitz 2005-10-02 13:07:08 PDT
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.
Comment 17 mitz 2005-10-02 13:32:35 PDT
Created attachment 4154 [details]
updated patch
Comment 18 mitz 2005-10-02 13:33:51 PDT
Created attachment 4155 [details]
updated expected test results
Comment 19 mitz 2005-10-02 13:35:22 PDT
Created attachment 4156 [details]
updated expected pixel test result
Comment 20 mitz 2005-10-02 14:49:45 PDT
Created attachment 4159 [details]
better updated patch
Comment 21 mitz 2005-10-02 14:56:37 PDT
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 22 Darin Adler 2005-10-02 20:33:36 PDT
Comment on attachment 4159 [details]
better updated patch

Great! r=me
Comment 23 Darin Adler 2005-10-08 18:10:38 PDT
Working on landing this one now.
Comment 24 mitz 2005-10-09 10:07:56 PDT
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.
Comment 25 Darin Adler 2005-10-10 10:56:37 PDT
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.