Bug 17557

Summary:

incorrect results for css2.1 layout tests that use setTimeout in body onload event

Product:

WebKit

Reporter:

Tony Chang <tony>

Component:

Tools / Tests

Assignee:

Carol Szabo <carol>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, ap, carol, darin, hamaji, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
remove setTimeout and update expected results	mitz: review-
fork files and use waitUntilDone/notifyDone	none
Patch for the 2 affected tests	none
Create copy in fast/css/counters	none

Tony Chang

Reported 2008-02-26 13:40:39 PST

There are six layout tests that have the wrong expected results. These tests have the following: <body onload="setTimeout('run()', 0)"> Which doesn't get run when the layout test results are dumped. The tests that do this are: css2.1/t1204-increment-00-c-o.html css2.1/t1204-increment-01-c-o.html css2.1/t1204-increment-02-c-o.html css2.1/t1204-reset-00-c-o.html css2.1/t1204-reset-01-c-o.html css2.1/t1204-reset-02-c-o.html I have a patch to remove the setTimeout since it's not needed.

Attachments
remove setTimeout and update expected results (15.79 KB, patch) 2008-02-26 13:42 PST, Tony Chang	mitz: review-	Details Formatted Diff Diff
fork files and use waitUntilDone/notifyDone (12.72 KB, patch) 2008-03-03 15:46 PST, Tony Chang	no flags	Details Formatted Diff Diff
Patch for the 2 affected tests (5.16 KB, patch) 2008-05-21 09:54 PDT, Bradley Meck	no flags	Details Formatted Diff Diff
Create copy in fast/css/counters (121.27 KB, patch) 2010-01-21 18:28 PST, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Tony Chang

Comment 1 2008-02-26 13:42:11 PST

Created attachment 19384 [details] remove setTimeout and update expected results I tried to update the pixel test results too, but I couldn't get any of the css2.1 tests to pass on my machine (Tiger).

mitz

Comment 2 2008-02-26 13:53:21 PST

I think the proposed change makes the tests invalid. The purpose of the timeout is to defer the change until after the document has been laid out and rendered once.

Tony Chang

Comment 3 2008-02-26 14:02:56 PST

Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets run.

mitz

Comment 4 2008-02-26 14:07:19 PST

(In reply to comment #3) > Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets > run. That might work, but I think that as a rule the tests in /css2.1 are not to be modified. If these scenarios are not covered by other tests, then it might be best to make DumpRenderTree-compatible copies of these tests and put them, e.g. in fast/css/counters.

Darin Adler

Comment 5 2008-02-29 09:53:33 PST

Comment on attachment 19384 [details] remove setTimeout and update expected results Seems good. r=me

mitz

Comment 6 2008-02-29 11:41:28 PST

Assigning to myself so that nobody lands the patch before I discuss my comments with Darin :-)

mitz

Comment 7 2008-02-29 20:13:44 PST

Comment on attachment 19384 [details] remove setTimeout and update expected results Setting to r- now. This bug should be addressed by creating copies of the tests and modifying them to work with DumpRenderTree using waitUntilDone and notifyDone. The updated patch should ideally include expected pixel results as well.

Tony Chang

Comment 8 2008-03-03 15:46:46 PST

Created attachment 19505 [details] fork files and use waitUntilDone/notifyDone This patch forks the test files (svn cp) and uses waitUntilDone/notifyDone. However, I don't have expected results because it looks like this produces the wrong output. The setTimeout causes the wrong output to be displayed. The same is true of the existing tests in LayoutTests/css2.1, I just didn't notice before. It looks like this is a bug in WebKit.

Eric Seidel (no email)

Comment 9 2008-04-09 12:03:11 PDT

(In reply to comment #7) > (From update of attachment 19384 [details] [edit]) > Setting to r- now. > > This bug should be addressed by creating copies of the tests and modifying them > to work with DumpRenderTree using waitUntilDone and notifyDone. > > The updated patch should ideally include expected pixel results as well. Odd. We've modified W3C tests in the pass to do things like this. I don't remember which set of tests though. I don't have any problems with modifying their tests. It makes updating to the latest versions of the tests more tricky, but it's silly to have broken versions of the tests checked into LayoutTests.

Bradley Meck

Comment 10 2008-05-21 09:54:09 PDT

Created attachment 21270 [details] Patch for the 2 affected tests After going through all the test I found that only these 2 tests used settimout in the onload event and have altered them accordingly, unfortunately saving the files in windows changed the newlines hence the duplication.

mitz

Comment 11 2008-05-21 12:22:57 PDT

Comment on attachment 21270 [details] Patch for the 2 affected tests I am guessing this needs to be reviewed, but it would really help if you could make a diff that showed only what's changed.

Darin Adler

Comment 12 2008-05-24 22:37:19 PDT

Comment on attachment 21270 [details] Patch for the 2 affected tests r=me

Darin Adler

Comment 13 2008-06-08 12:50:01 PDT

Comment on attachment 21270 [details] Patch for the 2 affected tests Once I applied the patch and put these diffs into a form I could read, I could tell they were incorrect. I was wrong to say r=me before! The meta-refresh-vs-open.html change was only adding a call to notifyDone inside the onload handler. That's not going to fix anything in the success case, since the onload handler only runs if the test fails. The http/tests/navigation/onload-navigation-iframe-timeout.html was no change at all, only a whitespace tweak.

Carol Szabo

Comment 14 2009-11-20 08:29:33 PST

I believe that there is a deeper problem in the CSS counters code than the waiting for the tests to finish. Currently there is a big fixme in CounterNode.cpp that acknowledges that the counters are not set up to handle changes to the DOM tree correctly. A fix for this is being discussed as part of bug 11031. Also I have been previously instructed by Darin to have modified CSS 2.1 tests (just as those for this bug put in fast/css/counters instead of css2.1)

Alexey Proskuryakov

Comment 15 2009-11-20 12:44:15 PST

I also think it's OK to modify css2.1 tests as long as only the harness is affected, and not anything in what the tests check for. It's just confusing to have tests that fail for wrong reasons.

Tony Chang

Comment 16 2010-01-20 16:01:35 PST

Carol offered to take over this bug.

Carol Szabo

Comment 17 2010-01-21 08:44:20 PST

I will be working slowly on fixing these tests. Those that should work now, but I am doing this in my dead time while working on other tasks so it will take a while. If anyone wants to do this quickly feel free to pick up the task.

Alexey Proskuryakov

Comment 18 2010-01-21 08:54:19 PST

See also: bug 31311. It sounds like a waste to fix the obsolete version of test suite - on the other hand, I do not know if the "new" version is mature enough to replace it.

Shinichiro Hamaji

Comment 19 2010-01-21 18:28:05 PST

Created attachment 47168 [details] Create copy in fast/css/counters

Shinichiro Hamaji

Comment 20 2010-01-21 18:29:27 PST

It seems the "current" version of the six tests in question have almost no differences from the copies we have (only some meta tags changed). So, I think we can modify these tests if we want. We've already had 5 copies of these tests in fast/css/counters and we only need to add one more test. So, this patch does - Add README.txt into LayoutTests/css2.1 . This was suggested by Darin in Bug 33942. - Disable the 6 tests in LayoutTests/css2.1 as they are invalid with DumpRenderTree. - Add counter-reset-001.html to LayoutTests/fast/css . This is modified version of t1204-reset-01-c-o.html . - Rename t1204-* in LayoutTests/fast/css to counter-* . It seems the recent version of W3C's CSS2.1 test suite uses this naming. - Remove unnecessary expectation files. I hope this fix makes sense.

Darin Adler

Comment 21 2010-01-21 23:24:31 PST

Comment on attachment 47168 [details] Create copy in fast/css/counters Seems fine. rs=me

Shinichiro Hamaji

Comment 22 2010-01-22 06:29:13 PST

Comment on attachment 47168 [details] Create copy in fast/css/counters Clearing flags on attachment: 47168 Committed r53701: <http://trac.webkit.org/changeset/53701>

Shinichiro Hamaji

Comment 23 2010-01-22 06:29:25 PST

All reviewed patches have been landed. Closing bug.

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