Bug 67725

Summary:

css2.1/t090204-display-change-01-b-ao.html is not attaching event listener properly

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Tools / Tests

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric, hyatt, mitz, simon.fraser

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

67772

Bug Blocks:

Attachments:

Description	Flags
fixes the test	none
swaps line-after-floating-div.html and t090204-display-change-01-b-ao.html	none
Fixed the change log	mitz: review+

Ryosuke Niwa

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

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

Simon Fraser (smfr)

Comment 1 2011-09-07 12:02:02 PDT

This should be reported on public-css-testsuite@w3.org

Ryosuke Niwa

Comment 2 2011-09-07 12:02:58 PDT

Created attachment 106618 [details] fixes the test

Ryosuke Niwa

Comment 3 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?

Dave Hyatt

Comment 4 2011-09-07 12:09:20 PDT

Comment on attachment 106618 [details] fixes the test r=me

Eric Seidel (no email)

Comment 5 2011-09-07 12:10:08 PDT

http://www.w3.org/Bugs/Public/ might help :)

Ryosuke Niwa

Comment 6 2011-09-07 12:13:28 PDT

Committed r94696: <http://trac.webkit.org/changeset/94696>

Ryosuke Niwa

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

mitz

Comment 8 2011-09-07 12:16:23 PDT

Is it no longer the policy to not modify third-party test suites?

Simon Fraser (smfr)

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

Ryosuke Niwa

Comment 10 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?

Simon Fraser (smfr)

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

mitz

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

Darin Adler

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

Darin Adler

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

Ryosuke Niwa

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

Ryosuke Niwa

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

Ryosuke Niwa

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

Ryosuke Niwa

Comment 18 2011-09-07 16:06:31 PDT

Reopen the bug.

Ryosuke Niwa

Comment 19 2011-09-07 16:07:34 PDT

Created attachment 106668 [details] Fixed the change log

Ryosuke Niwa

Comment 20 2011-09-07 16:11:23 PDT

Thanks for the timely review. Will land now.

Ryosuke Niwa

Comment 21 2011-09-07 16:15:05 PDT

Committed r94724: <http://trac.webkit.org/changeset/94724>

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