Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[geometry] DOMMatrix should not depend on the CSS parser #122

Closed
esprehn opened this issue Mar 28, 2017 · 10 comments
Closed

[geometry] DOMMatrix should not depend on the CSS parser #122

esprehn opened this issue Mar 28, 2017 · 10 comments
Assignees

Comments

@esprehn
Copy link

esprehn commented Mar 28, 2017

I do not think the low level geometry types should depend on having a CSS tokenizer and parser available.

The spec does not provide affordances to parse any other CSS syntax, for example rect() into DOMRect, and parsing CSS is better handled by the Typed OM spec which uses an operation list to represent transforms instead of a matrix (which is a better fit for CSS anyway).

If authors wish to create a DOMMatrix from a list of steps they can do:

  new DOMMatrix()
     .translateSelf(...)
     .scaleSelf(...)
     ...

which will also be faster since it doesn't require parsing all of the numbers back out of a string.

@shans @tabatkins @bfgeek

@zcorpan
Copy link
Member

zcorpan commented Apr 4, 2017

Is there data on how often the CSS parsing happens for WebKitCSSMatrix?

Maybe we could specify a simpler non-CSS parser that is compatible enough with deployed content?

@chrishtr
Copy link
Contributor

chrishtr commented Apr 6, 2017

Blink and Gecko do not have use counter data at present for how how often the CSS parsing happens,
either via the constructor or setMatrixValue. On the other hand, we do have data from HTTPArchive, summarized as follows:

In httparchive:har.2017_01_15_chrome_requests_bodies there are 571 hits for 'setMatrixValue', and most of them are various versions of this library:
http://animate.adobe.com/runtime/4.0.0/edge.4.0.0.min.js
This does not measure usage of the constructor from a CSS string.

We also have data that DOMMatrix in Mozilla is very rarely used:
https://bugzilla.mozilla.org/show_bug.cgi?id=1346399#c3

However, WebkitCSSMatrix in Chrome is used in some way on 1% of pages, according to https://www.chromestatus.com/metrics/feature/popularity#WebKitCSSMatrix.

@foolip
Copy link
Member

foolip commented Apr 25, 2017

As reported by @chrishtr on blink-dev, he added separate use counters for setMatrixValue and the string constructor. They haven't reached the stable channel yet, but getting rid of the constructor does not look promising.

A related Chromium issue is https://bugs.chromium.org/p/chromium/issues/detail?id=703908

Since the string-parsing is already shipping for WebKitCSSMatrix or DOMMatrix in all engines, I propose to leave that intact and just limit it to the main thread. Concretely:

  • Add [Exposed=Window] to setMatrixValue
  • Let the DOMString constructor throw TypeError in workers (using prose)

(The idea is to behave as if the string constructor didn't exist, even though it does. If the third sequence<unrestricted double> numberSequence) variant wasn't there, I'd instead suggest behaving like the no-argument constructor.)

@foolip
Copy link
Member

foolip commented Apr 25, 2017

@esprehn, would you consider this issue resolved if the spec is changed as suggested in #122 (comment) or are there other options still?

@zcorpan
Copy link
Member

zcorpan commented Apr 25, 2017

Another option is #122 (comment) but that is nothing concrete and would probably need a bit of compat research.

@foolip
Copy link
Member

foolip commented Apr 25, 2017

I dunno. In a worker context there's arguably much less need for the parser, since the input probably comes from getComputedStyle(e).transform on the main thread, and could be parsed there before being passed on to the worker. Unless it does exactly the same thing as the CSS parser, there would also be odd cases where the same input doesn't produce the same output for the two parsers, which seems not great.

@zcorpan
Copy link
Member

zcorpan commented Apr 26, 2017

With "two parsers" you mean the result of new DOMMatrix(str) is different from e.style.transform = str ? Or do you mean main thread would use CSS parser for DOMMatrix but worker context would use a different one?

My idea would be to use a single parser for DOMMatrix that is always used.

@foolip
Copy link
Member

foolip commented Apr 26, 2017

I mean that new DOMMatrix(str) and e.style.transform = str would involve different parsers. Maybe it doesn't have to be that way, but I imagine the parser for the transform property is fully entangled in the rest of the CSS parser?

@zcorpan
Copy link
Member

zcorpan commented Apr 26, 2017

It is, and clearly we shouldn't change what e.style.transform = str does.

I agree that it would be a bit confusing if e.g. CSS comments wouldn't work in DOMMatrix when it does for transform, but there's also no use case for that that can't be solved by using JS comments instead. I think it's about equally confusing with differences between window and worker contexts.

That said, I'm happy to go either way, just wanted to make sure we consider the options at hand.

zcorpan added a commit that referenced this issue May 3, 2017
Annotate setMatrixValue with [Exposed=Window] and throw TypeError
when DOMMatrix/DOMMatrixReadOnly is constructed with a string
if the current global is not a Window.

Fixes #122.
@tabatkins tabatkins added the ready label May 3, 2017
zcorpan added a commit that referenced this issue May 3, 2017
Annotate setMatrixValue with [Exposed=Window] and throw TypeError
when DOMMatrix/DOMMatrixReadOnly is constructed with a string
if the current global is not a Window.

Fixes #122.

Tests: web-platform-tests/wpt#5769
@zcorpan
Copy link
Member

zcorpan commented May 3, 2017

Spec PR #141
Test PR web-platform-tests/wpt#5769

zcorpan added a commit that referenced this issue May 4, 2017
Annotate setMatrixValue with [Exposed=Window] and throw TypeError
when DOMMatrix/DOMMatrixReadOnly is constructed with a string
if the current global is not a Window.

Fixes #122.

Tests: web-platform-tests/wpt#5769
@tabatkins tabatkins removed the ready label May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants