1241575 - WebKitCSSMatrix constructor throws for certain values in WebKit layout tests

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Mike Taylor [:miketaylr]]

This seems like a spec bug.

See https://github.com/WebKit/webkit/blob/master/LayoutTests/transforms/cssmatrix-2d-interface.xhtml#L23-L24 and https://github.com/WebKit/webkit/blob/master/LayoutTests/transforms/cssmatrix-2d-interface.xhtml#L79-L80

shouldBeNonNull('new WebKitCSSMatrix("none")');
shouldBeNonNull('new WebKitCSSMatrix("   none   ")');

m = new WebKitCSSMatrix();
m.setMatrixValue("translate(10px, 20px) scale(2, 3)");
shouldBe('m.a', '2');
shouldBe('m.b', '0');
shouldBe('m.c', '0');
shouldBe('m.d', '3');
shouldBe('m.e', '10');
shouldBe('m.f', '20');


https://github.com/WebKit/webkit/blob/master/LayoutTests/transforms/cssmatrix-3d-interface.xhtml#L29-L31

m = new WebKitCSSMatrix("matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)");
if (m)
  testPassed("string constructor");

I guess the issue here is what constitutes a valid transformList. Looking more closely at that, but wanted to get a bug filed.

Comment 1

[William Chen [:wchen]]

It looks like WebKit is parsing the transformList using the transform property syntax [1] while DOMMatrix is parsing using the SVG transform attribute syntax as described in the SVG spec [2].

The DOMMatrix spec [3] says it should be parsed using the SVG transform attribute syntax, but we do get it slightly wrong in Gecko because according to the CSS transforms spec [4], the SVG transform attribute should also accept transform property syntax but that is currently not the case in any implementations I have tested.

[1] https://www.w3.org/TR/css-transforms-1/#propdef-transform
[2] https://www.w3.org/TR/SVG/coords.html#TransformAttribute
[3] https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-fromstring
[4] https://www.w3.org/TR/css-transforms-1/#svg-syntax

Comment 2

[Mike Taylor [:miketaylr]]

I suggest we update the compat spec to parse a transformList the way that WebKit does (as a transform property), and update our implementation accordingly. Does that sound OK, William?

Comment 3

[Mike Taylor [:miketaylr]]

Spec issue filed here: https://github.com/whatwg/compat/issues/34

Comment 4

[William Chen [:wchen]]

Attachment: Use transform property syntax to parse WebKitCSSMatrix transform list.

Things that happen in this patch:

- Use nsCSSParser to parse transform lists as a transform property.
- Add a new function to nsStyleTransformMatrix to return a matrix for a transform list that contain only absolute lengths.
- Add new helper functions in nsStyleTransformMatrix for transform function so that they do not require a style context, presentation context and bounding box (unnecessary since we only allow absolute lengths).

With this patch, we pass all the transform list related tests.

