Bug 4898

Summary:

Failures in dbaron's bidi ordering tests

Product:

WebKit

Reporter:

mitz

Component:

Layout and Rendering

Assignee:

Darin Adler <darin>

Status:

VERIFIED FIXED

Severity:

Normal

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

3245, 4862

Bug Blocks:

Attachments:

Description	Flags
Bidirectional Text Test 2 - CSS	none
Bidirectional Text Test 2 - Formatting Characters	none
Bidirectional Text Test 2 - HTML	none
Bidirectional Text Test 2 - CSS	none
patch for embedding levels in bidi.cpp	none
patch for embedding levels in bidi.cpp	none
patch for embedding levels in bidi.cpp	none
patch for embedding levels in bidi.cpp	none
patch for embedding levels in bidi.cpp	none
Bidirectional Text Test 2 - Formatting Characters	none
Bidirectional Text Test 2 - HTML	none
Bidirectional Text Test 2 - CSS	none
updated patch	none
updated expected test results	none
updated expected pixel test result	none
better updated patch	darin: review+

mitz

Reported 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.

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
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

mitz

Comment 1 2005-09-09 15:36:20 PDT

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

mitz

Comment 2 2005-09-09 15:54:54 PDT

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

mitz

Comment 3 2005-09-09 15:55:32 PDT

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

mitz

Comment 4 2005-09-09 15:55:57 PDT

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

mitz

Comment 5 2005-09-10 08:39:29 PDT

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

mitz

Comment 6 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

mitz

Comment 7 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.

mitz

Comment 8 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.

mitz

Comment 9 2005-09-13 00:30:35 PDT

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

mitz

Comment 10 2005-09-22 07:40:56 PDT

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

Darin Adler

Comment 11 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

Darin Adler

Comment 12 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.

mitz

Comment 13 2005-09-26 16:02:19 PDT

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

mitz

Comment 14 2005-09-26 16:03:24 PDT

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

mitz

Comment 15 2005-09-26 16:04:28 PDT

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

mitz

Comment 16 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.

mitz

Comment 17 2005-10-02 13:32:35 PDT

Created attachment 4154 [details] updated patch

mitz

Comment 18 2005-10-02 13:33:51 PDT

Created attachment 4155 [details] updated expected test results

mitz

Comment 19 2005-10-02 13:35:22 PDT

Created attachment 4156 [details] updated expected pixel test result

mitz

Comment 20 2005-10-02 14:49:45 PDT

Created attachment 4159 [details] better updated patch

mitz

Comment 21 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).

Darin Adler

Comment 22 2005-10-02 20:33:36 PDT

Comment on attachment 4159 [details] better updated patch Great! r=me

Darin Adler

Comment 23 2005-10-08 18:10:38 PDT

Working on landing this one now.

mitz

Comment 24 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.

Darin Adler

Comment 25 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.

Note You need to log in before you can comment on or make changes to this bug.