Closed
Bug 1259889
Opened 9 years ago
Closed 9 years ago
Consider adding an internal-only syntax for @supports to detect pref
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: xidorn, Assigned: TYLin)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
MozReview Request: Bug 1259889 Part 1 - Add @supports -moz-bool-pref for internal-only style sheets.
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
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•9 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
Assignee | ||
Comment 1•9 years ago
|
||
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•9 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)
Assignee | ||
Comment 3•9 years ago
|
||
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•9 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•9 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 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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/
Assignee | ||
Comment 9•9 years ago
|
||
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!
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
xidorn & heycam, thank you for the prompt review! I hope I get close this time :)
Comment 13•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8737060 -
Flags: review?(cam) → review+
Comment 14•9 years ago
|
||
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
Comment 15•9 years ago
|
||
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•9 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•9 years ago
|
||
Alternatively, you can add this new syntax to html.css directly in part 2, so that the current code there makes sense.
Assignee | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Comment 19•9 years ago
|
||
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/
Assignee | ||
Comment 20•9 years ago
|
||
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/
Assignee | ||
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Comment 22•9 years ago
|
||
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/
Assignee | ||
Comment 23•9 years ago
|
||
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•9 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.
Assignee | ||
Comment 25•9 years ago
|
||
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.
Comment 26•9 years ago
|
||
You can add a pref just for testing to testing/profiles/prefs_general.js.
Assignee | ||
Comment 27•9 years ago
|
||
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/
Assignee | ||
Comment 28•9 years ago
|
||
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/
Assignee | ||
Comment 29•9 years ago
|
||
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.
Comment 30•9 years ago
|
||
Assignee | ||
Comment 31•9 years ago
|
||
heycam & xidorn, thank you for your help along the way. If there are other comments, I'll address them as a followup.
Comment 32•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6559f99e1805
https://hg.mozilla.org/mozilla-central/rev/30aaf3805b56
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 34•9 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•9 years ago
|
||
It is only available in user agent stylesheets. Probably it is actually testable via using nsIStyleSheetService.
Flags: needinfo?(tlin)
Reporter | ||
Comment 36•9 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•9 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•9 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•9 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.
Comment 40•9 years ago
|
||
:( 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•9 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•9 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•9 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.
Updated•8 years ago
|
Comment 46•6 years ago
|
||
This is now documented at:
- https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Chrome/CSS/-moz-bool-pref
- https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/48#Changes_for_add-on_and_Mozilla_developers
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•