Closed Bug 677302 Opened 11 years ago Closed 6 years ago

Support putting UA CSS style sheet rules behind a boolean preferences

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1267890

People

(Reporter: jwalker, Unassigned, NeedInfo)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Example use:

@-moz-pref(slimline.feature.enabled) {
  .blah: margin-left: 2px;
}

.blah: margin-left: 6px;

Alternatives:
Currently it seems that the best option is to have JS reflect pref state in a parent node, and have 'preffed out CSS' include the reflected pref state in some way.

Pros:
The option above (along the lines of @-moz-document) could make it cleaner to develop features that are preffed out.

Cons:
Is this too much of a point solution - one that only applies to a limitted set of cases?
I like the idea, it would be useful in front-end code.

Perhaps obviously, it would need to be chrome-only to prevent sniffing/fingerprinting user prefs by untrusted content. Or the "pref" namespace could just be separate from the UA prefs... document.setStylePref() or Cc[xxx].getService(Ci.nsICSSFoo).setStylePref(). Details. :)
We can pretty easily allow the syntax in chrome sheets only, fwiw.
Version: unspecified → Trunk
Component: DOM: Mozilla Extensions → Style System (CSS)
QA Contact: general → style-system
This implements the following syntax:
  '@-moz-preference' '(' <pref-name> ',' <pref-value> ')' '{' <rule>* '}'
where
   pref-name: <string>
   pref-value: [ false | true | <string> | <integer> ]

when pref-value is false/true use Preferences::GetBool to lookup the value,
<string> use Preferences::GetString and <integer> use Preferences::GetInt.

I made @-moz-preference("pref", false) special in the sense that
it matches also when "pref" does not exist, is this what we want?

