Consider adding an internal-only syntax for @supports to detect pref

RESOLVED FIXED in Firefox 48

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: xidorn, Assigned: TYLin)

Tracking

({dev-doc-needed})

Trunk
mozilla48
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
As bug 1258657 comment 1 mentioned, adding rules to user agent style sheet could make some feature partially expose to the web unconditionally.

We probably need an internal-only syntax for @supports to guard those rules. That could probably be -moz-pref or something.
(Reporter)

Updated

2 years ago
Summary: Consider add an internal-only syntax for @supports to detect pref → Consider adding an internal-only syntax for @supports to detect pref
Xidorn, what syntax do you think of? Something like this?

@supports ( -moz-pref: "dom.details_element.enabled" ) {
  /* Evaluate rules only when "dom.details_element.enabled" is true */
}

I must admit this syntax look weird to me since @supports is for detecting CSS properties...
Flags: needinfo?(quanxunzhen)
(Reporter)

Comment 2

2 years ago
I think it should be something like
> @supports -moz-pref(details_element) {
>   /* rules */
> }

Checking the pref directly could affect performance, I suppose. Not sure how often ua stylesheets are parsed. If that happens only once, then accepting random pref string could be fine as well.

The spec has reserved this form for future extension. [1] As far as this is an internal-only syntax, I think it is fine. Don't forget to use "AgentRulesEnabled()" guard the parsing. [2]


[1] https://drafts.csswg.org/css-conditional-3/#general_enclosed
[2] https://dxr.mozilla.org/mozilla-central/rev/494289c72ba3997183e7b5beaca3e0447ecaf96d/layout/style/nsCSSParser.cpp#347
Flags: needinfo?(quanxunzhen)
Created attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

This is a internal-only syntax for guarding rules from a boolean
preference. This is needed for rendering the disclosure triangle of the
summary element by using "display: list-item".

Usage example:
@supports -moz-bool-pref("dom.details_element.enabled") {
  /* css rules */
}

Review commit: https://reviewboard.mozilla.org/r/43545/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43545/
Attachment #8736772 - Flags: review?(cam)
(Reporter)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/43545/#review40205

::: layout/style/nsCSSParser.cpp:4532
(Diff revision 1)
> +  if (ParseSupportsMozBoolPref(aConditionMet)) {
> +    return true;
> +  }

I'd suggest you move this after the check against eCSSToken_URL, so that you don't need to UngetToken() repeatedly inside ParseSupportsMozBoolPref.

