Bug 6891

Confirmed, adding testcase in a sec.

Created attachment 6052 [details] Testcase

The mentioned Mozilla bug has a number of additional test cases for CSS EOF handling.

Created attachment 47315 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet.

Hi, The above patch requires that for https://bugs.webkit.org/show_bug.cgi?id=34067 . Yuzo

Comment on attachment 47315 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet. Looks sane to me. This can't be commit-queue'd because it fails to apply. :( Do FF and IE pass these tests? If not, then we will need to re-consider this change.

This patch is applicable only after the patch for 34067 is landed. Test results: IE 8.0: FAIL FF 3.5: FAIL Opera 10.10: FAIL Hmm, perhaps the patch would make WebKit too lenient?

Comment on attachment 47315 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet. Being too lenient has been known to cause compatibility bugs for us, too! r-, at least we until have a good explanation of why we should diverge.

(In reply to comment #7) > This patch is applicable only after the patch for 34067 is landed. > > Test results: > IE 8.0: FAIL > FF 3.5: FAIL > Opera 10.10: FAIL > > Hmm, perhaps the patch would make WebKit too lenient? I mean you checked the following test case: https://bugs.webkit.org/attachment.cgi?id=6052 ? My test results for it: - Firefox 3.6 - PASS - Opera 10.50 - PASS - Webkit r53677 - FAIL

Created attachment 47508 [details] Testcase with additional test In import-css-newline.html, unclosed import is followed by a newline then by </style>.

Hi, Robert, I noticed that the behavior changes depending on whether a newline follows the unclosed import or not. I've added import-css-newline.html to the testcase. The result (import-css.html, import-css-newline.html): IE 8.0: (FAIL, FAIL) FF 3.6: (PASS, FAIL) Opera 10.10: (PASS, PASS) WebKit r53468: (FAIL, FAIL) This patch: (FAIL, PASS) I admit this patch is not good enough.

Yuzo in standards mode the test case with new line should match nothing. The code: <style>@import "import-css.css </style> is the same as <style>@import "import-css.css\";</style> Firefox 3.6, Opera 10.50 renders this case correctly - no style sheet is imported. In quirks mode Opera is much more forgiving in this case and ignores new line in path. So Opera imports the stylesheet but only in quirks mode. IMO WebKit should behaves as Firefox 3.6 in this case in standards and also in quirks mode. The second test case without new line is correct and exposes bug in WebKit. Firefox, Opera import stylesheet. So also WebKit should import the stylesheet in standards and also in quirks mode.

I've changed the patch for 34067. The new result (import-css.html, import-css-newline.html): IE 8.0: (FAIL, FAIL) FF 3.6: (PASS, FAIL) Opera 10.10: (PASS, PASS) WebKit r53468: (FAIL, FAIL) This patch: (PASS, FAIL) Now this patch shows the same behavior as that of FF 3.6.

(In reply to comment #13) > I've changed the patch for 34067. Did you upload the new patchset for review? > > The new result (import-css.html, import-css-newline.html): > > IE 8.0: (FAIL, FAIL) > FF 3.6: (PASS, FAIL) > Opera 10.10: (PASS, PASS) > WebKit r53468: (FAIL, FAIL) > This patch: (PASS, FAIL) > > Now this patch shows the same behavior as that of FF 3.6. What about quirks vs standards mode?

Created attachment 47808 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet.

Created attachment 47809 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet.

Created attachment 47810 [details] One piece testcase

Reviewers, I've updated the patch. Can you take another look? Note: This still depends on 34067. This patch will make WebKit's behavior more compliant to http://www.w3.org/TR/CSS21/syndata.html#parsing-errors . Firefox behaves the same as far as for the tests below. Test results: (Test 1, Test 2, Test 3, Test 4, Test 5, Test 6) IE 8.0, Opera 10.10: (FAIL, FAIL, FAIL, FAIL, PASS, PASS) This patch, FF 3.6: All PASS WebKit r53468: All FAIL The tests are in the patch. For your convenience, they are also attached above as "One piece testcase". Yuzo

Comment on attachment 47809 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet. OK. Looks fine.

Comment on attachment 47809 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet. Actually, can't set cq+ yet.

Attachment 47809 [details] was posted by a committer and has review+, assigning to Yuzo Fujishima for commit.

Comment on attachment 47809 [details] Make @import work even when the file url/path is unclosed at the end of stylesheet. This change can't be commit-queue'd because it does not apply. Click any of the purple EWS bubbles to see the svn-apply log.

This has been blocked by 34067 for more than a month. That's why the patch doesn't apply cleanly.

huh, i just felt this on my own back. forgot ';' at the end of line and all webkit browsers fail to import css file.

We need a new patch posted here.

Created attachment 78510 [details] Pre Source/WebCore move

I'm releasing the ownership of this bug.

Attachment 78510 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7385120

Attachment 78510 [details] did not build on qt: Build output: http://queues.webkit.org/results/7391146

Attachment 78510 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7468085

Attachment 78510 [details] did not build on mac: Build output: http://queues.webkit.org/results/7445121

Attachment 78510 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7399125

Comment on attachment 78510 [details] Pre Source/WebCore move I'm not planning to review this in the future but the current patch is obviously wrong in that it breaks almost every build so r-.

Fixed by http://trac.webkit.org/changeset/111132