Currently there are no restrictions where @-moz-preference can be used...
This doesn't look like it handles dynamic changes, but otherwise seems reasonable.
(In reply to Mats Palmgren [:mats] from comment #3)
> I made @-moz-preference("pref", false) special in the sense that
> it matches also when "pref" does not exist, is this what we want?

I can't properly answer for 'we', but I can generalize from me: Yes.

The primary use for me is in enabling features which are preffed off, which frequently means that the pref does not exist.


I can see there are times when non-dynamic behaviour is what is wanted. It could be reasonable to be non-dynamic in JS about a pref to turn on a feature that is intended to be on permanently.

However in this case it's perhaps reasonable to have the instructions 'add this pref and then immediately restart if you don't want things to look wierd'.
This is hot like a fire.

(In reply to Mats Palmgren [:mats] from comment #3)
> Currently there are no restrictions where @-moz-preference can be used...

If that's so, then I think, dolske's point in comment 1 bears repeating: we should only match against prefs in an otherwise-empty branch (devtools.css.*? devtools.mozpref.*? css.mozpref.*?) so that it can't be used for content-space pref-sniffing.

Also this is a hot like a fire.
(Adding Curtis and Jesse as cc's to watch for the gotchas because I love this idea very much and don't want it to bite us)
Attached patch wip2 (obsolete) — Splinter Review
(In reply to David Baron [:dbaron] from comment #4)
> This doesn't look like it handles dynamic changes, ...

Right, dynamic changes are hard to do effeciently with the
@-moz-preference rule approach.  Fortunately all that hard work has
already been done for @media queries!  So I changed it to a @media query
expression instead.  Then we get effecient dynamic updates for free,
plus you can write more powerful expressions.

This patch implements a -moz-preference expression syntax like so:
'(' '-moz-preference' <pref-name> ':' <pref-value> ')'
  where pref-name: <string>
        pref-value: [ false | true | <string> | <integer> ]

and you can put that in a @media rule anywhere an expression is allowed
(http://www.w3.org/TR/css3-mediaqueries/), for example:

@media print and (-moz-preference "print.print_color" : true) {
  ...
}

In this patch I've restricted -moz-preference to agent/user/override
sheets.  In other sheets a -moz-preference expressions does not match
regardless of the pref value.  It's still parsed though, so you can have
@media (-moz-preference "print.print_color" : true) , screen {
  ...
}
in a document sheet and it would match for screen media.
Dynamic changes shouldn't be any easier or harder to handle either way; I think we should decide on the syntax based on which makes more sense.
Would it make sense for -moz-preference to work as a value in addition to a selector? Then we could fix bug 648218 without insane JS.

tabbrowser-tab:not([pinned]) { 
  max-width: -moz-preference("browser.tabs.tabMaxWidth");
}
(In reply to Jesse Ruderman from comment #10)
> Would it make sense for -moz-preference to work as a value in addition to a
> selector?

This sounds like a separate feature.
Blocks: 597564
Blocks: 649671
Blocks: 681317
No longer blocks: 597564
sec review triage = need to sched a full review
Whiteboard: [secr:curtisk]
Curtis: As far as I know the plan is to limit this to chrome stylesheets only, per comment 2, so I don't see why this would need privacy/security review.
Gavin: I don't pretend to understand why the team makes some of the decisions they do, but multiple members wanted to do a review. They generally do have concerns over things that touch chrome. If it is really simple and they see no immediate interest the review will be very short.
That said, if you could pick a date (https://mail.mozilla.com/home/ckoenig@mozilla.com/Security Review.html) that works for the needed knowledgeable people that would be appreciated.
I looked at the parts of the patch that mention AllowMozPreference. Looks like the parsing code isn't constrained to agent sheets. Why not, and how much attack surface does that add?

What is nsStyleSet::eOverrideSheet?

What are the implications of handling (or not handling) dynamic changes to prefs? Could a pref change occur at a time when it is not safe to do layout?

Will this feature make it more difficult to parallelize layout? IIRC prefs are main-thread only, and they're special in e10s (https://developer.mozilla.org/en/Multi-Process_Architecture/Multi_Process_Preference_System ?).

I'd like this patch to have tests, including tests to ensure web page content can't use the feature.
The patch is a WIP :)
This is awesome.  If it's restricted to chrome code (comment 2), there's no privacy risk at all.  Exposing this to content sounds kind of freaky unless we restrict it to a known empty subtree that is clearly for public consumption (comment 6, to avoid sniffing things like browser.privatebrowsing.autostart, services.sync.*, add-on prefs, etc).
Dao: why did you remove the privacy-review-needed keyword?  Are the plans to make this only available to chrome-level code?  I had been waiting to clear the keyword since this feature is still WIP and may change (comment 16).
(In reply to Sid Stamm [:geekboy] from comment #18)
> Dao: why did you remove the privacy-review-needed keyword?  Are the plans to
> make this only available to chrome-level code?

Yes, that's the one and only plan.
mats, bz, and I just had a quick discussion about this.

We want to do @-moz-preference rather than a media query, but it may be helpful to reuse nsMediaQueryResultCacheKey (etc.) mechanisms for helping with the dynamic change handling.

We definitely want to allow this in UA sheets and in chrome sheets.  User sheets is an open question, but perhaps leaning towards allowing.
Whiteboard: [secr:curtisk] → [sec-assigned:curtisk:749363]
Flags: sec-review?(curtisk)
Whiteboard: [sec-assigned:curtisk:749363]
Duplicate of this bug: 942075
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #20)
> We definitely want to allow this in UA sheets and in chrome sheets.  User
> sheets is an open question, but perhaps leaning towards allowing.

Let's make this only about UA style sheets for now, and deal with exposing the mechanism to user style sheets (and the associated security review) as a follow-up bug. We shouldn't delay making this available to UA sheets because we're not sure about the latter.
Summary: Preffing out CSS should be easier → Support putting UA CSS style sheet rules behind a boolean preferences
I tend to think an MQ expression is the better option, because you can combine
it with existing expressions for a richer query language.  That also allows
prefs to be used in the 'media' attribute of <link> / <style>, and in @import
etc for free.  MQ gives nice synergy effects that @-moz-preference won't have.

FYI, "IndieUI User Context" propose a new Media Query feature that is similar:
https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-context.html

There was a brief discussion on www-style about it (Review of IndieUI User Context):
http://lists.w3.org/Archives/Public/www-style/2013Nov/thread.html#msg182
Attached patch wip, media querySplinter Review
Here's an up-to-date trunk patch for the media query solution (as described
in comment 8), in case anyone wants play around with it.
Attachment #552826 - Attachment is obsolete: true
Attachment #552256 - Attachment description: wip → wip, @-moz-preference
Mats, are you planning to finish this, or should jwatt or somebody else take it over?
Flags: needinfo?(matspal)
David, as I understand it this stalled because your comment 20 is at odds with Mats's revised opinion expressed in comment 23. Can you review and come to some agreement with Mats so that this can move forward?
Flags: needinfo?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #23)
> I tend to think an MQ expression is the better option, because you can
> combine
> it with existing expressions for a richer query language.  That also allows
> prefs to be used in the 'media' attribute of <link> / <style>, and in @import
> etc for free.  MQ gives nice synergy effects that @-moz-preference won't
> have.
> 
> FYI, "IndieUI User Context" propose a new Media Query feature that is
> similar:
> https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-context.html
> 
> There was a brief discussion on www-style about it (Review of IndieUI User
> Context):
> http://lists.w3.org/Archives/Public/www-style/2013Nov/thread.html#msg182

We're getting close to a FPWD, but significantly reduced the reliance on media queries. 
https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-context.html

The primary interface is through an extension to the Window object that would allow you to request a setting such as:

  window.userSetting('user-font-size');

Vendor-specific preferences are also allowed, either for proposed standardization or for the scenario you're discussing above:

  window.userSetting('-moz-browser-tabs-tabMaxWidth');


There are still media features defined for some of the keys. Here's an example from the spec using 'user-font-size' to determine if layout should be single- or multi-column.

  /* Default layout uses 2 columns */
  main {
    columns: 2;
  }

  /* But if the user's default font size (from browser text zoom setting or... */
  /* user style sheet...) is larger than 32px, drop the columns. */
  /* Note: the CSS3 syntax is (min-user-font-size: 32) */
  /* Note: this example uses the greater-than/less-than syntax likely to be adopted by CSS4. */
  @media (user-font-size > 32) { 
    main {
      columns: auto;
    }
  }

Please provide feedback to public-indie-ui@w3.org. Thanks.
Bug 935803 comment 37 is another reason to want a fix here.

dbaron? Any thoughts on comment 26?
jwatt, feel free to take this; I'm probably not going to get to it anytime soon.
Flags: needinfo?(mats)
Flags: sec-review?(curtisk) → sec-review?
Yeah, comment 23 is reasonable.  But this got implemented in @supports in a separate bug, without reference to this one.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(dbaron)
Resolution: --- → DUPLICATE
Duplicate of bug: 1259889
Fantastic news! Thanks!
See Also: → 1267890
See Also: 1267890
Duplicate of bug: 1267890
Hey mats - how feasible do you think it'd be to dust off attachment 8337489 [details] [diff] [review] and rebase it on tip? The ability to pref on certain styles like this would be extremely helpful for the Photon project (especially as the theme-ing and animation teams would prefer to land things now rather than all at once when 57 hits central[1]).

Or jwatt, do you have the cycles for this one, along with the SVG work you're doing?

[1]: Yes, a build-time pref could also do the trick here, but having run-time pref support would help with the efforts going on in bug 1352069 as well.
Flags: needinfo?(mats)
Flags: needinfo?(jwatt)
(In reply to Mike Conley (:mconley) from comment #33)
> Or jwatt, do you have the cycles for this one, along with the SVG work
> you're doing?

Not right now, unfortunately. :(

(Also it seems bug 1352069 is fixed now. Is this still important for specific things you guys are working on?)
Flags: needinfo?(jwatt)
Flags: needinfo?(mconley)
(In reply to Jonathan Watt [:jwatt][back 18th May] from comment #34)
> 
> (Also it seems bug 1352069 is fixed now. Is this still important for
> specific things you guys are working on?)

I think it'd make it easier to do some of the larger scale theme-ing work that's still in the pipe for Photon. Right now, we're doing #ifdef's, meaning that it's kinda difficult to test both the "Photon" configuration and the "Default" configuration without doing a rebuild.
Flags: needinfo?(mconley)
No longer blocks: 681317
You need to log in before you can comment on or make changes to this bug.