end-of-file handling for @import is wrong

• Component: CSS
• Status: RESOLVED FIXED

[Anne van Kesteren]

See <http://www.w3.org/TR/CSS21/syndata.html#parsing-errors>

For:

# <style> @import "test</style>

... a file "test" should be imported.

(See also <https://bugzilla.mozilla.org/show_bug.cgi?id=325064>)

[Joost de Valk (AlthA)]

Confirmed, adding testcase in a sec.

[Joost de Valk (AlthA)]

Created attachment 6052 [details]
Testcase

[Alexey Proskuryakov]

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

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Hi,

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

Yuzo

[Eric Seidel (no email)]

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.

[Yuzo Fujishima]

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?

[Alexey Proskuryakov]

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.

[Robert Blaut]

(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

[Yuzo Fujishima]

Created attachment 47508 [details]
Testcase with additional test

In import-css-newline.html, unclosed import is followed by a newline then by </style>.

[Yuzo Fujishima]

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.

[Robert Blaut]

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.

[Yuzo Fujishima]

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.

[Robert Blaut]

(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?

[Yuzo Fujishima]

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

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Created attachment 47810 [details]
One piece testcase

[Yuzo Fujishima]

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

[Eric Seidel (no email)]

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

OK.  Looks fine.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Yuzo Fujishima]

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

[Rion]

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

[Eric Seidel (no email)]

We need a new patch posted here.

[Yuzo Fujishima]

Created attachment 78510 [details]
Pre Source/WebCore move

[Yuzo Fujishima]

I'm releasing the ownership of this bug.

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[David Levin]

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

[Robert Hogan]

http://test.csswg.org/suites/css2.1/20110323/html4/eof-005.htm

[Robert Hogan]

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