We have (preffed-off) CSS parser support for "contain", based on the spec as it stood a few years ago (from bug 1170173) Morgan is going to update our implementation to support the new keywords that have been added to the spec since that time. I believe our existing support handles: none | strict | [ layout || style || paint ] ...and now we need to add "size" as well as "content" (which is equivalent to "layout style paint"). https://drafts.csswg.org/css-contain/#contain-property

Updated servo/components/style/values/specified/box.rs to have support for parsing Size as a Contain property. Right now, size is the 0xC bitflag, but I'm not sure if that makes the most sense; 'strict' which implicitly should set all the properties to be true has its own bitflag which is 0x8 (less than 0xC). I haven't looked around yet, but I think swapping these two might make more intuitive sense so the flags go in order.

Yeah, we probably want to increase all the bit values and insert "size" at the beginning, to match the ordering in the spec: https://drafts.csswg.org/css-contain/#contain-property (Be sure to update STRICT_BITS to include "size", a few lines down, and add a "maybe_write_value" line for it in ToCSS below that, and add it to the "impl Parse" chunk below that.) And then, probably in a second patch (or even a separate bug if you like), you'll want to add support for "contain: content" (which is a shorthand value kind of like "strict"). For that one, I suspect you can crib from the existing code about "strict" around there, but just with a different set of bits.

Based on review flags, I think Morgan intended for the first two patches to be up for review, but the 3rd patch was posted by accident. (I think Morgan updated the review requests in Bugzilla, but sadly that doesn't make a difference to MozReview -- MozReview is the Source Of Truth on reviews, and information only flows in one direction -- from mozreview to bugzilla. So we need to fix the review flags on MozReview, not on bugzilla.) I'm going to cancel review for the 3rd patch to be sure emilio doesn't accidentally start reviewing it, and set the review flag for the first two to mirror what I think Morgan's intentions were. :)

Comment on attachment 8980769 [details] Bug 1463589 - Implement contain:size and contain:content parse functionality. https://reviewboard.mozilla.org/r/246942/#review253114 Looks good, mostly nits and the pref change that can't really sneak in :) Given the patch on its own doesn't compile because of the NS_STYLE_CONTAIN_SIZE bits, so please either fix that up, or squash both before landing them (I don't particularly mind either way). Thanks for working on this! :) ::: layout/style/nsComputedDOMStyle.cpp:5051 (Diff revision 1) > } else if (mask & NS_STYLE_CONTAIN_STRICT) { > NS_ASSERTION(mask == (NS_STYLE_CONTAIN_STRICT | NS_STYLE_CONTAIN_ALL_BITS), > "contain: strict should imply contain: layout style paint"); > val->SetIdent(eCSSKeyword_strict); > - } else { > + } else if (mask & NS_STYLE_CONTAIN_CONTENT) { > + NS_ASSERTION(mask == (NS_STYLE_CONTAIN_CONTENT | NS_STYLE_CONTAIN_CONTENT_BITS), nit: trailing whitespace. ::: layout/style/nsStyleConsts.h:459 (Diff revision 1) > #endif > }; > > // See nsStyleDisplay > // If these are re-ordered, nsComputedDOMStyle::DoGetContain() and > // nsCSSValue::AppendToString() must be updated. While you're touching this, feel free to remove the mention of nsCSSValue::AppendToString, which no longer exists. ::: layout/style/nsStyleConsts.h:470 (Diff revision 1) > +#define NS_STYLE_CONTAIN_CONTENT 0x20 > // NS_STYLE_CONTAIN_ALL_BITS does not correspond to a keyword. > #define NS_STYLE_CONTAIN_ALL_BITS (NS_STYLE_CONTAIN_LAYOUT | \ > NS_STYLE_CONTAIN_STYLE | \ > + NS_STYLE_CONTAIN_PAINT | \ > + NS_STYLE_CONTAIN_SIZE) I don't know how this builds: NS_STYLE_CONTAIN_SIZE is not defined, is it? ::: modules/libpref/init/all.js:2931 (Diff revision 1) > > // Is support for CSS overflow-clip-box enabled for non-UA sheets? > pref("layout.css.overflow-clip-box.enabled", false); > > // Is support for CSS contain enabled? > -pref("layout.css.contain.enabled", false); > +pref("layout.css.contain.enabled", true); This cannot land (yet :P) ::: servo/components/style/properties/gecko.mako.rs:3679 (Diff revision 1) > use properties::longhands::contain::{self, SpecifiedValue}; > > let mut servo_flags = contain::computed_value::T::empty(); > let gecko_flags = self.gecko.mContain; > > if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 && I think this can just be `if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0` (that is, no need for the `&&` condition). I think ALL_BITS should be guaranteed to be present when `STRICT` is, shouldn't it? Maybe file a followup, or just change it here to: ``` if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 { debug_assert_eq!( gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8), NS_STYLE_CONTAIN_ALL_BITS as u8, "When strict is specified, ALL_BITS should be specified as well" ); // ... } ``` ::: servo/components/style/properties/gecko.mako.rs:3685 (Diff revision 1) > gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8) != 0 { > servo_flags.insert(SpecifiedValue::STRICT | SpecifiedValue::STRICT_BITS); > return servo_flags; > } > + if gecko_flags & (NS_STYLE_CONTAIN_CONTENT as u8) != 0 && > + gecko_flags & (NS_STYLE_CONTAIN_CONTENT_BITS as u8) != 0 { Same comment here, checking `CONTENT_BITS` against zero should be unnecessary. ::: servo/components/style/values/specified/box.rs:540 (Diff revision 1) > /// `paint` variant, turns on paint containment > - const PAINT = 0x04; > + const PAINT = 0x08; > /// `strict` variant, turns on all types of containment > - const STRICT = 0x8; > + const STRICT = 0x10; > + /// 'content' variant, turns on style, layout, and paint containment > + const CONTENT = 0x20; Hmm, so we have CONTENT and CONTENT_BITS (and ALL and ALL_BITS) so that we can preserve whether it was specified using the `content` keyword or not, and thus to avoid serializing `contain: layout style paint` as `contain: content`. Chrome seems to preserve it in the specified value: ``` document.body.style.contain = "paint layout style" > "paint layout style" document.body.style.contain > "paint layout style" ``` But not in the computed value: ``` getComputedStyle(document.body).contain > "content" ``` I think per the shortest serialization principle (see [1]) we could consider simplifying this setup and removing STRICT / CONTENT's separate bits from the value. Then in the serialization code we could just: ``` if self.contains(Contain::ALL_BITS) { return dest.write_str("strict"); } if self.contains(Contain::CONTENT_BITS) { return dest.write_str("content"); } ``` Not sure if you think the simplification is worth it or not, but if you think it is, mind filing a followup to simplify this? It doesn't need to block this, making `content` do the same as `strict` for now sounds fine, let's change both if we think it's worth the simplification. [1]: https://github.com/w3c/csswg-drafts/issues/1564

