1170173 - Parse CSS 'contain' property.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kyle Zentner]

This feature will be gated using the preference "layout.css.contain.enabled". See the main bug 1150081 for details on the property.

Comment 1

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52032fb046b9

Comment 2

[Kyle Zentner]

Attachment: ParseContain

This is pretty standard, as far as I can tell. The entries in the property_database might be a bit excessive for testing, but I wanted to make sure this was done right.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8613538 [details] [diff] [review]
ParseContain

>+static const KTableValue strictTable[] = {
>+  eCSSKeyword_strict,             NS_STYLE_CONTAIN_STRICT,
>+  eCSSKeyword_UNKNOWN,-1
>+};
>+
>+static const int32_t maskContain[] = {
>+  MASK_END_VALUE
>+};
>+
>+bool
>+CSSParserImpl::ParseContain(nsCSSValue& aValue)
>+{
>+  if (ParseVariant(aValue,
>+                   VARIANT_INHERIT | VARIANT_NONE | VARIANT_KEYWORD,
>+                   strictTable)) {
>+    return true;
>+  } else {
>+    return ParseBitmaskValues(aValue, nsCSSProps::kContainKTable,
>+                              maskContain);
>+  }
>+}
[...]
>+const KTableValue nsCSSProps::kContainKTable[] = {
>+  eCSSKeyword_none,               NS_STYLE_CONTAIN_NONE,
>+  eCSSKeyword_strict,             NS_STYLE_CONTAIN_STRICT,
>+  eCSSKeyword_layout,             NS_STYLE_CONTAIN_LAYOUT,
>+  eCSSKeyword_style,              NS_STYLE_CONTAIN_STYLE,
>+  eCSSKeyword_paint,              NS_STYLE_CONTAIN_PAINT,
>+  eCSSKeyword_UNKNOWN,-1
>+};

