| Summary: | CSS 2.1 failure: at-import-* | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||
| Component: | CSS | Assignee: | Rob Buis <rwlbuis> | ||||||||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, ap, darin, eric, rniwa, rwlbuis, simon.fraser, zalan | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 47141 | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 74501 [details]
Patch
This fixes the specified tests, let me know if tests need to be added or whether the harness is enough. Cheers, Rob. Created attachment 74506 [details]
Added tests and really fixed 09
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? 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. Sounds good. I'd suggest contacting test suite maintainer about these tests - perhaps this has all been considered already. Discussion of the test suite happens on public-css-testsuite@w3.org. Created attachment 75313 [details]
Fix html4/at-import-010 test
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.
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. Well, if these tests pass in both IE and Firefox, then we should just proceed. Attachment 75313 [details] was posted by a committer and has review+, assigning to Rob Buis for commit.
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 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.
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! |
Created attachment 69748 [details] at-import-009.htm These tests fail: html4/at-import-009 fail html4/at-import-010 fail