208730 - investigate output of shorthand properties when some subprops !important

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bzbarsky wrote in bug 125246 comment 41:

  In nsCSSDeclaration::GetValue(nsCSSProperty, nsAString&):

    It looks like our existing code also screws up in the case when a shorthand
    has both !important and not !important parts.  File a bug to deal with your
    XXX comment, please?  It would be good to decide what exactly we should
    return in such cases....  Same applies to "border".

Comment 1

[Daniel Holbert [:dholbert]]

For reference: I think the XXX comment referenced here (from the attachment in bug 125246 Comment 40) was:

+  if (nsCSSProps::IsShorthand(aProperty) ||
+      aProperty == eCSSProperty_play_during) {
+    // XXX What about checking the consistency of '!important'?
+    switch (aProperty) {
+      case eCSSProperty_margin: 
+      case eCSSProperty_padding: 
+      case eCSSProperty_border_color: 
+      case eCSSProperty_border_style: 
+      case eCSSProperty__moz_border_radius: 
+      case eCSSProperty__moz_outline_radius: 
+      case eCSSProperty_border_width: {
+        const nsCSSProperty* subprops =
+          nsCSSProps::SubpropertyEntryFor(aProperty);
+        NS_ASSERTION(nsCSSProps::kTypeTable[subprops[0]] == eCSSType_Value &&
+                     nsCSSProps::kTypeTable[subprops[1]] == eCSSType_Value &&
+                     nsCSSProps::kTypeTable[subprops[2]] == eCSSType_Value &&
+                     nsCSSProps::kTypeTable[subprops[3]] == eCSSType_Value,
+                     "type mismatch");
+        if (!AppendValueOrImportantValueToString(subprops[0], aValue) ||
+            !(aValue.Append(PRUnichar(' ')), AppendValueOrImportantValueToString(subprops[1], aValue)) ||
+            !(aValue.Append(PRUnichar(' ')), AppendValueOrImportantValueToString(subprops[2], aValue)) ||
+            !(aValue.Append(PRUnichar(' ')), AppendValueOrImportantValueToString(subprops[3], aValue))) {
+          aValue.Truncate();
         }

Comment 2

[Daniel Holbert [:dholbert]]

This code all was replaced in stylo, so I suspect this is invalid at this point (nothing to be fixed, at least no such XXX comment anymore).

I'm not entirely clear on what the problematic scenario was; presumably it was something like inspecting the specified-style of a shorthand for an element whose subproperties had mixed important vs non-important. Currently we just decline to serialize a value (we return the empty string), which seems reasonable, I think; and it matches what Chrome does as well FWIW.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase to demonstrate what this bug was getting at (I think?)