It looks to me like this would accept values like "layout strict", which should really be invalid.  (We'll fail the first ParseVariant call, and then succeed in the ParseBitmaskValues call, with "layout" & "strict".

Rather than using this two-keyword-table strategy, I'd rather we just stuck with a single table, and do a check after the ParseBitmaskValues call to say that "if we got strict, we should have gotten *exactly* strict and nothing else". Something like the logic in ParseGridAutoFlow(), except with the post-ParseBitmaskValues-check code being something like:
  if ((intValue & NS_STYLE_CONTAIN_STRICT) &&
      (intValue != NS_STYLE_CONTAIN_STRICT)) {
    // 'strict' is only valid on its own.
    return false;
  }

Comment 4

[Daniel Holbert [:dholbert]]

(Also, please add an invalid_value like "layout strict" to property_database.js -- with one of the multi-keywords, followed by single-keyword 'strict'.)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8613538 [details] [diff] [review]
ParseContain

>Bug 1170173 - Parse CSS contain property.

Nit: put 'contain' in quotes here.  Otherwise, this is easy to misread as gibberish/confusing, if someone misinterprets "contain" as being a verb in this sentence. (which seems likely for people skimming the commit log who don't immediately know that "contain" is the name of a property)

>+++ b/layout/style/nsCSSPropList.h
>+CSS_PROP_DISPLAY(
>+    contain,
>+    contain,
>+    Contain,
>+    CSS_PROPERTY_PARSE_VALUE |
>+        CSS_PROPERTY_VALUE_PARSER_FUNCTION,
>+    "layout.css.contain.enabled",
>+    VARIANT_HK,
>+    kContainKTable,
>+    CSS_PROP_NO_OFFSET,
>+    eStyleAnimType_None)

VARIANT_HK should just be 0 here; I think that's only used if we use the default parsing function.  (Its value here will be unused -- so, better to provide 0 than to provide something & give the false implication that it has an effect.)  

See e.g. "image-orientation", which also has a keyword-table & has a custom parsing function -- it specifies 0 for this macro-argument.

(I think I told you VARIANT_HK was what you'd want, when skimming a sample style-system patch with you last week - sorry for misleading you. That was before I'd realized we need a custom parsing function, I think.)

>+++ b/layout/style/nsCSSProps.h
>@@ -604,16 +604,17 @@ public:
>   static const KTableValue kColorKTable[];
>   static const KTableValue kContentKTable[];
>   static const KTableValue kControlCharacterVisibilityKTable[];
>   static const KTableValue kCursorKTable[];
>   static const KTableValue kDirectionKTable[];
>   // Not const because we modify its entries when various 
>   // "layout.css.*.enabled" prefs changes:
>   static KTableValue kDisplayKTable[];
>+  static const KTableValue kContainKTable[];
>   static const KTableValue kElevationKTable[];
>   static const KTableValue kEmptyCellsKTable[];

Insert this up a few lines, to preserve alphabetical order.  (Chunks of this list aren't in alphabetical order, but the immediate section above this insertion-point seems to be, so best not to break that.)

Same for the array definition in the corresponding .cpp file.

(It looks like you're putting things alongside "display" elsewhere, which is probably why you chose this point here.  That probably makes sense elsewhere, but this file seems to be sorted mostly alphabetically instead of functionally.)

>+++ b/layout/style/nsStyleConsts.h
>+// See nsStyleDisplay
>+#define NS_STYLE_CONTAIN_NONE                   0
>+#define NS_STYLE_CONTAIN_STRICT                 7
>+#define NS_STYLE_CONTAIN_LAYOUT                 1
>+#define NS_STYLE_CONTAIN_STYLE                  2
>+#define NS_STYLE_CONTAIN_PAINT                  4

Per IRC, I think "strict" might be becoming its own bit here; but if it stays a bitmask, please define it explicitly in terms of the other values (as an "|" expression), instead of just using "7".

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>--- a/layout/style/nsStyleStruct.h
>+++ b/layout/style/nsStyleStruct.h
>@@ -2061,16 +2061,17 @@ struct nsStyleDisplay {
>   typedef nsStyleBackground::Position Position;
> 
>   // We guarantee that if mBinding is non-null, so are mBinding->GetURI() and
>   // mBinding->mOriginPrincipal.
>   nsRefPtr<mozilla::css::URLValue> mBinding;    // [reset]
>   nsRect  mClip;                // [reset] offsets from upper-left border edge
>   float   mOpacity;             // [reset]
>   uint8_t mDisplay;             // [reset] see nsStyleConsts.h NS_STYLE_DISPLAY_*
>+  uint8_t mContain;             // [reset] see nsStyleConsts.h NS_STYLE_CONTAIN_*
>   uint8_t mOriginalDisplay;     // [reset] saved mDisplay for position:absolute/fixed
>                                 //         and float:left/right; otherwise equal
>                                 //         to mDisplay

Don't split up mDisplay & mOriginalDisplay here. (Insert after mOriginalDisplay, I guess.

>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
>+// Is support for CSS contain enabled?
>+pref("layout.css.contain.enabled", false);

You should enable this pref for mochitests (so that your property_database.js code gets exercised during testing), by adding a line somewhere in this file:
http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#149

Comment 6

[Daniel Holbert [:dholbert]]

I think you may also need to add some custom code to nsCSSValue::AppendToString(), with a call to AppendBitmaskCSSValue() similar to your DoGetContain() code.  (This is for serializing a specified-style representation, e.g. if someone sets an element's "style" attribute and then tries to read back elem.style.contain.)

(Once you've enabled this property in mochitests via testing/profiles/prefs_general.js, you'll likely hit test-failures that are pointing to this missing serialization code.)

Comment 7

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f006bc34b5c0

Comment 8

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48d284e9d58

Comment 9

[Kyle Zentner]

Attachment: ParseContain

Comment 10

[Kyle Zentner]

It seems that if I remove VARIANT_HK, as in the patch above, that tab-completion of the property in the style editor no longer works. This is because of 

> layout/inspector/inDOMUtils.cpp:870
>    if (propertyParserVariant & VARIANT_KEYWORD) {
>      GetKeywordsForProperty(propertyID, array);
>    }

I assume this means I should add VARIANT_HK back in?

Comment 11

[Daniel Holbert [:dholbert]]

That's interesting; I didn't realize that feature depends on the VARIANT_KEYWORD flag. I'm not sure that it should...

Comment 12

[Daniel Holbert [:dholbert]]

I suppose it's worth throwing VARIANT_HK back in there for now, though, since it's not entirely unused as I'd thought.

(Could you check some other properties from nsCSSPropList.h that (a) include a keyword table in their macro-blurb, and (b) have 0 instead of VARIANT_HK, and see if they have this problem too? e.g. "image-orientation: from-image".  If so, please file a bug in Core|CSS on fixing those.)

Comment 13

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe9be01e2834

Comment 14

[Kyle Zentner]

Attachment: ParseContain

This is the version tested in the most recent try run. I think I've at least implemented all of parsing and serialization now.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8613773 [details] [diff] [review]
ParseContain

>+++ b/layout/style/nsCSSParser.cpp
>+bool
>+CSSParserImpl::ParseContain(nsCSSValue& aValue)
>+{
>+  static const int32_t maskContain[] = {
>+    MASK_END_VALUE
>+  };
>+
>+  if (ParseVariant(aValue,
>+                   VARIANT_INHERIT | VARIANT_NONE,
>+                   nullptr)) {
>+    return true;
>+  } else {

Drop the "else" here.  (else after return is unnecessary)

If you haven't already, it's worth familiarizing yourself with the general & C++ sections of the coding style guide -- this style guideline is mentioned there:
   https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style


>+    bool parsed = ParseBitmaskValues(aValue, nsCSSProps::kContainKTable,
>+                                     maskContain);
>+    if (parsed && aValue.GetIntValue() & NS_STYLE_CONTAIN_STRICT) {


I'd marginally prefer:
  if (!ParseBitmaskValues(...)) {
    return false;
  }
  [proceed assuming it succeeded]

...though what you've got is OK too.

(But if you keep this part as-is, I'd suggest s/parse/parseSuccess/ or something boolier-sounding, so that "return parsed;" later on is clearer that it's returning a success value, rather than a parsed value)

>+      if (aValue.GetIntValue() != NS_STYLE_CONTAIN_STRICT) {
>+        // Disallow any other keywords in combination with 'strict'.
>+        return false;
>+      }
>+      aValue.SetIntValue(NS_STYLE_CONTAIN_STRICT |
>+          NS_STYLE_CONTAIN_EVERYTHING, eCSSUnit_Enumerated);

Add a comment before the SetIntValue call here, e.g.:

>+++ b/layout/style/nsCSSProps.h
>@@ -604,16 +604,17 @@ public:
>   static KTableValue kDisplayKTable[];
>+  static const KTableValue kContainKTable[];
>   static const KTableValue kElevationKTable[];

This is still out of order, alphabetically (both .h and .cpp file)

>+++ b/layout/style/nsCSSValue.cpp
>@@ -1287,16 +1287,42 @@ nsCSSValue::AppendToString(nsCSSProperty
>+    case eCSSProperty_contain:
>+      MOZ_ASSERT(GetUnit() == eCSSUnit_Enumerated, "unexpected unit");

I think this is inside of a "if (eCSSUnit_Enumerated == unit) {" check, yes?   Given that, this assertion seems overly paranoid, so I'd just drop it.

>+      if (intValue & NS_STYLE_CONTAIN_STRICT) {
>+        // Only output strict.
>+        AppendASCIItoUTF16(nsCSSProps::ValueToKeyword(NS_STYLE_CONTAIN_STRICT,
>+                              nsCSSProps::kContainKTable),
>+                           aResult);
>+      } else {
>+        // Output each property.
>+        bool needSpace = false;
>+        for (uint32_t i = NS_STYLE_CONTAIN_LAYOUT;
> [etc]

nsStyleUtil::AppendBitmaskCSSValue can save you a lot of code here, both for the multi-value case and for the "strict" case.

I'd suggest:
  if (intValue & NS_STYLE_CONTAIN_STRICT) {
    intValue = NS_STYLE_CONTAIN_STRICT; // so we only output "strict"
  }
  AppendBitmaskCSSValue(...)

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+  if (mask == 0) {
>+    val->SetIdent(eCSSKeyword_none);
>+  } else if (mask & NS_STYLE_CONTAIN_STRICT) {
>+    val->SetIdent(eCSSKeyword_strict);
>+  } else {
>+    nsAutoString valueStr;
>+
>+    nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_contain,
>+      mask, NS_STYLE_CONTAIN_LAYOUT,
>+      NS_STYLE_CONTAIN_PAINT, valueStr);
>+    val->SetString(valueStr);
>+  }

(You could do the same "mask = NS_STYLE_CONTAIN_STRICT;" tweak here, if you like, though it's less of a simplification in this case, so probably not worth it.)

>+++ b/layout/style/nsStyleConsts.h>+// See nsStyleDisplay
>+// If these are re-ordered, nsComputedDOMStyle::DoGetContain() and
>+// nsCSSValue::AppendToString() must be updated.
>+#define NS_STYLE_CONTAIN_NONE                   0
>+#define NS_STYLE_CONTAIN_STRICT                 0x1
>+#define NS_STYLE_CONTAIN_LAYOUT                 0x2
>+#define NS_STYLE_CONTAIN_STYLE                  0x4
>+#define NS_STYLE_CONTAIN_PAINT                  0x8
>+// NS_STYLE_CONTAIN_EVERYTHING does not correspond to a keyword.
>+#define NS_STYLE_CONTAIN_EVERYTHING             (NS_STYLE_CONTAIN_LAYOUT | \
>+                                                 NS_STYLE_CONTAIN_STYLE  | \
>+                                                 NS_STYLE_CONTAIN_PAINT)

I'm mixed on CONTAIN_EVERYTHING. It sounds a bit too much like an actual thing ("contain everything").  The documentation here clears that up a bit, but that won't be as clear at its usages.

I wonder if "NS_STYLE_CONTAIN_ALL_BITS" might be a clearer name here? Or NS_STYLE_CONTAIN_LSP_MASK (LSP for Layout/Style/Paint)?

>+    invalid_values: [
>+      "",

I don't think you need to bother putting the empty string as an invalid value -- I'm pretty sure we already have a test that exercises that for each longhand property.  (since it's equally-invalid for each longhand property, I think.)

>+++ b/testing/profiles/prefs_general.js
> // Enable speech synth test service, and disable built in platform services.
> user_pref("media.webspeech.synth.test", true);
>+
>+// Enable CSS 'contain' for testing
>+user_pref("layout.css.contain.enabled", true);

Move this up to be alongside the other layout.css.* prefs in this file.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> >+      aValue.SetIntValue(NS_STYLE_CONTAIN_STRICT |
> >+          NS_STYLE_CONTAIN_EVERYTHING, eCSSUnit_Enumerated);
> 
> Add a comment before the SetIntValue call here, e.g.:

Sorry to trail off there -- I forgot to paste in the my draft sample-comment :)
Sample comment:
 // 'strict' implies all the other keyword values. We keep
 //  the 'strict' bit set as well, for use when re-serializing.

Comment 17

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2960dca0ca03

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8613773 [details] [diff] [review]
ParseContain

(Marking yesterday's patch r- to clear out my review queue, since it's not quite ready per comments above.  Looks pretty close though!)

Comment 19

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1b5d8affea5

Comment 20

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a65032e3bf3

Comment 21

[Kyle Zentner]

Attachment: ParseContain

This version passes the tests, and everything seems to work well when I experimented with it in the browser.

If you think I should remove / change the assertions I added, please let me know.

The reason to the strange change in the devtools test is that that test previously depended on there only being 6 CSS properties which began with 'co'.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8615543 [details] [diff] [review]
ParseContain

Looks good! Just some minor stylistic nits. r=me with the following addressed:

>+++ b/layout/style/nsCSSParser.cpp
>+bool
>+CSSParserImpl::ParseContain(nsCSSValue& aValue)
>+{
>+  static const int32_t maskContain[] = {
>+    MASK_END_VALUE
>+  };
>+
>+  if (ParseVariant(aValue,
>+                   VARIANT_INHERIT | VARIANT_NONE,
>+                   nullptr)) {
>+    return true;
>+  }

Looks like this ParseVariant(...) call can be collapsed to one line -- let's do that, for brevity.

>+  if (!ParseBitmaskValues(aValue, nsCSSProps::kContainKTable,
>+                                     maskContain)) {

Fix indentation here.  Also, declare maskContain immediately before this line, since it's not used until this point (and it's short), to make its purpose more clear. Also, I'd say we should just make it a one-liner, e.g.
  static const int32_t maskContain[] = { MASK_END_VALUE };
...since it doesn't actually have any useful entries to list out, so there's no benefit from making it a multi-line thing.

>+++ b/layout/style/nsCSSPropList.h
>+CSS_PROP_DISPLAY(
>+    contain,
>+    contain,
>+    Contain,
>+    CSS_PROPERTY_PARSE_VALUE |
>+        CSS_PROPERTY_VALUE_PARSER_FUNCTION,
>+    "layout.css.contain.enabled",
>+    // Does not affect parsing, but is needed for tab completion.
>+    VARIANT_HK | VARIANT_NONE,

End this comment with a ":" to make it clearer that it's talking about the next line.

Also, s/tab completion/tab completion in devtools/

(Remember to file a bug on fixing other properties that have broken tab-completion due to this, too.)

>+    kContainKTable,
>+    offsetof(nsStyleDisplay, mContain),
>+    eStyleAnimType_None)

I'm pretty sure this "offsetof" parameter is only used if we have a non-"eStyleAnimType_None" animtype.

So: given that this is eStyleAnimType_None, let's just use CSS_PROP_NO_OFFSET instead of offsetof(...).  We do this for a lot of other keyword-valued properties, and I think that's probably the pattern we should follow. (I think we do what you've got here for some properties, too, but I think those places should probably be simplified to use CSS_PROP_NO_OFFSET as well.)

>+++ b/layout/style/nsCSSProps.cpp
>+const KTableValue nsCSSProps::kContainKTable[] = {
>+  eCSSKeyword_none,               NS_STYLE_CONTAIN_NONE,
>+  eCSSKeyword_strict,             NS_STYLE_CONTAIN_STRICT,
>+  eCSSKeyword_layout,             NS_STYLE_CONTAIN_LAYOUT,
>+  eCSSKeyword_style,              NS_STYLE_CONTAIN_STYLE,
>+  eCSSKeyword_paint,              NS_STYLE_CONTAIN_PAINT,
>+  eCSSKeyword_UNKNOWN,-1
>+};

Reduce the amount of whitespace between the entries here (by ~12 space characters I think).

(The list-alignment is nice for readability, but with too much space, it's harder to scan from left value to right value)

>+++ b/layout/style/nsCSSValue.cpp
>+    case eCSSProperty_contain:
>+      if (intValue & NS_STYLE_CONTAIN_STRICT) {
>+        NS_ASSERTION(intValue == (NS_STYLE_CONTAIN_STRICT | NS_STYLE_CONTAIN_ALL_BITS),
>+                     "contain: strict should imply contain: layout style paint.");

Nit: drop the period at the end of the assertion message. (When an assertion fails, we print it with a semicolon appended to the end of the message, IIRC; so if the message already ends with a period, it ends up ending with ".;" which looks weird.)

>+      nsStyleUtil::AppendBitmaskCSSValue(aProperty,
>+          intValue,
>+          NS_STYLE_CONTAIN_STRICT,
>+          NS_STYLE_CONTAIN_PAINT,
>+          aResult);
>+      break;

I don't think we need the de-indentation here -- doesn't look like we're at risk of going over 80 characters. So, just align the args below "aProperty".

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+  } 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.");

As above, drop the final "."

>+  } else {
>+    nsAutoString valueStr;
>+
>+    nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_contain,
>+      mask, NS_STYLE_CONTAIN_LAYOUT,
>+      NS_STYLE_CONTAIN_PAINT, valueStr);

As above, I don't think there's a need for this de-indentation here -- so, don't de-indent the later function-args; align them below the 1st one.

Comment 23

[Kyle Zentner]

Attachment: ParseContain

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8422e49e2ab3

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8422e49e2ab3

Comment 26

[yisibl]

The spec add the `size` value: https://drafts.csswg.org/css-containment/#contain-property