Some quirks I noticed while working on this (and may need to be spec'ed):
- Chrome will accept "none", "inherit" and "initial". Safari will only accept "none". This patch will only accept "none" and treat it as the a 2d identity matrix (Chrome and Safari don't keep track of the is2D flag).
- Chrome and Safari will accept an empty string as a transform list but will syntax error on a string consisting of only whitespace. Empty string is treated as the identity matrix. This patch does the same.
- Chrome accepts relative length values. Safari accepts only absolute length values. DOMMatrix spec says to syntax error if values are not absolute length. This patch only accepts absolute length values.
- Calculated values are accepted in Chrome and accepted (but buggy and crashy) in Safari. This patch treats calculated values as a syntax error.

heycam: Can you review this to make sure I'm doing the CSS parts correctly?

Comment 5

[William Chen [:wchen]]

Comment on attachment 8711213 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list.

baku: Can you review the DOM part?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to William Chen [:wchen] from comment #1)
> It looks like WebKit is parsing the transformList using the transform
> property syntax [1] while DOMMatrix is parsing using the SVG transform
> attribute syntax as described in the SVG spec [2].
> 
> The DOMMatrix spec [3] says it should be parsed using the SVG transform
> attribute syntax, but we do get it slightly wrong in Gecko because according
> to the CSS transforms spec [4], the SVG transform attribute should also
> accept transform property syntax but that is currently not the case in any
> implementations I have tested.

Yeah, I'm not surprised that implementations don't yet parse transform="" with the new old-syntax-or-transform-property syntax.  Bug 878346 would cover that for us.

(In reply to William Chen [:wchen] from comment #4)
> Things that happen in this patch:
> 
> - Use nsCSSParser to parse transform lists as a transform property.

Just for WebKitCSSMatrix, yeah?  Leaving DOMMatrix to accept SVG syntax still?

> - Add a new function to nsStyleTransformMatrix to return a matrix for a
> transform list that contain only absolute lengths.
> - Add new helper functions in nsStyleTransformMatrix for transform function
> so that they do not require a style context, presentation context and
> bounding box (unnecessary since we only allow absolute lengths).
> 
> With this patch, we pass all the transform list related tests.
> 
> Some quirks I noticed while working on this (and may need to be spec'ed):
> - Chrome will accept "none", "inherit" and "initial". Safari will only
> accept "none". This patch will only accept "none" and treat it as the a 2d
> identity matrix (Chrome and Safari don't keep track of the is2D flag).

That sounds right to me.

> - Chrome and Safari will accept an empty string as a transform list but will
> syntax error on a string consisting of only whitespace. Empty string is
> treated as the identity matrix. This patch does the same.

Weird, but OK if we spec it that way.

> - Chrome accepts relative length values. Safari accepts only absolute length
> values. DOMMatrix spec says to syntax error if values are not absolute
> length. This patch only accepts absolute length values.

Sounds fine.

> - Calculated values are accepted in Chrome and accepted (but buggy and
> crashy) in Safari. This patch treats calculated values as a syntax error.

OK, as long as we spec that.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8711213 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list.

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

I would rather avoid the duplication of all of the matrix transform parsing functions here.  And I think it would be simpler to update the CSS parser to reject non-absolute lengths rather than doing a check in nsStyleTransformMatrix.  What do you think about:

* adding an argument to the existing ReadTransforms function that indicates it should accept absolute lengths and numbers and not calc() values
* adding a new VARIANT_* bit (the last one in our 32 bit variant bitfields!) which means "allow lengths only if they are absolute"
* adding a new nsCSSParser method specifically to parse a transform value, which takes an argument that controls acceptance of non-absolute values, and which passes that down to GetFunctionParseInformation so that the new VARIANT_XXX bit is included
* adding aContains{3dTransform,NonAbsoluteLength} outparams to the existing ReadTransforms function
* passing in dummy aConditions/aBounds values from the WebKitCSSMatrix constructor and null for the style context / pres context

?

Comment 8

[William Chen [:wchen]]

Comment on attachment 8711213 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list.

(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #6)
> (In reply to William Chen [:wchen] from comment #4)
> > Things that happen in this patch:
> > 
> > - Use nsCSSParser to parse transform lists as a transform property.
> 
> Just for WebKitCSSMatrix, yeah?  Leaving DOMMatrix to accept SVG syntax
> still?
Yes

Cancelling review request as I try heycam's approach.

Comment 9

[Mike Taylor [:miketaylr]]

(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #6)
> (In reply to William Chen [:wchen] from comment #1)
> > Some quirks I noticed while working on this (and may need to be spec'ed):
> > - Chrome will accept "none", "inherit" and "initial". Safari will only
> > accept "none". This patch will only accept "none" and treat it as the a 2d
> > identity matrix (Chrome and Safari don't keep track of the is2D flag).
> 
> That sounds right to me.

Edge also only accepts "none".

> > - Chrome and Safari will accept an empty string as a transform list but will
> > syntax error on a string consisting of only whitespace. Empty string is
> > treated as the identity matrix. This patch does the same.
> 
> Weird, but OK if we spec it that way.

Edge will take any amount of empty string, but agreed let's follow Chrome and Safari.

> > - Chrome accepts relative length values. Safari accepts only absolute length
> > values. DOMMatrix spec says to syntax error if values are not absolute
> > length. This patch only accepts absolute length values.
> 
> Sounds fine.

Edge throws a SyntaxError with relative lengths.

> 
> > - Calculated values are accepted in Chrome and accepted (but buggy and
> > crashy) in Safari. This patch treats calculated values as a syntax error.
> 
> OK, as long as we spec that.

Oh wow, new WebKitCSSMatrix("translate(calc(100em - 80px), 100em)") crashes my desktop Safari >_<. (will report a bug to WebKit). 

Edge also throws a SyntaxError here.

Comment 10

[Mike Taylor [:miketaylr]]

(Oh, just found https://bugs.webkit.org/show_bug.cgi?id=153333, thanks wchen)

Comment 11

[William Chen [:wchen]]

Attachment: Use transform property syntax to parse WebKitCSSMatrix transform list. v2

Implemented the changes suggested in comment 7.

Using this new approach it was easy to support calculated absolute length values (which also works in Chrome and Safari) so I've added that as well.

Comment 12

[William Chen [:wchen]]

Attachment: Part 2: Change type of variant bit fields from int32_t to uint32_t.

clang complains that the variants no longer fit in int32_t so I've changed the type to uint32_t.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8713073 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list. v2

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

r=me with the comments below addressed.

::: dom/base/WebKitCSSMatrix.cpp
@@ +57,5 @@
>  WebKitCSSMatrix*
>  WebKitCSSMatrix::SetMatrixValue(const nsAString& aTransformList,
>                                  ErrorResult& aRv)
>  {
> +  // An empty transform-list is a no-op.

s/transform-list/string/, since we will be treating non-empty whitespace-only strings as an error.

::: layout/style/nsCSSParser.cpp
@@ +15250,5 @@
> +    eTwoNumbers,
> +    eThreeNumbers,
> +    eThreeNumbersOneAngle,
> +    eMatrix,
> +    eMatrix,

Why not eMatrixPrefixed?

@@ +15252,5 @@
> +    eThreeNumbersOneAngle,
> +    eMatrix,
> +    eMatrix,
> +    eMatrix3d,
> +    eMatrix3d };

and eMatrix3dPrefixed?

@@ +15363,5 @@
>      return false;
>    }
>  
> +  if (aDisallowRelativeValues) {
> +    variantIndex = kNonRelativeVariantMap[variantIndex];

This is fine, although maybe you could also have done it by just masking off the bits that correspond to relative units.

@@ +15467,5 @@
>  /* Parses a transform property list by continuously reading in properties
>   * and constructing a matrix from it.
>   */
> +bool CSSParserImpl::ParseTransform(bool aIsPrefixed,
> +                                   bool aDisallowRelativeValues) {

Please keep the brace on a new line.  And while touching this, would you mind inserting a line break after the return type?

::: layout/style/nsStyleTransformMatrix.h
@@ +166,5 @@
>                                           nsPresContext* aPresContext,
>                                           mozilla::RuleNodeCacheConditions& aConditions,
>                                           TransformReferenceBox& aBounds,
> +                                         float aAppUnitsPerMatrixUnit,
> +                                         bool* aContains3dTransform = nullptr);

I don't think it makes sense to default this to null if MatrixForTransformFunction will unconditionally dereference it.  Either add null checks to MatrixForTransformFunction or remove the default value here and the null check in ReadTransforms.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8713315 [details] [diff] [review]
Part 2: Change type of variant bit fields from int32_t to uint32_t.

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

I didn't check carefully that you got all int32_ts in nsCSSParser.cpp that need changing.

Comment 15

[William Chen [:wchen]]

(In reply to Cameron McCormack (:heycam) (busy Jan 30 – Feb 6) (away Feb 8–9) from comment #13)
> Comment on attachment 8713073 [details] [diff] [review]
> Use transform property syntax to parse WebKitCSSMatrix transform list. v2
> 
> Review of attachment 8713073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not eMatrixPrefixed?
> and eMatrix3dPrefixed?
The prefixed version allow percent values.
> This is fine, although maybe you could also have done it by just masking off
> the bits that correspond to relative units.
The method returns a pointer to a static array, so it isn't as simple as masking off the bits.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8989a435a6cb

Comment 17

[Cameron McCormack (:heycam)]

(In reply to William Chen [:wchen] from comment #15)
> > Why not eMatrixPrefixed?
> > and eMatrix3dPrefixed?
> The prefixed version allow percent values.

OK.  (Though we always pass in false for aIsPrefixed so I guess it doesn't matter.)

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8989a435a6cb

Comment 19

[Mike Taylor [:miketaylr]]

Comment on attachment 8713073 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list. v2

Approval Request Comment
[Feature/regressing bug #]:
Bug 717722, add support for WebKitCSSMatrix for webcompat.

[User impact if declined]:
Sites could break .

[Describe test coverage new/current, TreeHerder]:
Tests in the patch.

[Risks and why]: 
Low risk, IMO. This patch fixes part of our implementation which is already in Aurora (due to a spec bug, which has been fixed).

[String/UUID change made/needed]:
None

Note: I guess these two patches were combined into a single commit that landed on m-c. This uplift request is for the equivalent of what landed on m-c.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Supporting the spec sounds good to me. But this is a fairly big patch to uplift. 

Since the other work to support WebKitCSSMatrix is going to stay on Aurora for now, will these changes also be limited to aurora?

Comment 21

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Yes, WebKitCSSMatrix support is behind the same pref as all the other webkit compat work. (which means it won't ship past Aurora for now)

Comment 22

[Mike Taylor [:miketaylr]]

Yep, what Daniel said. This patch fixes a long-standing Gecko bug with DOMMatrix, so that's a bonus too.

Comment 23

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Oh, I missed that Gecko-DOMMatrix bugfix aspect of this bug -- I'm mostly a bystander here.)

So the answer to Liz's question is, a hypothetical 46aurora uplift here *will* have an effect beyond beta, due to the DOMMatrix changes.  (Which is a bonus, but also a risk.)

Comment 24

[Mike Taylor [:miketaylr]]

Oh you know what, scratch that. I got this bug confused with Bug 1245242 (which fixes a DOMMatrix bug to fix a WebKitCSSMatrix issue). 

This one is purely WebKitCSSMatrix -- the benefits being that our Aurora population will have more code doing what it was intended for (that is, is compatible with code written for mobile WebKit browsers).

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8713073 [details] [diff] [review]
Use transform property syntax to parse WebKitCSSMatrix transform list. v2

WebKit work, will stay in aurora. Please uplift.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ad1db9f3c4c