Bug 47153 – CSS 2.1 failure: at-import-*

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Simon Fraser (smfr)]

Created attachment 69748 [details]
at-import-009.htm

These tests fail:

html4/at-import-009	fail
html4/at-import-010	fail

[Rob Buis]

Created attachment 74501 [details]
Patch

[Rob Buis]

This fixes the specified tests, let me know if tests need to be added or whether the harness is enough.
Cheers,

Rob.

[Rob Buis]

Created attachment 74506 [details]
Added tests and really fixed 09

[Alexey Proskuryakov]

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?

[Rob Buis]

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.

[Alexey Proskuryakov]

Sounds good. I'd suggest contacting test suite maintainer about these tests - perhaps this has all been considered already.

[Simon Fraser (smfr)]

Discussion of the test suite happens on [e-mail].

[Rob Buis]

Created attachment 75313 [details]
Fix html4/at-import-010 test

[Alexey Proskuryakov]

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.

[Rob Buis]

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 [e-mail]?
Cheers,

Rob.

[Alexey Proskuryakov]

Well, if these tests pass in both IE and Firefox, then we should just proceed.

[Eric Seidel (no email)]

Attachment 75313 [details] was posted by a committer and has review+, assigning to Rob Buis for commit.

[Rob Buis]

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.

[Darin Adler]

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.

[Ahmad Saleem]

Created attachment 460293 [details]
Safari 15.5 matches other browsers

I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 based on the following test case:

Test - http://test.csswg.org/suites/css21_dev/20110323/html4/at-import-010.htm

From Comment 05, it seems that 009 was questionable and the later bug was focused on fixing alt-import-010 but didn't move due to test being valid as per Comment 14.

Since now all browsers as shown in the picture are working as expected and showing "Green" text, I think this can be marked as "RESOLVED INVALID" or "RESOLVED CONFIGURATION CHANGED". Depending on, whether it was web-spec change, which aligned behavior or something along the line got it fixed. Thanks!