Bug 17557 - incorrect results for css2.1 layout tests that use setTimeout in body onload event
: incorrect results for css2.1 layout tests that use setTimeout in body onload ...
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-02-26 13:40 PST by
Modified: 2010-01-29 05:03 PST (History)


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


Note

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


Description From 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 From 2008-02-26 13:42:11 PST -------
Created an attachment (id=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 From 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 From 2008-02-26 14:02:56 PST -------
Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets run.
------- Comment #4 From 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 From 2008-02-29 09:53:33 PST -------
(From update of attachment 19384 [details])
Seems good. r=me
------- Comment #6 From 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 From 2008-02-29 20:13:44 PST -------
(From update of attachment 19384 [details])
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 From 2008-03-03 15:46:46 PST -------
Created an attachment (id=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 From 2008-04-09 12:03:11 PST -------
(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 From 2008-05-21 09:54:09 PST -------
Created an attachment (id=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 From 2008-05-21 12:22:57 PST -------
(From update of attachment 21270 [details])
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 From 2008-05-24 22:37:19 PST -------
(From update of attachment 21270 [details])
r=me
------- Comment #13 From 2008-06-08 12:50:01 PST -------
(From update of attachment 21270 [details])
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 From 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 From 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 From 2010-01-20 16:01:35 PST -------
Carol offered to take over this bug.
------- Comment #17 From 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 From 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 From 2010-01-21 18:28:05 PST -------
Created an attachment (id=47168) [details]
Create copy in fast/css/counters
------- Comment #20 From 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 From 2010-01-21 23:24:31 PST -------
(From update of attachment 47168 [details])
Seems fine. rs=me
------- Comment #22 From 2010-01-22 06:29:13 PST -------
(From update of attachment 47168 [details])
Clearing flags on attachment: 47168

Committed r53701: <http://trac.webkit.org/changeset/53701>
------- Comment #23 From 2010-01-22 06:29:25 PST -------
All reviewed patches have been landed.  Closing bug.