Bug 6891 - end-of-file handling for @import is wrong
Summary: end-of-file handling for @import is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 416.x
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: https://bugzilla.mozilla.org/attachme...
Keywords: HasReduction
Depends on: 34067
Blocks: 47141
  Show dependency treegraph
 
Reported: 2006-01-28 12:15 PST by Anne van Kesteren
Modified: 2012-04-22 07:47 PDT (History)
17 users (show)

See Also:


Attachments
Testcase (461 bytes, application/zip)
2006-01-28 13:48 PST, Joost de Valk (AlthA)
no flags Details
Make @import work even when the file url/path is unclosed at the end of stylesheet. (4.59 KB, patch)
2010-01-24 23:12 PST, Yuzo Fujishima
ap: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Testcase with additional test (741 bytes, application/zip)
2010-01-27 01:20 PST, Yuzo Fujishima
no flags Details
Make @import work even when the file url/path is unclosed at the end of stylesheet. (13.88 KB, patch)
2010-01-31 20:00 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Make @import work even when the file url/path is unclosed at the end of stylesheet. (4.33 KB, patch)
2010-01-31 20:10 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
One piece testcase (1.65 KB, text/html)
2010-01-31 20:15 PST, Yuzo Fujishima
no flags Details
Pre Source/WebCore move (4.55 KB, patch)
2011-01-11 03:21 PST, Yuzo Fujishima
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2006-01-28 12:15:13 PST
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>)
Comment 1 Joost de Valk (AlthA) 2006-01-28 13:48:31 PST
Confirmed, adding testcase in a sec.
Comment 2 Joost de Valk (AlthA) 2006-01-28 13:48:57 PST
Created attachment 6052 [details]
Testcase
Comment 3 Alexey Proskuryakov 2007-01-06 03:33:17 PST
The mentioned Mozilla bug has a number of additional test cases for CSS EOF handling.
Comment 4 Yuzo Fujishima 2010-01-24 23:12:58 PST
Created attachment 47315 [details]
Make @import work even when the file url/path is unclosed at the end of stylesheet.
Comment 5 Yuzo Fujishima 2010-01-24 23:15:29 PST
Hi,

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

Yuzo
Comment 6 Eric Seidel (no email) 2010-01-26 14:14:54 PST
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.
Comment 7 Yuzo Fujishima 2010-01-26 22:38:56 PST
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 8 Alexey Proskuryakov 2010-01-26 23:06:07 PST
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.
Comment 9 Robert Blaut 2010-01-27 00:28:13 PST
(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
Comment 10 Yuzo Fujishima 2010-01-27 01:20:56 PST
Created attachment 47508 [details]
Testcase with additional test

In import-css-newline.html, unclosed import is followed by a newline then by </style>.
Comment 11 Yuzo Fujishima 2010-01-27 01:31:28 PST
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.
Comment 12 Robert Blaut 2010-01-27 05:11:18 PST
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.
Comment 13 Yuzo Fujishima 2010-01-28 04:14:30 PST
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.
Comment 14 Robert Blaut 2010-01-28 05:31:26 PST
(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?
Comment 15 Yuzo Fujishima 2010-01-31 20:00:44 PST
Created attachment 47808 [details]
Make @import work even when the file url/path is unclosed at the end of stylesheet.
Comment 16 Yuzo Fujishima 2010-01-31 20:10:56 PST
Created attachment 47809 [details]
Make @import work even when the file url/path is unclosed at the end of stylesheet.
Comment 17 Yuzo Fujishima 2010-01-31 20:15:14 PST
Created attachment 47810 [details]
One piece testcase
Comment 18 Yuzo Fujishima 2010-01-31 20:27:31 PST
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 19 Eric Seidel (no email) 2010-02-01 15:16:00 PST
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 20 Eric Seidel (no email) 2010-02-01 15:16:21 PST
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.
Comment 21 Eric Seidel (no email) 2010-02-08 11:21:09 PST
Attachment 47809 [details] was posted by a committer and has review+, assigning to Yuzo Fujishima for commit.
Comment 22 Eric Seidel (no email) 2010-03-05 17:15:18 PST
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.
Comment 23 Yuzo Fujishima 2010-03-08 00:31:37 PST
This has been blocked by 34067 for more than a month.
That's why the patch doesn't apply cleanly.
Comment 24 Rion 2010-07-09 05:23:46 PDT
huh, i just felt this on my own back.
forgot ';' at the end of line and all webkit browsers fail to import css file.
Comment 25 Eric Seidel (no email) 2010-09-02 13:57:28 PDT
We need a new patch posted here.
Comment 26 Yuzo Fujishima 2011-01-11 03:21:05 PST
Created attachment 78510 [details]
Pre Source/WebCore move
Comment 27 Yuzo Fujishima 2011-01-11 03:27:33 PST
I'm releasing the ownership of this bug.
Comment 28 WebKit Review Bot 2011-01-11 03:27:49 PST
Attachment 78510 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7385120
Comment 29 Early Warning System Bot 2011-01-11 03:33:31 PST
Attachment 78510 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7391146
Comment 30 WebKit Review Bot 2011-01-11 03:33:55 PST
Attachment 78510 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7468085
Comment 31 WebKit Review Bot 2011-01-11 04:24:17 PST
Attachment 78510 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7445121
Comment 32 WebKit Review Bot 2011-01-11 04:31:07 PST
Attachment 78510 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7399125
Comment 33 David Levin 2011-02-16 10:06:24 PST
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-.
Comment 35 Robert Hogan 2012-04-22 07:47:54 PDT
Fixed by http://trac.webkit.org/changeset/111132