Comment on attachment 8980770 [details] Bug 1463589 - Add contain:size parse functionality. https://reviewboard.mozilla.org/r/246944/#review253116 This looks fine, with the nits and the issues of the previous patch addressed. Thanks! ::: layout/style/nsComputedDOMStyle.cpp:5058 (Diff revision 1) > val->SetIdent(eCSSKeyword_content); > } else { > nsAutoString valueStr; > > nsStyleUtil::AppendBitmaskCSSValue(nsCSSProps::kContainKTable, > - mask, NS_STYLE_CONTAIN_LAYOUT, > + mask, nit: whitespace.

Comment on attachment 8980769 [details] Bug 1463589 - Implement contain:size and contain:content parse functionality. https://reviewboard.mozilla.org/r/246942/#review253114 > I don't know how this builds: NS_STYLE_CONTAIN_SIZE is not defined, is it? I squashed the two together, so this should be fine now. I ran the mochitests locally and everything looks good. > Hmm, so we have CONTENT and CONTENT_BITS (and ALL and ALL_BITS) so that we can preserve whether it was specified using the `content` keyword or not, and thus to avoid serializing `contain: layout style paint` as `contain: content`. > > Chrome seems to preserve it in the specified value: > > ``` > document.body.style.contain = "paint layout style" > > "paint layout style" > document.body.style.contain > > "paint layout style" > ``` > > But not in the computed value: > > ``` > getComputedStyle(document.body).contain > > "content" > ``` > > I think per the shortest serialization principle (see [1]) we could consider simplifying this setup and removing STRICT / CONTENT's separate bits from the value. > > Then in the serialization code we could just: > > ``` > if self.contains(Contain::ALL_BITS) { > return dest.write_str("strict"); > } > if self.contains(Contain::CONTENT_BITS) { > return dest.write_str("content"); > } > ``` > > Not sure if you think the simplification is worth it or not, but if you think it is, mind filing a followup to simplify this? It doesn't need to block this, making `content` do the same as `strict` for now sounds fine, let's change both if we think it's worth the simplification. > > [1]: https://github.com/w3c/csswg-drafts/issues/1564 I think this is probably worth doing. Follow-up bug posted here: https://bugzilla.mozilla.org/show_bug.cgi?id=1465133

