Closed Bug 1029867 Opened 6 years ago Closed 3 years ago

Fix or remove use of mHTMLMediaMode in nsCSSParser

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: johns, Assigned: emilio)

References

Details

Attachments

(1 file)

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)
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.
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.
Blocks: 1304792
Assignee: nobody → ecoal95
(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.
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.
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 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.
Attachment #8818189 - Flags: review?(cam) → review+
(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.
(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 :)
(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...
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce8eed27c43
Remove useless nsCSSParser::mHTMLMediaMode. r=heycam
https://hg.mozilla.org/mozilla-central/rev/7ce8eed27c43
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.