1053114 - shorthand serialization for properties using variables is sometimes incorrect

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

Given the style declaration:

<p style="--a: 1px solid red; border:  var(--a);"></p>

p.style.getPropertyValue("border") ==> "  var(--a)"
p.style.getPropertyValue("border-right") ==> "  "

The bug here is that subproperties that are shorthands should all be "" but because this isn't handled universally for all shorthands, the code within Declaration::GetValue falls down into the code for each individual subproperty. For shorthand properties that serialize a set of subproperties and insert spaces in between, this leads to values that are not "". This is incorrect I think.

It also seems lame that the serialization here doesn't trim and collapse unnecessary whitespace but, meh, whatever...

I think this case should be handled for all subproperties and not within the serialization code for individual shorthands.

Comment 1

[John Daggett (:jtd)]

Attachment: testcase, border-right serialized to " " rather than ""

Comment 2

[John Daggett (:jtd)]

Attachment: patch, add test of other shorthands affected by variable usage

Add tests to test_value_storage.html to assure that we are correctly serializing other shorthands affected by the use of a variable for a given style property.

In short, for any shorthand property X, all other shorthands that affect any longhand property affected by X must serialize to empty string.

Example:

  border: var(--a);

  getPropertyValue('border') ==> "var(--a)"
  getPropertyValue('border-right') ==> ""

This is slightly tricky because of the mixture of prefixed and unprefixed properties in property_database.js.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, fix serialization of shorthands with token value subproperties

As suggested by dbaron on IRC, handle this by counting non-matching token stream values and returning immediately if the count is greater than zero *and* the property is not the shorthand containing the variable reference.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to John Daggett (:jtd) from comment #0)
> It also seems lame that the serialization here doesn't trim and collapse
> unnecessary whitespace but, meh, whatever...

For properties that are token streams (have a var() in them), I think it's correct to return the untrimmed white space.  Collapsing is probably OK, since conceptually we should be storing a list of tokens, rather than the input string, but I think it's easiest just to return exactly the string used in the value.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8472811 [details] [diff] [review]
patch, add test of other shorthands affected by variable usage

Review of attachment 8472811 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this change.

::: layout/style/test/test_value_storage.html
@@ +296,5 @@
> +          subpropinfo.shorthands = [];
> +        }
> +        subpropinfo.shorthands.push(prop);
> +      }
> +    }

I think I would prefer these be stored outside gCSSProperties, just to avoid any confusion if people might looking at the earlier code and expect to be able to look up .shorthands in gCSSProperties in other tests.  So maybe define a global gPropertyShorthands that maps property names to arrays of shorthands they are set by?

Comment 6

[John Daggett (:jtd)]

Pushed to inbound, with changes based on review comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3f26bbff7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d70950f73e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af3f26bbff7f
https://hg.mozilla.org/mozilla-central/rev/74d70950f73e