Comment on attachment 8980769 [details] Bug 1463589 - Implement contain:size and contain:content parse functionality. https://reviewboard.mozilla.org/r/246942/#review253704 Code-wise looks great, thank you! But I need to admit I'm confused about the reftests: How does contain parsing affect the outcome of those reftests? If it does not, are the reftests useful? r=me removing the reftests, and the other nits below. Feel free to ask Daniel to skim over it once that's done to land it, or re-request review, at your will :) ::: .lldbinit:16 (Diff revision 2) > # topsrcdir appropriately.) > script topsrcdir = topsrcdir if locals().has_key("topsrcdir") else os.getcwd(); sys.path.append(os.path.join(topsrcdir, "third_party/python/lldbutils")); import lldbutils; lldbutils.init() > > # Mozilla's use of UNIFIED_SOURCES to include multiple source files into a > # single compiled file breaks lldb breakpoint setting. This works around that. > -# See http://lldb.llvm.org/troubleshooting.html for more info. > +# See http://lldb.llvm.org/troubleshooting.html for more. nit: This change needs to be reverted. ::: layout/reftests/w3c-css/submitted/contain/contain-bad-parse-001.html:14 (Diff revision 2) > + <style> > + body { > + margin: 0; > + } > + .root { > + contain: nothing; What is this test testing, exactly? This test would produce the same output regardless of your change, right? In general a reftest is not super-useful to test parsing of properties. The changes to property_database.js that you have made should be enough. If you want to test that something doesn't parse, for example, you could create an automated test using the testharness API[1] that would look like: ``` <script> test(function() { let element = document.createElement('div'); element.setAttribute("contains: foobiedoobie"); assert_equals(element.style.length, 0, "foobiedoobie is not a valid contain value"); }, "Containg parsing with invalid value"); </script> ``` Though that's what a lot of property_database.js tests do under the hood (and more), so I don't think it's necessary in this case. [1]: https://web-platform-tests.org/writing-tests/testharness-api.html ::: layout/reftests/w3c-css/submitted/contain/contain-content-parse-001-ref.html:14 (Diff revision 2) > + margin: 0; > + } > + #a { > + width: 100px; > + height: 100px; > + background: red; nit: Just for future reference, since I think this test should be removed as well: Generally red is used to indicate failure in a reftest, so it's somewhat weird to find it as a reference. ::: layout/style/test/test_value_computation.html:171 (Diff revision 2) > // This means that it's important that we set the prereqs on > // gRule1.style rather than on gElement.style. > gRule2.style.setProperty(property, val, ""); > var val_computed_n = get_computed_value(getComputedStyle(gElementN, ""), property); > var val_computed_f = get_computed_value(getComputedStyle(gElementF, ""), property); > + nit: This change shouldn't be in the patch either. ::: modules/libpref/init/all.js:2931 (Diff revision 2) > > // Is support for CSS overflow-clip-box enabled for non-UA sheets? > pref("layout.css.overflow-clip-box.enabled", false); > > // Is support for CSS contain enabled? > -pref("layout.css.contain.enabled", false); > +pref("layout.css.contain.enabled", true); nit: This change shouldn't be in the patch in the first place.

Comment on attachment 8981526 [details] Bug 1463589 - Adjust whitespace and condition checking for strict/content bits. https://reviewboard.mozilla.org/r/247632/#review253706 Again, looks great with those bits below addressed. Feel free to either make Daniel take a look before if you want to land it (since he's in your timezone), or re-request review (though I'll look at it tomorrow). Thanks for working on this! ::: layout/style/nsComputedDOMStyle.cpp:5057 (Diff revision 1) > val->SetIdent(eCSSKeyword_content); > } else { > nsAutoString valueStr; > - > nsStyleUtil::AppendBitmaskCSSValue(nsCSSProps::kContainKTable, > mask, Oh, I just meant before that there's a trailing space here to the right of the comma (`s/mask, /mask,`), which should be removed. Though the other change is also fine if you want :) ::: modules/libpref/init/all.js:2931 (Diff revision 1) > > // Is support for CSS overflow-clip-box enabled for non-UA sheets? > pref("layout.css.overflow-clip-box.enabled", false); > > // Is support for CSS contain enabled? > -pref("layout.css.contain.enabled", true); > +pref("layout.css.contain.enabled", false); I'm assuming this goes away if the first patch doesn't have the opposite change. ::: servo/components/style/properties/gecko.mako.rs:3685 (Diff revision 1) > > let mut servo_flags = contain::computed_value::T::empty(); > let gecko_flags = self.gecko.mContain; > > - if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 && > - gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8) != 0 { > + if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 { > + debug_assert_eq!(gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8), nit: The general rust style [1][2] is generally using block indentation. We don't enforce usage of rustfmt on automation, but keeping that in mind would be sweet! So this could look like: ``` debug_assert_eq!( gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8), NS_STYLE_CONTAIN_ALL_BITS as u8, "When strict is specified ..." ); ``` And same below. [1]: https://github.com/rust-lang-nursery/fmt-rfcs [2]: https://github.com/rust-lang-nursery/rustfmt

Sorry – disregard the current state of the bug fixes right now, I need to clean up my local repository and squash some of the commits before re-pushing.

Comment on attachment 8981656 [details] Bug 1463589 - Add contain:size and contain:content parsing functionality. https://reviewboard.mozilla.org/r/247774/#review253798 Looks great, thanks! And thanks for bearing with me :-)

(It's possible/likely that "./mach devtools-css-db" will have a few things to update as a result of this change -- if so, let's include those changes here, too -- ideally in a separate commit because it's nice to separate automated changes from human-authored changes, but it doesn't matter too much.)

Comment on attachment 8981908 [details] Bug 1463589 - regenerate devtools database after updating css 'contain' values https://reviewboard.mozilla.org/r/247930/#review254058 I think this patch is not needed. This patch has been generated with the pref turned on, thus the new entry in the property database. Given it's off by default, the entry shouldn't be there, and you don't need this patch to land the previous one I'd think.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b87edc90b50d Add contain:size and contain:content parsing functionality. r=emilio