Bug 101527 – CSS charset parsing is too loose, doesn't match other browsers

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tab Atkins]

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=19882 , a bug on my CSS Syntax spec, Firefox and IE are both very strict and follow the CSS 2.1 spec *exactly* with regards to @charset parsing.

We, on the other hand, are much looser, and accept a lot of things that the others won't.

We should match FF and IE, because there's no good reason to be loose, it's unlikely there's much content that depends on our laxity, and matching the spec and other browsers makes it easier for web authors to figure out the right thing to do.

[Tab Atkins]

Created attachment 172914 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172914 [details]
Patch

Attachment 172914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14748879

New failing tests:
fast/encoding/css-charset-evil.html

[Build Bot]

Comment on attachment 172914 [details]
Patch

Attachment 172914 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14760611

New failing tests:
fast/encoding/css-charset-evil.html

[Tab Atkins]

Created attachment 173120 [details]
Patch

[Alexey Proskuryakov]

> it's unlikely there's much content that depends on our laxity

How did you arrive at this conclusion?

[Tab Atkins]

(In reply to comment #5)
> > it's unlikely there's much content that depends on our laxity
> 
> How did you arrive at this conclusion?

The fact that both IE and FF are strict about it, and very little content contains @charset in the first place.

[Alexey Proskuryakov]

> The fact that both IE and FF are strict about it

This does not carry a lot of weight any more. We certainly care about mobile content, which is not tested with these browsers. We deeply care about walled garden content, too.

> and very little content contains @charset in the first place.

I'm willing to buy this. I hoped that you had numbers to back this statement.

[Tab Atkins]

(In reply to comment #7)
> > The fact that both IE and FF are strict about it
> 
> This does not carry a lot of weight any more. We certainly care about mobile content, which is not tested with these browsers. We deeply care about walled garden content, too.
> 
> > and very little content contains @charset in the first place.
> 
> I'm willing to buy this. I hoped that you had numbers to back this statement.

Unfortunately I don't.

However, this only affects content which (a) uses @charset at all, and (b) has their CSS and HTML use different encodings, if (c) they happened to use one of the handful of incorrect ways of doing it that we recognized (wrong number of spaces on either side of the string, or using single quotes in the string).

(a) is vanishingly small in the first place (honestly, we could probably just throw out @charset altogether, default to utf-8, and suffer minimal breakage as a result), (b) is frankly pathological, and (c) can shave off a chunk of whatever tiny sliver might be left.

[Alexey Proskuryakov]

Comment on attachment 173120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173120&action=review

Oh, css-charset-evil.html says that both Firefox and IE used to pass. Especially with IE, the behavior of old versions is important.

I suggest going ahead with this one, but toning it down a little, so that people would not be confused about our rationale in case actual content regresses.

> Source/WebCore/ChangeLog:3
> +        Charset parsing is too loose, doesn't match other browsers

Please update ChangeLog to match new bug title. It's way too scary as is.

> Source/WebCore/ChangeLog:9
> +        Per <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19882#attach_1244>,
> +        IE and FF are strict about CSS @charset parsing.

Please verify that the added tests all pass at least in Firefox, and preferably in both browsers.

> Source/WebCore/ChangeLog:10
> +        We should match them and the spec.

Should mention that this is a recent behavior change in IE and Firefox.

I disagree with this rationale. An accurate one would be something like "since we think that @charset is very uncommon, we can as well match the new behavior for platform consistency".

> Source/WebCore/ChangeLog:17
> +        (WebCore):

prepare-ChangeLog generates these lines, but ChangeLogs are meant for human consumption, and should be edited by hand for best readability.

> LayoutTests/ChangeLog:15
> +        (#a1ä):

Somewhat more importantly, please remove these garbage lines.

[Tab Atkins]

Created attachment 173136 [details]
Patch

[Tab Atkins]

(In reply to comment #9)
> (From update of attachment 173120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173120&action=review
> 
> Oh, css-charset-evil.html says that both Firefox and IE used to pass. Especially with IE, the behavior of old versions is important.

Yeah, I'm not sure exactly when IE changed.  I just know that IE9 is strict, and presumably IE6 was loose.  If I could figure out how to determine when a file was added to the repo, I could establish a lower bound at least.  ^_^

> I suggest going ahead with this one, but toning it down a little, so that people would not be confused about our rationale in case actual content regresses.

Ok.  I've clarified the wording in both ChangeLogs to make it clearer that this is a recent change in FF and IE, and we expect that matching them is safe.

> > Source/WebCore/ChangeLog:3
> > +        Charset parsing is too loose, doesn't match other browsers
> 
> Please update ChangeLog to match new bug title. It's way too scary as is.

Done.

> > Source/WebCore/ChangeLog:9
> > +        Per <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19882#attach_1244>,
> > +        IE and FF are strict about CSS @charset parsing.
> 
> Please verify that the added tests all pass at least in Firefox, and preferably in both browsers.

I have verified that they pass in Firefox.  The W3C bug that I built this test from verified it for IE.

> > Source/WebCore/ChangeLog:10
> > +        We should match them and the spec.
> 
> Should mention that this is a recent behavior change in IE and Firefox.
> 
> I disagree with this rationale. An accurate one would be something like "since we think that @charset is very uncommon, we can as well match the new behavior for platform consistency".
> 
> > Source/WebCore/ChangeLog:17
> > +        (WebCore):
> 
> prepare-ChangeLog generates these lines, but ChangeLogs are meant for human consumption, and should be edited by hand for best readability.
>
> > LayoutTests/ChangeLog:15
> > +        (#a1ä):
> 
> Somewhat more importantly, please remove these garbage lines.

I didn't know that I was supposed to clean up that section of the ChangeLog.  Done!

[Alexey Proskuryakov]

Comment on attachment 173136 [details]
Patch

> Yeah, I'm not sure exactly when IE changed.  

I wonder if they still do the old thing in quirks mode.

> If I could figure out how to determine when a file was added to the repo, I could establish a lower bound at least.  ^_^

One of the ways is to go to <http://trac.webkit.org/browser/trunk/LayoutTests/fast/encoding/css-charset-evil.html>, and click "Revision Log". Alternatively, "svn log" on command line would do it.

6 years.

> I didn't know that I was supposed to clean up that section of the ChangeLog.  Done!

Most reviewers are very lax with this, I was just spiteful because my old fix was being undone ;). More seriously, the goal is to make reading patches and history as easy as possible, and removing garbage lines helps a little.

[WebKit Review Bot]

Comment on attachment 173136 [details]
Patch

Clearing flags on attachment: 173136

Committed r134518: <http://trac.webkit.org/changeset/134518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.