1029867 - Fix or remove use of mHTMLMediaMode in nsCSSParser

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Schoenick [:johns]]

The comment says "True for parsing media lists for HTML attributes, where we have to ignore CSS comments.", but the attribute is never read or honored. Per bz, we should make sure this isn't needed and remove it (or implement it)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So in https://hg.mozilla.org/mozilla-central/rev/fa6f776cfdf7 I didn't remove the XXXldb comment about skipping comments.  It appears to still be there.  The question is what the HTML spec and media queries spec say about whether it should be.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So the html spec defines the media attribute as containing a "Valid media query list", then:

> A string is a valid media query list if it matches the <media-query-list> production of the Media Queries specification.

Where there's no reference to HTML comments at all.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1029867: Remove useless nsCSSParser::mHTMLMediaMode.

Review commit: https://reviewboard.mozilla.org/r/98314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98314/

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> > A string is a valid media query list if it matches the <media-query-list> production of the Media Queries specification.
> 
> Where there's no reference to HTML comments at all.

FWIW, the issue would be about CSS comments, not HTML comments.  In particular, whether CSS comment syntax is expected to be honored inside of the media query syntax when used in HTML.

Comment 5

[Cameron McCormack (:heycam)]

http://mcc.id.au/temp/mq-comment.html shows that all major browsers do accept CSS comments.  While the HTML spec doesn't explicitly link to https://drafts.csswg.org/css-syntax/#parse-grammar I think it makes sense to interpret the "if it matches" wording to mean that.  That will end up consuming CSS tokens, which skips past CSS comments.

Comment 6

[Cameron McCormack (:heycam)]

Well, that test is for window.matchMedia() rather than media="", so maybe that is different, but I think we should be accepting comments per spec.

Comment 7

[Cameron McCormack (:heycam)]

Test for <link media>: http://mcc.id.au/temp/mq-comment-2.html

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8818189 [details]
Bug 1029867: Remove useless nsCSSParser::mHTMLMediaMode.

https://reviewboard.mozilla.org/r/98314/#review98532

::: layout/style/nsCSSParser.cpp:2139
(Diff revision 1)
>      // XXXldb We need to make the scanner not skip CSS comments!  (Or
>      // should we?)

Drop this comment too.

::: layout/style/nsCSSParser.cpp:18214
(Diff revision 1)
>  nsCSSParser::ParseSourceSizeList(const nsAString& aBuffer,
>                                   nsIURI* aURI,
>                                   uint32_t aLineNumber,
>                                   InfallibleTArray< nsAutoPtr<nsMediaQuery> >& aQueries,
>                                   InfallibleTArray<nsCSSValue>& aValues,
> -                                 bool aHTMLMode)
> +                                 )

Does this compile? :)  Move the paren to the previous line and drop the comma.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) from comment #4)
> FWIW, the issue would be about CSS comments, not HTML comments.  In
> particular, whether CSS comment syntax is expected to be honored inside of
> the media query syntax when used in HTML.

Right, sorry. Anyway, my point was that there's no distinction between parsing media queries in an HTML attribute spec-wise.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> Comment on attachment 8818189 [details]
> Bug 1029867: Remove useless nsCSSParser::mHTMLMediaMode.
> 
> https://reviewboard.mozilla.org/r/98314/#review98532
> 
> ::: layout/style/nsCSSParser.cpp:2139
> (Diff revision 1)
> >      // XXXldb We need to make the scanner not skip CSS comments!  (Or
> >      // should we?)
> 
> Drop this comment too.
> 
> ::: layout/style/nsCSSParser.cpp:18214
> (Diff revision 1)
> >  nsCSSParser::ParseSourceSizeList(const nsAString& aBuffer,
> >                                   nsIURI* aURI,
> >                                   uint32_t aLineNumber,
> >                                   InfallibleTArray< nsAutoPtr<nsMediaQuery> >& aQueries,
> >                                   InfallibleTArray<nsCSSValue>& aValues,
> > -                                 bool aHTMLMode)
> > +                                 )
> 
> Does this compile? :)  Move the paren to the previous line and drop the
> comma.

Gah, This patch lied in a big patch stack from stylo, and I applied that fixup to the wrong commit apparently :)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Gah, This patch lied in a big patch stack from stylo, and I applied that
> fixup to the wrong commit apparently :)

Actually, the patch was applied to the right commit, but mozreview submitted the pre-fixup commit.

I'll try to reproduce and file a git-mozreview bug, I guess...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce8eed27c43
Remove useless nsCSSParser::mHTMLMediaMode. r=heycam

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ce8eed27c43