Bug 67725 - css2.1/t090204-display-change-01-b-ao.html is not attaching event listener properly
Summary: css2.1/t090204-display-change-01-b-ao.html is not attaching event listener pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 67772
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-07 11:59 PDT by Ryosuke Niwa
Modified: 2011-09-08 03:35 PDT (History)
5 users (show)

See Also:


Attachments
fixes the test (24.43 KB, patch)
2011-09-07 12:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
swaps line-after-floating-div.html and t090204-display-change-01-b-ao.html (48.25 KB, patch)
2011-09-07 16:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the change log (47.49 KB, patch)
2011-09-07 16:07 PDT, Ryosuke Niwa
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-09-07 11:59:37 PDT
Per discussion with Dave, we found that css2.1/t090204-display-change-01-b-ao.html  is trying to attach load event listener on document instead of window. We should fix that.
Comment 1 Simon Fraser (smfr) 2011-09-07 12:02:02 PDT
This should be reported on public-css-testsuite@w3.org
Comment 2 Ryosuke Niwa 2011-09-07 12:02:58 PDT
Created attachment 106618 [details]
fixes the test
Comment 3 Ryosuke Niwa 2011-09-07 12:04:12 PDT
(In reply to comment #1)
> This should be reported on public-css-testsuite@w3.org

That's a good point but I'm not subscribed to that mailing list. Could you or someone else familiar with the matter do that on behalf of me?
Comment 4 Dave Hyatt 2011-09-07 12:09:20 PDT
Comment on attachment 106618 [details]
fixes the test

r=me
Comment 5 Eric Seidel (no email) 2011-09-07 12:10:08 PDT
http://www.w3.org/Bugs/Public/ might help :)
Comment 6 Ryosuke Niwa 2011-09-07 12:13:28 PDT
Committed r94696: <http://trac.webkit.org/changeset/94696>
Comment 7 Ryosuke Niwa 2011-09-07 12:15:12 PDT
(In reply to comment #5)
> http://www.w3.org/Bugs/Public/ might help :)

Ah, thanks for the pointer. Filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=14064.
Comment 8 mitz 2011-09-07 12:16:23 PDT
Is it no longer the policy to not modify third-party test suites?
Comment 9 Simon Fraser (smfr) 2011-09-07 12:17:58 PDT
I don't see this test in the the current 2.1 test suite: <http://test.csswg.org/suites/css2.1/20110111/html4/toc.html>
Comment 10 Ryosuke Niwa 2011-09-07 12:29:26 PDT
(In reply to comment #8)
> Is it no longer the policy to not modify third-party test suites?

(In reply to comment #9)
> I don't see this test in the the current 2.1 test suite: <http://test.csswg.org/suites/css2.1/20110111/html4/toc.html>

I am confused now. What is the correct solution then?
Comment 11 Simon Fraser (smfr) 2011-09-07 13:05:49 PDT
We have an old snapshot of the CSS 2.1 suite. We should import the (huge) current one.
Comment 12 mitz 2011-09-07 14:33:58 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Is it no longer the policy to not modify third-party test suites?
> 
> (In reply to comment #9)
> > I don't see this test in the the current 2.1 test suite: <http://test.csswg.org/suites/css2.1/20110111/html4/toc.html>
> 
> I am confused now. What is the correct solution then?

Unless the bug in the test is causing it to (a) crash or hang the test tool or (b) produce inconsistent results, I think the change should be reverted. If the bug in the test is causing it to be invalid or not cover what it is supposed to test, then the solution is to add a valid test somewhere outside the css2.1 directory.
Comment 13 Darin Adler 2011-09-07 15:36:40 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Is it no longer the policy to not modify third-party test suites?
> > 
> > (In reply to comment #9)
> > > I don't see this test in the the current 2.1 test suite: <http://test.csswg.org/suites/css2.1/20110111/html4/toc.html>
> > 
> > I am confused now. What is the correct solution then?
> 
> Unless the bug in the test is causing it to (a) crash or hang the test tool or (b) produce inconsistent results, I think the change should be reverted. If the bug in the test is causing it to be invalid or not cover what it is supposed to test, then the solution is to add a valid test somewhere outside the css2.1 directory.

That’s right.
Comment 14 Darin Adler 2011-09-07 15:36:54 PDT
(In reply to comment #11)
> We have an old snapshot of the CSS 2.1 suite. We should import the (huge) current one.

Also a great idea.
Comment 15 Ryosuke Niwa 2011-09-07 15:42:20 PDT
(In reply to comment #12)
> Unless the bug in the test is causing it to (a) crash or hang the test tool or (b) produce inconsistent results, I think the change should be reverted. If the bug in the test is causing it to be invalid or not cover what it is supposed to test, then the solution is to add a valid test somewhere outside the css2.1 directory.

Okay. Then let's revert this change and add a fixed test in fast/css.
Comment 16 Ryosuke Niwa 2011-09-07 15:45:06 PDT
i.e. I'm going to swap css2.1/t090204-display-change-01-b-ao.html and fast/css/line-after-floating-div.html.
Comment 17 Ryosuke Niwa 2011-09-07 16:06:10 PDT
Created attachment 106667 [details]
swaps line-after-floating-div.html and t090204-display-change-01-b-ao.html
Comment 18 Ryosuke Niwa 2011-09-07 16:06:31 PDT
Reopen the bug.
Comment 19 Ryosuke Niwa 2011-09-07 16:07:34 PDT
Created attachment 106668 [details]
Fixed the change log
Comment 20 Ryosuke Niwa 2011-09-07 16:11:23 PDT
Thanks for the timely review. Will land now.
Comment 21 Ryosuke Niwa 2011-09-07 16:15:05 PDT
Committed r94724: <http://trac.webkit.org/changeset/94724>