pref'ing off properties doesn't pref off their will-change support
Categories
(Core :: CSS Parsing and Computation, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox62 | --- | fixed |
People
(Reporter: dbaron, Assigned: emilio)
References
Details
Attachments
(1 file)
https://dbaron.org/css/test/2018/stacking-context-z-order shows that: * `contain` support is preffed off * but we still support `will-change: contain` It seems like if we have support for a property pref'd off, we shouldn't be paying attention to the flags of what it does in 'will-change'.
|
Assignee | |
Comment 1•6 years ago
|
||
Will take some time to clean it up while at it.
| Comment hidden (mozreview-request) |
|
||
Comment 3•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8982463 [details] Bug 1466008: Make will-change honor prefs properly, and clean it up while at it. https://reviewboard.mozilla.org/r/248432/#review254636 ::: servo/components/style/properties/gecko.mako.rs:3520 (Diff revision 1) > - } > - > - bitfield as u8 > - } > - > self.gecko.mWillChangeBitField = 0; You can probably move this line into the `T::Auto` branch now. ::: servo/components/style/properties/gecko.mako.rs:3530 (Diff revision 1) > - Gecko_AppendWillChange(&mut self.gecko, feature.0.as_ptr()); > - } > - > - if let Ok(prop_id) = PropertyId::parse(&feature.0.to_string()) { > + Gecko_AppendWillChange( > + &mut self.gecko, > + feature.0.as_ptr(), > + ); It doesn't seem to me this should be broken into multiple lines... Actually, rustfmt would do the reverse, although file is not rustfmt-able. ::: servo/components/style/values/specified/box.rs:408 (Diff revision 1) > } > > +bitflags! { > + /// The change bits that we care about. > + /// > + /// These need to be in sync with NS_STYLE_WILL_CHANGE_*. Maybe we can use some static_assert technique like [1] to ensure this. [1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/servo/components/style/properties/gecko.mako.rs#1536-1548 ::: servo/components/style/values/specified/box.rs:447 (Diff revision 1) > + let id = match PropertyId::parse(ident) { > + Ok(id) => id, > + Err(..) => return WillChangeBits::empty(), > + }; > + > + if !id.allowed_in(context) { > + return WillChangeBits::empty(); > + } This makes me think we probably should have this check embeded into `PropertyId::parse`, so that every caller of it has to consider what set of properties they really want to accept. That should be better than patching each usage with an extra `allowed_in` after we find this kind of issues. Mind filing a followup bug? ::: servo/components/style/values/specified/box.rs:458 (Diff revision 1) > + let mut flags = WillChangeBits::empty(); > + for longhand in shorthand.longhands() { > + flags |= change_bits_for_longhand(longhand); > + } > + flags May look better this way? ```rust shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| { flags | change_bits_for_longhand(p) }) ```
|
|
Assignee | |
Comment 4•6 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8982463 [details] Bug 1466008: Make will-change honor prefs properly, and clean it up while at it. https://reviewboard.mozilla.org/r/248432/#review254636 > Maybe we can use some static_assert technique like [1] to ensure this. > > [1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/servo/components/style/properties/gecko.mako.rs#1536-1548 Maybe, though I'd need to enumerate the flags yet in another place... > This makes me think we probably should have this check embeded into `PropertyId::parse`, so that every caller of it has to consider what set of properties they really want to accept. That should be better than patching each usage with an extra `allowed_in` after we find this kind of issues. > > Mind filing a followup bug? So, I tried to do something like that before and didn't end up liking the result... The issue with that not all callers have a ParserContext available, and those should probably use enabled_for_all_content... Probably we can change the semantics to that, will look into it.
|
|
||
Comment 5•6 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8982463 [details] Bug 1466008: Make will-change honor prefs properly, and clean it up while at it. https://reviewboard.mozilla.org/r/248432/#review254636 > So, I tried to do something like that before and didn't end up liking the result... The issue with that not all callers have a ParserContext available, and those should probably use enabled_for_all_content... > > Probably we can change the semantics to that, will look into it. Probably we can rename the current `parse` to something like `parse_ignoring_enabled_state`, and have `parse` use it then check allowed. For the case you mentioned, maybe we can have a `parse_for_all_content` in addition.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4fa69beec8 Make will-change honor prefs properly, and clean it up while at it. r=xidorn
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/24447bd95fbd followup: set the rule type in the custom properties code, since we use it. r=me on a CLOSED TREE
|
||
Comment 8•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/ff4fa69beec8 https://hg.mozilla.org/mozilla-central/rev/24447bd95fbd
|
||
Comment 9•4 years ago
|
||
I was about to post a patch to remove the CSS Containment pref entirely (in bug 1626458), but I noticed this bug's reftest[1] depends on toggling that pref to false[2] (to be sure will-change still works).
If I were to remove the CSS Containment pref, I assume this test would stop testing the codepath/conditions that it's intending to test. So for now, we can't remove that pref. (which is sort of fine, but perhaps not the best thing long-term)
How should we address this moving forward? Should we keep CSS Containment preffable, for the sake of this reftest & its code-coverage? Should we add a dummy pref that we can use for tests like this? Or just get rid of the test and cross our fingers that it doesn't regress? (probably not that one :) ) Something else?
[1] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/layout/reftests/bugs/1466008.html
[2] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/layout/reftests/bugs/reftest.list#2054
|
|
||
Updated•4 years ago
|
|
Reporter | |
Comment 10•4 years ago
|
||
Just switch it to another pref-controlled property?
|
|
Assignee | |
Comment 11•4 years ago
|
||
Yeah, so, that reftest relies on the observable effects of contain creating a fixed-pos containing block.
Other properties which are preffed with the same effect are the individual transform properties (layout.css.individual-transform.enabled), or backdrop-filter (layout.css.backdrop-filter.enabled).
Using any of those instead would do.
|
|
||
Comment 12•4 years ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)
Yeah, so, that reftest relies on the observable effects of contain creating a fixed-pos containing block.
Other properties which are preffed with the same effect are the individual transform properties (
layout.css.individual-transform.enabled)
This pref shipped as default-enabled in Firefox 72 (bug 1424900), and now we're up to having released Firefox 74, so it's kind of in the same boat as contain in the sense that we're unlikely to un-ship it & we might as well remove its pref if possible. So, I'm hesitant to just kick the can over to this property.
or
backdrop-filter(layout.css.backdrop-filter.enabled).
That one might be a good fit, since it's complicated & will likely be webrender-dependent for the foreseeable future & hence we'll probably want to keep it preffable for a while.
Question: is it OK that the pref is off by default? (i.e. were you trying to exercise some condition where a default-enabled-property is dynamically preffed off, or is it sufficient for the property to simply be disabled, period [regardless of whether it's default-disabled or dynamically disabled]?)
Description
•