1330172 - Property value with variable reference is not idempotent with assigning cssText

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Assuming there is a `decl` which refers to "display: var(--a);". `decl.display` would be " var(--a)", and `decl.cssText` would be "display:  var(--a);" (notice the change on the whitespaces). If you do `decl.cssText = decl.cssText;`, `decl.display` would now be "  var(--a)", and `decl.cssText` would become "display:   var(--a);", which is not idempotent.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It is not completely clear to me what is the right approach to fix this.

CSSOM spec says we should append a whitespace after ":", and we preserve all whitespaces after ":" when parsing, and thus the issue.

It seems Blink currently strips the whitespaces at the beginning, and collapse continuous whitespaces inside. I guess we may at least want to strip the whitespaces at the beginning, since that doesn't really make sense for properties.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

This is also an issue for custom properties.

It seems for custom properties, Blink doesn't strip whitespaces (but it still collapse continuous whitespaces, FWIW), and it doesn't append a whitespace after ":" in that case.

Comment 3

[Cameron McCormack (:heycam)]

I don't feel too strongly about it, but I think there is a similarity to regular properties with variable references and custom properties (with whatever value), in that they are both token streams.  So I think we should be trimming or not trimming spaces for both of them or neither of them.  It definitely makes sense to me to preserve the space for custom properties, so I would say to preserve the space for regular properties with token stream values, even though we know that in reality no regular property will care about that space.

The idempotency sounds like something we can solve.  Easiest seems to be to change CSSOM to not emit the space after the colon if the value is a token stream.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It is a common failure pattern on stylo that, assigning "var(--a)" to a property produces "property:var(--a);", but we currently expect "property: var(--a);". So are you proposing we should align to servo's behavior on this?

I'm a bit concerned about how to change the spec given the inconsistency between impls.

Comment 5

[Cameron McCormack (:heycam)]

If the implementations are inconsistent then it might make it easier to tighten the spec.  I propose we:

* file an issue on Custom Properties to make clear that leading and trailing spaces are part of the value of both regular and custom properties with token stream values (not sure what the right spec term is for token streams)

* file an issue on CSSOM to omit the space after the colon if the value of the property is a token stream

* change Gecko to follow this

Then Servo doesn't need to change, I think.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I can probably understand why Blink chose the other way.

Declaration serialization and value serialization is pretty far in code, and the latter is invoked in various places, so passing extra information from value serialization to declaration serialization could be painful (need changes to lots of places with little usefulness).

I'd like to choose a different way that we adding whitespace according to whether the value already has preceding and trailing whitesapces. If it doesn't, we add one between it and ":" and "!important", otherwise we leave it as is. It would solve the idempotent issue of cssText, but property value would be changed by assigning cssText.

But changing Servo code to that behavior also seems painful: we would need to serialize the value into a string and then append that string to result, rather than simply writing the result in one pass.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(The difference of handling this accounts for ~5.5k failures in style system mochitests, as a record.)

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1330172 part 1 - Fix serialization of CSS-wide keyword in variable.

Review commit: https://reviewboard.mozilla.org/r/106056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106056/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1330172 part 2 - Fix serialization of property declaration with variable reference.

Review commit: https://reviewboard.mozilla.org/r/106058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106058/

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8828765 [details]
Bug 1330172 part 1 - Fix serialization of CSS-wide keyword in variable.

https://reviewboard.mozilla.org/r/106056/#review107268

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8828766 [details]
Bug 1330172 part 2 - Fix serialization of property declaration with variable reference.

https://reviewboard.mozilla.org/r/106058/#review107270

::: layout/style/Declaration.cpp:1751
(Diff revision 1)
>          continue;
>      }
>  
>      // Try to use this property in a shorthand.
>      nsAutoString value;
> +    bool isTokenStream;

Move the isTokenStream declaration inside the loop?

::: layout/style/Declaration.cpp:1770
(Diff revision 1)
>        if (shorthand == eCSSProperty_font_variant &&
>            value.EqualsLiteral("-moz-use-system-font")) {
>          continue;
>        }
>  
>        // If GetPropertyValueByID gives us a non-empty string back, we can

s/GetPropertyValueByID/GetPropertyValueInternal/

::: layout/style/Declaration.cpp:1811
(Diff revision 1)
>      MOZ_ASSERT(value.IsEmpty(), "value should be empty now");
> -    AppendPropertyAndValueToString(property, value, aString);
> +    AppendPropertyAndValueToString(property, aString, value, isTokenStream);

If value is empty here, then the aValueIsTokenStream argument value doesn't matter.  So maybe we should just pass in false so it doesn't look like isTokenStream's value is important here.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8828765 [details]
Bug 1330172 part 1 - Fix serialization of CSS-wide keyword in variable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106056/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8828766 [details]
Bug 1330172 part 2 - Fix serialization of property declaration with variable reference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106058/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de759b957d75
part 1 - Fix serialization of CSS-wide keyword in variable. r=heycam
https://hg.mozilla.org/integration/autoland/rev/434b0b5d7780
part 2 - Fix serialization of property declaration with variable reference. r=heycam

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de759b957d75
https://hg.mozilla.org/mozilla-central/rev/434b0b5d7780