Bug 18265 – @charset rule after the first byte causes the rest of css to be ignored

• Component: CSS
• Status: RESOLVED FIXED

[jasneet]

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

[jasneet]

Created attachment 20278 [details]
reduction

[Alexey Proskuryakov]

Confirmed with r34277. The stylesheet is of course invalid, but that doesn't mean that we should have a different behavior than other browsers.

[Robert Blaut]

Created attachment 22917 [details]
further reduction

[Robert Blaut]

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/

[Alexey Proskuryakov]

See also: bug 23744 (misplaced @comment in external CSS).

[Alexey Proskuryakov]

*** Bug 28888 has been marked as a duplicate of this bug. ***

[Viatcheslav Ostapenko]

*** Bug 28821 has been marked as a duplicate of this bug. ***

[Adam Barth]

Assigned for investigation.

[Adam Barth]

*** Bug 23744 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 41305 [details]
Patch v1

[Adam Barth]

Please review this patch carefully.  I don't actually have any idea what I'm doing.

[Adam Barth]

Created attachment 41306 [details]
Patch v1

[Alexey Proskuryakov]

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.

[Adam Barth]

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

[Adam Barth]

Created attachment 41341 [details]
Patch v1

[Adam Barth]

Created attachment 41342 [details]
Patch v1

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 41342 [details]
Patch v1

Builders are red and I need to head home for the day.  I'll land this tonight.

[Adam Barth]

Comment on attachment 41342 [details]
Patch v1

Clearing flags on attachment: 41342

Committed r49727: <http://trac.webkit.org/changeset/49727>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Jungshik Shin]

*** Bug 29046 has been marked as a duplicate of this bug. ***