Bug 47153 - CSS 2.1 failure: at-import-*
Summary: CSS 2.1 failure: at-import-*
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks: 47141
  Show dependency treegraph
 
Reported: 2010-10-04 21:43 PDT by Simon Fraser (smfr)
Modified: 2011-06-18 11:52 PDT (History)
4 users (show)

See Also:


Attachments
at-import-009.htm (758 bytes, text/html)
2010-10-04 21:43 PDT, Simon Fraser (smfr)
no flags Details
Patch (2.70 KB, patch)
2010-11-21 06:10 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Added tests and really fixed 09 (19.28 KB, patch)
2010-11-21 08:55 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Fix html4/at-import-010 test (10.69 KB, patch)
2010-12-01 13:21 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-10-04 21:43:46 PDT
Created attachment 69748 [details]
at-import-009.htm

These tests fail:

html4/at-import-009	fail
html4/at-import-010	fail
Comment 1 Rob Buis 2010-11-21 06:10:26 PST
Created attachment 74501 [details]
Patch
Comment 2 Rob Buis 2010-11-21 06:11:16 PST
This fixes the specified tests, let me know if tests need to be added or whether the harness is enough.
Cheers,

Rob.
Comment 3 Rob Buis 2010-11-21 08:55:02 PST
Created attachment 74506 [details]
Added tests and really fixed 09
Comment 4 Alexey Proskuryakov 2010-11-22 11:38:16 PST
Comment on attachment 74506 [details]
Added tests and really fixed 09

View in context: https://bugs.webkit.org/attachment.cgi?id=74506&action=review

These tests are strange. Somehow, a syntax requirement of @import being the first rule (besides @charset) can be waived by semantical correctness of preceding rules - I wouldn't expect it to work that way.

r=me, but please don't land without confirming that IE and Firefox agree.

> WebCore/ChangeLog:8
> +        Fix at-import-009.htm by ignoring invalid @media and @charset rules.

Valid or not, @charset is allowed before @import. Could you add an explanation of what the problem with that was?
Comment 5 Rob Buis 2010-12-01 10:14:14 PST
Hi Alexey,

Sorry to react so late, very busy with other things lately.

(In reply to comment #4)
> (From update of attachment 74506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74506&action=review
> 
> These tests are strange. Somehow, a syntax requirement of @import being the first rule (besides @charset) can be waived by semantical correctness of preceding rules - I wouldn't expect it to work that way.
> 
> r=me, but please don't land without confirming that IE and Firefox agree.

Indeed at-import-009.htm works in FF but fails in IE. at-import-010.htm works as expected
in both (text shows up green). If at-import-009.htm as a test is questionable, should I redo the
patch to only fix at-import-010.htm?
Cheers,

Rob.
Comment 6 Alexey Proskuryakov 2010-12-01 11:04:14 PST
Sounds good. I'd suggest contacting test suite maintainer about these tests - perhaps this has all been considered already.
Comment 7 Simon Fraser (smfr) 2010-12-01 11:47:42 PST
Discussion of the test suite happens on public-css-testsuite@w3.org.
Comment 8 Rob Buis 2010-12-01 13:21:49 PST
Created attachment 75313 [details]
Fix html4/at-import-010 test
Comment 9 Alexey Proskuryakov 2010-12-01 13:53:53 PST
Comment on attachment 75313 [details]
Fix html4/at-import-010 test

Somehow, I expected this test to be more controversial, because ":unknownpseudo { background: red; }" seems "more valid" than "@page". Weird.

This test doesn't contain any page rules, but you modify CSSParser::createPageRule().

r=me assuming skipping invalid @page rules matches both IE and Firefox. It's unfortunate to land this without a test.
Comment 10 Rob Buis 2010-12-06 13:13:23 PST
Hello Alexey, Simon,

(In reply to comment #9)
> (From update of attachment 75313 [details])
> Somehow, I expected this test to be more controversial, because ":unknownpseudo { background: red; }" seems "more valid" than "@page". Weird.
> 
> This test doesn't contain any page rules, but you modify CSSParser::createPageRule().
> 
> r=me assuming skipping invalid @page rules matches both IE and Firefox. It's unfortunate to land this without a test.

I decided to skip the @page bits as it was not needed for fixing html4/at-import-010. That part of the patch is now in webkit repository.

I noticed later that at-import-009.htm does actually work in IE! I can only guess that I forgot to create the 
support/import-green.css on my windows XP intially, but again after a later retry it worked. So should I create a new patch for at-import-009.htm fixing or should I first ask about its validity on public-css-testsuite@w3.org?
Cheers,

Rob.
Comment 11 Alexey Proskuryakov 2010-12-06 13:31:06 PST
Well, if these tests pass in both IE and Firefox, then we should just proceed.
Comment 12 Eric Seidel (no email) 2010-12-14 15:13:44 PST
Attachment 75313 [details] was posted by a committer and has review+, assigning to Rob Buis for commit.
Comment 13 Rob Buis 2010-12-14 23:05:03 PST
Hi Eric,

(In reply to comment #12)
> Attachment 75313 [details] was posted by a committer and has review+, assigning to Rob Buis for commit.

Sorry for my tardiness here, for one thing I was travelling lately. The other, more serious thing, is that
last week before landing I decided to recheck and actually found FF4 on OS X failing the test! So maybe we have to wait landing this and clear up validness of the testcase on the css tests mailing list?
Cheers,

Rob.
Comment 14 Darin Adler 2011-06-18 11:52:31 PDT
Comment on attachment 75313 [details]
Fix html4/at-import-010 test

Clearing review flag while we wait to hear if the test is valid or not.