Bug 17557 - incorrect results for css2.1 layout tests that use setTimeout in body onload event
Summary: incorrect results for css2.1 layout tests that use setTimeout in body onload ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Carol Szabo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-26 13:40 PST by Tony Chang
Modified: 2010-01-29 05:03 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 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.
Comment 1 Tony Chang 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).
Comment 2 mitz 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.
Comment 3 Tony Chang 2008-02-26 14:02:56 PST
Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets run.
Comment 4 mitz 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.
Comment 5 Darin Adler 2008-02-29 09:53:33 PST
Comment on attachment 19384 [details]
remove setTimeout and update expected results

Seems good. r=me
Comment 6 mitz 2008-02-29 11:41:28 PST
Assigning to myself so that nobody lands the patch before I discuss my comments with Darin :-)
Comment 7 mitz 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.
Comment 8 Tony Chang 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.
Comment 9 Eric Seidel (no email) 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.

Comment 10 Bradley Meck 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.
Comment 11 mitz 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.
Comment 12 Darin Adler 2008-05-24 22:37:19 PDT
Comment on attachment 21270 [details]
Patch for the 2 affected tests

r=me
Comment 13 Darin Adler 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.
Comment 14 Carol Szabo 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)
Comment 15 Alexey Proskuryakov 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.
Comment 16 Tony Chang 2010-01-20 16:01:35 PST
Carol offered to take over this bug.
Comment 17 Carol Szabo 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.
Comment 18 Alexey Proskuryakov 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.
Comment 19 Shinichiro Hamaji 2010-01-21 18:28:05 PST
Created attachment 47168 [details]
Create copy in fast/css/counters
Comment 20 Shinichiro Hamaji 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.
Comment 21 Darin Adler 2010-01-21 23:24:31 PST
Comment on attachment 47168 [details]
Create copy in fast/css/counters

Seems fine. rs=me
Comment 22 Shinichiro Hamaji 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>
Comment 23 Shinichiro Hamaji 2010-01-22 06:29:25 PST
All reviewed patches have been landed.  Closing bug.