shorthand serialization for properties using variables is sometimes incorrect

RESOLVED FIXED in mozilla34

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

Trunk
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8472214 [details]
testcase, border-right serialized to "  " rather than ""
(Assignee)

Comment 2

4 years ago
Created attachment 8472811 [details] [diff] [review]
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.
Attachment #8472811 - Flags: review?(cam)
(Assignee)

Comment 3

4 years ago
Created attachment 8472813 [details] [diff] [review]
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.
Attachment #8472813 - Flags: review?(cam)
(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 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?
Attachment #8472811 - Flags: review?(cam) → review+
Attachment #8472813 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/af3f26bbff7f
https://hg.mozilla.org/mozilla-central/rev/74d70950f73e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.