::: layout/style/nsCSSParser.cpp:4616
(Diff revision 1)
> +  }
> +
> +  aConditionMet = Preferences::GetBool(ToNewUTF8String(mToken.mIdent));
> +
> +  if (!ExpectSymbol(')', true)) {
> +    UngetToken();

Given there is an unpaired '(' parsed (as part of the function token), I think there should be a SkipUntil(')') here rather than an UngetToken().
(Reporter)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/43545/#review40209

::: layout/style/nsCSSParser.cpp:4613
(Diff revision 1)
> +  if (mToken.mType != eCSSToken_String) {
> +    UngetToken();
> +    return false;
> +  }
> +
> +  aConditionMet = Preferences::GetBool(ToNewUTF8String(mToken.mIdent));

This looks like a memory leak.

You should probably use "NS_ConvertUTF16toUTF8(mToken.mIdent).get()" instead of "ToNewUTF8String".

(Actually, I think converting to ASCII is enough, but this isn't a hot path and converting to UTF-8 is certainly safer.)
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

https://reviewboard.mozilla.org/r/43545/#review40207

Please mention in the commit message that nothing causes the @supports rule to be re-evaluated when prefs change.

::: layout/style/nsCSSParser.cpp:4532
(Diff revision 1)
> +  if (ParseSupportsMozBoolPref(aConditionMet)) {
> +    return true;
> +  }

There are two ways that ParseSupportsMozBoolPref can fail: either it fails because the next token isn't a "-moz-bool-pref(" function token, and so we want to try the other things allowed in supports_condition_in_parens, or we failed in the middle of parsing it because for example we didn't get a string token.

This is assuming you want a malformed -moz-bool-pref() expression to cause the @supports to fail (just like a malformed (property: value) expression would).  And I think that's the right thing to do.

So I think instead we need to confirm here in ParseSupportsConditionInParens that the next token is "-moz-bool-pref(" and only then call ParseSupportsMozBoolPref.  Then when we can just return ParseSupportsMozBoolPref's return value.

::: layout/style/nsCSSParser.cpp:4580
(Diff revision 1)
> +// general_enclosed
> +//   : -moz-bool-pref '(' boolean_pref_name ')'
> +//   ;

Since general_enclosed in the spec is meant to represent "any balanced content inside parens that we don't understand and want to evaluate to false", I think we should instead say something like:

// supports_condition_pref
//   : -moz-bool-pref(' boolean_pref_name ')'
//   ;

and then up above ParseSupportsConditionInParens have:

// supports_condition_in_parens
//   : '(' S\* supports_condition_in_parens_inside_parens ')' S\*
//   | supports_condition_pref
//   | general_enclosed
//   ;

::: layout/style/nsCSSParser.cpp:4586
(Diff revision 1)
> +  if (!AgentRulesEnabled()) {
> +    return false;
> +  }

The AgentRulesEnabled check then probably makes more sense at the point where you will initially check for "-moz-bool-pref(" up in ParseSupportsConditionInParens.

::: layout/style/nsCSSParser.cpp:4594
(Diff revision 1)
> +  if (mToken.mType != eCSSToken_Function) {
> +    UngetToken();
> +    return false;
> +  }
> +
> +  if (!mToken.mIdent.LowerCaseEqualsLiteral("-moz-bool-pref")) {
> +    UngetToken();
> +    return false;
> +  }

Nit: I would just combine these two.

::: layout/style/nsCSSParser.cpp:4608
(Diff revision 1)
> +  if (mToken.mType != eCSSToken_String) {
> +    UngetToken();
> +    return false;
> +  }

In here you need to call SkipUntil(')') so that when we indicate a parse error (by returning false) we move past the ')' that matches the '-moz-bool-pref('.  Otherwise we might skip past too much invalid content further up the stack when trying to recover from the overall @supports {} parse error.

::: layout/style/nsCSSParser.cpp:4613
(Diff revision 1)
> +  if (mToken.mType != eCSSToken_String) {
> +    UngetToken();
> +    return false;
> +  }
> +
> +  aConditionMet = Preferences::GetBool(ToNewUTF8String(mToken.mIdent));

ToNewUTF8String allocates a new char* buffer that you have to call free() on later, so instead I think you want:

Preferences::GetBool(NS_ConvertUTF16toUTF8(mToken.mIdent).get())

::: layout/style/nsCSSParser.cpp:4615
(Diff revision 1)
> +  if (!ExpectSymbol(')', true)) {
> +    UngetToken();
> +    return false;
> +  }

SkipUntil(')') in here too.
Attachment #8736772 - Flags: review?(cam)
Created attachment 8737060 [details]
MozReview Request: Bug 1259889 Part 2 - Load html.css lazily for pref changes.

We need to re-evaluate html.css whenever "dom.details_element.enabled"
is changed to make the disclosure triangle for summary elements show up.
Reftests for details and summary will need the a live pref to work.

Review commit: https://reviewboard.mozilla.org/r/43695/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43695/
Attachment #8736772 - Attachment description: MozReview Request: Bug 1259889 - Add @supports -moz-bool-pref. → MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.
Attachment #8737060 - Flags: review?(cam)
Attachment #8736772 - Flags: review?(cam)
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43545/diff/1-2/
https://reviewboard.mozilla.org/r/43545/#review40205

> I'd suggest you move this after the check against eCSSToken_URL, so that you don't need to UngetToken() repeatedly inside ParseSupportsMozBoolPref.

Mix both xidorn and heycam's idea by moving ParseSupportsMozBoolPref into the middle of the ParseSupportsConditionInParens().

> Given there is an unpaired '(' parsed (as part of the function token), I think there should be a SkipUntil(')') here rather than an UngetToken().

Yes. Fixed!
https://reviewboard.mozilla.org/r/43545/#review40209

> This looks like a memory leak.
> 
> You should probably use "NS_ConvertUTF16toUTF8(mToken.mIdent).get()" instead of "ToNewUTF8String".
> 
> (Actually, I think converting to ASCII is enough, but this isn't a hot path and converting to UTF-8 is certainly safer.)

Fixed the leak.
https://reviewboard.mozilla.org/r/43545/#review40207

I'll mention this in my patchset 2. However html.css needs to be re-evaulate whenever dom.details_element.enabled changed, so I'll do this in Part 2.

> There are two ways that ParseSupportsMozBoolPref can fail: either it fails because the next token isn't a "-moz-bool-pref(" function token, and so we want to try the other things allowed in supports_condition_in_parens, or we failed in the middle of parsing it because for example we didn't get a string token.
> 
> This is assuming you want a malformed -moz-bool-pref() expression to cause the @supports to fail (just like a malformed (property: value) expression would).  And I think that's the right thing to do.
> 
> So I think instead we need to confirm here in ParseSupportsConditionInParens that the next token is "-moz-bool-pref(" and only then call ParseSupportsMozBoolPref.  Then when we can just return ParseSupportsMozBoolPref's return value.

Fixed.

> Since general_enclosed in the spec is meant to represent "any balanced content inside parens that we don't understand and want to evaluate to false", I think we should instead say something like:
> 
> // supports_condition_pref
> //   : -moz-bool-pref(' boolean_pref_name ')'
> //   ;
> 
> and then up above ParseSupportsConditionInParens have:
> 
> // supports_condition_in_parens
> //   : '(' S\* supports_condition_in_parens_inside_parens ')' S\*
> //   | supports_condition_pref
> //   | general_enclosed
> //   ;

Fixed.

> The AgentRulesEnabled check then probably makes more sense at the point where you will initially check for "-moz-bool-pref(" up in ParseSupportsConditionInParens.

Yes. I move AgentRulesEnabled() into ParseSupportsConditionInParens().

> Nit: I would just combine these two.

Move thses two statements into ParseSupportsConditionInParens().

> ToNewUTF8String allocates a new char* buffer that you have to call free() on later, so instead I think you want:
> 
> Preferences::GetBool(NS_ConvertUTF16toUTF8(mToken.mIdent).get())

Fixed the leak.
xidorn & heycam, thank you for the prompt review! I hope I get close this time :)
Assignee: nobody → tlin
Blocks: 1258657
Status: NEW → ASSIGNED
See Also: bug 1258657
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

https://reviewboard.mozilla.org/r/43545/#review40249

::: layout/style/nsCSSParser.cpp:4603
(Diff revision 2)
> +  aConditionMet = Preferences::GetBool(
> +    NS_ConvertUTF16toUTF8(mToken.mIdent).get());
> +
> +  if (!ExpectSymbol(')', true)) {
> +    SkipUntil(')');
> +    aConditionMet = false;

I think we don't need to set aConditionMet to false when returning false.
Attachment #8736772 - Flags: review?(cam) → review+
Attachment #8737060 - Flags: review?(cam) → review+
Comment on attachment 8737060 [details]
MozReview Request: Bug 1259889 Part 2 - Load html.css lazily for pref changes.

https://reviewboard.mozilla.org/r/43695/#review40251
It would be good to have a test for @supports -moz-bool-pref() working as expected, and not being available in non-chrome sheets.
(Reporter)

Comment 16

2 years ago
I don't think you want to do part 2 in this bug, given you don't actually add anything to html.css yet.

When you add the stuff to html.css, you'd probably want to add a comment there saying there is some code in nsLayoutStylesheetCache.cpp which should be removed when the "@supports -moz-bool-pref()" in html.css gets removed.
(Reporter)

Comment 17

2 years ago
Alternatively, you can add this new syntax to html.css directly in part 2, so that the current code there makes sense.
https://reviewboard.mozilla.org/r/43545/#review40249

> I think we don't need to set aConditionMet to false when returning false.

OK. Will fixed in patchset 3.
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43545/diff/2-3/
Comment on attachment 8737060 [details]
MozReview Request: Bug 1259889 Part 2 - Load html.css lazily for pref changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43695/diff/1-2/
(In reply to Cameron McCormack (:heycam) from comment #15)
> It would be good to have a test for @supports -moz-bool-pref() working as
> expected, and not being available in non-chrome sheets.

OK. I'll add a test in part 1 for -moz-bool-pref not available in non-chrome sheet. For chrome sheet tests, details summary reftests should suffice if I add -moz-bool-pref to guard the existing rules in html.css like xidorn suggested in comment 17.
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43545/diff/3-4/
Comment on attachment 8737060 [details]
MozReview Request: Bug 1259889 Part 2 - Load html.css lazily for pref changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43695/diff/2-3/
(Reporter)

Comment 24

2 years ago
https://reviewboard.mozilla.org/r/43545/#review40273

::: layout/reftests/css-parsing/supports-moz-bool-pref.html:8
(Diff revision 4)
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->
> +
> +<html>
> +  <style>
> +  /* This is not a user agent style sheet, so the style will be ignored. */
> +  @supports -moz-bool-pref("dom.details_element.enabled") {

You probably should not use an existing pref. A pref specific for this test should be better. I suppose you can add one via prepending |test-pref(<name>,true)| in reftest.list.

Please test locally to ensure this test fails if you remove |AgentRulesEnabled()| in your code.
https://reviewboard.mozilla.org/r/43545/#review40273

> You probably should not use an existing pref. A pref specific for this test should be better. I suppose you can add one via prepending |test-pref(<name>,true)| in reftest.list.
> 
> Please test locally to ensure this test fails if you remove |AgentRulesEnabled()| in your code.

Looks like reftest doesn't allow arbitrary pref name to be set.  We might need a pref which is true and unlikely to be removed. ("dom.details_element.enabled" might be removed once it is shipping for a few release version.)

I write `test-pref(test.supports.moz-bool-pref,true) == supports-moz-bool-pref.html supports-moz-bool-pref-ref.html`, and got

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/tlin/Projects/gecko-dev/layout/reftests/css-parsing/supports-moz-bool-pref.html | boolean preference 'test.supports.moz-bool-pref' not known or wrong type.
You can add a pref just for testing to testing/profiles/prefs_general.js.
Comment on attachment 8736772 [details]
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43545/diff/4-5/
Comment on attachment 8737060 [details]
MozReview Request: Bug 1259889 Part 2 - Load html.css lazily for pref changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43695/diff/3-4/
https://reviewboard.mozilla.org/r/43545/#review40273

> Looks like reftest doesn't allow arbitrary pref name to be set.  We might need a pref which is true and unlikely to be removed. ("dom.details_element.enabled" might be removed once it is shipping for a few release version.)
> 
> I write `test-pref(test.supports.moz-bool-pref,true) == supports-moz-bool-pref.html supports-moz-bool-pref-ref.html`, and got
> 
> REFTEST TEST-UNEXPECTED-FAIL | file:///Users/tlin/Projects/gecko-dev/layout/reftests/css-parsing/supports-moz-bool-pref.html | boolean preference 'test.supports.moz-bool-pref' not known or wrong type.

testing/profiles/prefs_general.js seems for mochitest. To set a preference in reftest, I changed layout/tools/reftest/reftest-preferences.js. I have run my reftest locally, it does fail if I remove |AgentRulesEnabled()| in the parser.
heycam & xidorn, thank you for your help along the way. If there are other comments, I'll address them as a followup.

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6559f99e1805
https://hg.mozilla.org/mozilla-central/rev/30aaf3805b56
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 677302
Keywords: dev-doc-needed

Comment 34

2 years ago
I read through the patches here and although the patches say that this rule is limited to privileged contexts, it doesn't say what that means in practice. There is one test which appears to test that this rule is not effective in a nonprivileged context, but no test to make sure this works properly in a privileged context?

Is this tied to the origin of the CSS file (chrome://browser/content/browser.css) or the including page?
Flags: needinfo?(tlin)
(Reporter)

Comment 35

2 years ago
It is only available in user agent stylesheets. Probably it is actually testable via using nsIStyleSheetService.
Flags: needinfo?(tlin)
(Reporter)

Comment 36

2 years ago
TYLin, you may want to improve your test here to test that it is actually effective when it is loaded as a UA sheet. nsIStyleSheetService.preloadSheet looks like what you would need for testing that.

Comment 37

2 years ago
This was mentioned in the Firefox team meeting, but as I understand it we won't be able to use this in Firefox chrome because Firefox chrome stylesheets aren't UA stylesheets.
(Reporter)

Comment 38

2 years ago
That's right, chrome stylesheets aren't UA stylesheets. If you want this feature to be available in chrome code, the condition there needs to be changed. You could open a new bug for adding that support.

I'm a bit concerned about the potential performance impact. UA stylesheets are fine because they are only parsed once (if no related pref change) in the browser lifetime, but chrome stylesheets could be parsed more times (probably not too many either, though). Also invalidating parsed stylesheet might be challenging for chrome stylesheet as well I suspect.
(Reporter)

Comment 39

2 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #38)
> That's right, chrome stylesheets aren't UA stylesheets. If you want this
> feature to be available in chrome code, the condition there needs to be
> changed. You could open a new bug for adding that support.

I mean, the condition in the parsing code needs to be changed, not the @supports condition.
:( I'm sad that bug 677302, which has a fair amount of discussion about how useful this would be for front-end code, got duped over to this one but this didn't actually fix what we wanted it for. So should that be reopened?

Is the performance impact really likely to be significant? Is that fixable? I'd expect that we'd be using this for things where the pref is unlikely to change very often.
(Reporter)

Comment 41

2 years ago
The performance impact is, every time the stylesheet is parsed, it would need a pref lookup, which should generally not be very expensive. So if the stylesheets are not parsed so often, it is probably not a big deal. I guess our JS code probably has been doing more pref lookup.
(Reporter)

Comment 42

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #40)
> :( I'm sad that bug 677302, which has a fair amount of discussion about how
> useful this would be for front-end code, got duped over to this one but this
> didn't actually fix what we wanted it for. So should that be reopened?

Bug 677302 is also about UA sheets, right? I think you should just file a new bug for frontend usage and we can discuss there.
Its meaning was changed in bug 677302 comment 22.

When I dup'd to this bug, I was a bit disappointed that nobody in this bug had bothered to look for previous discussions of the issue, though.  (Bug 677302 had also concluded that a media query expression would be better.)

At this point, though, we'd probably be best off with clean new bugs for the remaining issues.  TYLin or xidorn, would you be able to file those followups?
Flags: needinfo?(tlin)
Flags: needinfo?(bugzilla)
(Reporter)

Comment 44

2 years ago
Read through the bug, I still believe that a parse-time pref switch fits UA sheets better, because UA sheets would be loaded and restyled frequently in all documents we load, and paying an additional pref check for them makes no sense. (And using @supports costs less code.)

Probably adding a media query for chrome uses make sense.
(Reporter)

Comment 45

2 years ago
Will do.
Flags: needinfo?(tlin)
Flags: needinfo?(bugzilla)
(Reporter)

Updated

2 years ago
See Also: → bug 1267890

Updated

10 months ago
Blocks: 1267890
See Also: bug 1267890
You need to log in before you can comment on or make changes to this bug.