Bug 18265 - @charset rule after the first byte causes the rest of css to be ignored
Summary: @charset rule after the first byte causes the rest of css to be ignored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords: HasReduction
: 23744 28821 28888 29046 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-01 17:01 PDT by jasneet
Modified: 2011-04-04 07:01 PDT (History)
10 users (show)

See Also:


Attachments
reduction (334 bytes, text/html)
2008-04-01 17:01 PDT, jasneet
no flags Details
further reduction (120 bytes, text/html)
2008-08-20 23:13 PDT, Robert Blaut
no flags Details
Patch v1 (4.43 KB, patch)
2009-10-16 12:10 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch v1 (4.41 KB, patch)
2009-10-16 12:13 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch v1 (9.39 KB, patch)
2009-10-16 17:17 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch v1 (9.37 KB, patch)
2009-10-16 17:18 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-04-01 17:01:21 PDT
I Steps:
Go to 
the attached reduction

II Issue:
The div is not showing blue background

III Conclusion:
comment and the @charset ".." causes the rest of the css to be ignored.

IV Other browsers:
IE7: ok
FF3: ok
Opera9.24: ok

V Nightly tested: 31446
Comment 1 jasneet 2008-04-01 17:01:44 PDT
Created attachment 20278 [details]
reduction
Comment 2 Alexey Proskuryakov 2008-05-31 04:48:47 PDT
Confirmed with r34277. The stylesheet is of course invalid, but that doesn't mean that we should have a different behavior than other browsers.
Comment 3 Robert Blaut 2008-08-20 23:13:45 PDT
Created attachment 22917 [details]
further reduction
Comment 4 Robert Blaut 2008-08-20 23:16:33 PDT
Variant of the bug is covered by one test http://samples.msdn.microsoft.com/csstestpages/Chapter_4/Rules/at-rule-005.htm of the Microsoft CSS 2.1 Test Suite http://samples.msdn.microsoft.com/csstestpages/
Comment 5 Alexey Proskuryakov 2009-04-08 01:58:26 PDT
See also: bug 23744 (misplaced @comment in external CSS).
Comment 6 Alexey Proskuryakov 2009-09-02 11:31:27 PDT
*** Bug 28888 has been marked as a duplicate of this bug. ***
Comment 7 Viatcheslav Ostapenko 2009-09-09 06:57:58 PDT
*** Bug 28821 has been marked as a duplicate of this bug. ***
Comment 8 Adam Barth 2009-10-16 11:06:26 PDT
Assigned for investigation.
Comment 9 Adam Barth 2009-10-16 12:07:53 PDT
*** Bug 23744 has been marked as a duplicate of this bug. ***
Comment 10 Adam Barth 2009-10-16 12:10:42 PDT
Created attachment 41305 [details]
Patch v1
Comment 11 Adam Barth 2009-10-16 12:12:19 PDT
Please review this patch carefully.  I don't actually have any idea what I'm doing.
Comment 12 Adam Barth 2009-10-16 12:13:31 PDT
Created attachment 41306 [details]
Patch v1
Comment 13 Alexey Proskuryakov 2009-10-16 14:53:49 PDT
Comment on attachment 41306 [details]
Patch v1

Just a few nitpicks:

> +        @charset rule causes the rest of inline css to be ignored

You've marked a bug talking about non-inline case as duplicate - could you please update the title, and add tests for external CSS case?

Is any whitespace allowed by other browsers, or just a single linefeed after <style>? If it's the latter, perhaps HTML parser should consume the linefeed instead.

> +++ b/LayoutTests/fast/css/comment-before-charset-expected.txt

How did render tree dumps end up outside platform/mac? They should come with pixel results - but even better, these tests should be text-only (you can use getComputedStyle, or check document.styleSheets).

Looks good to me otherwise.
Comment 14 Adam Barth 2009-10-16 16:08:32 PDT
(In reply to comment #13)
> You've marked a bug talking about non-inline case as duplicate - could you
> please update the title, and add tests for external CSS case?

Done and will do.

> Is any whitespace allowed by other browsers, or just a single linefeed after
> <style>? If it's the latter, perhaps HTML parser should consume the linefeed
> instead.

I'll add more tests for these cases and verify that their results match other browsers.

> > +++ b/LayoutTests/fast/css/comment-before-charset-expected.txt
> 
> How did render tree dumps end up outside platform/mac?

I moved them.  I've never written a non-text test before, so I probably screwed that up.

> They should come with
> pixel results - but even better, these tests should be text-only (you can use
> getComputedStyle, or check document.styleSheets).

Will do.
Comment 15 Adam Barth 2009-10-16 17:17:39 PDT
Created attachment 41341 [details]
Patch v1
Comment 16 Adam Barth 2009-10-16 17:18:46 PDT
Created attachment 41342 [details]
Patch v1
Comment 17 Alexey Proskuryakov 2009-10-16 17:54:07 PDT
Comment on attachment 41342 [details]
Patch v1

Sorry, I now re-read the spec, and it says that we shouldn't allow the whitespace - we should ignore any @charset rule that isn't at the very beginning (except for possibly a BOM).

"User agents must ignore any @charset rule not at the beginning of the style sheet."

The difference is that an incorrect charset specified in a @charset rule will be ignored, and a default will be used (in most cases, the default will be main page encoding). I do not know if this ever happens in practice, but we did see examples of pages where a charset incorrectly specified in HTML head needed to be ignored.

r=me, because this patch is clearly an improvement. If you're not too tired of this, you can consider matching the spec though.
Comment 18 Adam Barth 2009-10-16 17:59:54 PDT
Comment on attachment 41342 [details]
Patch v1

I'm inclined to commit this change and file another bug for ignoring the charset because that change seems like a lower priority than fixing this issue.
Comment 19 Adam Barth 2009-10-16 18:16:17 PDT
Comment on attachment 41342 [details]
Patch v1

Builders are red and I need to head home for the day.  I'll land this tonight.
Comment 20 Adam Barth 2009-10-16 20:11:15 PDT
Comment on attachment 41342 [details]
Patch v1

Clearing flags on attachment: 41342

Committed r49727: <http://trac.webkit.org/changeset/49727>
Comment 21 Adam Barth 2009-10-16 20:11:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Jungshik Shin 2009-10-21 16:56:25 PDT
*** Bug 29046 has been marked as a duplicate of this bug. ***