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 stringifier behavior doesn't seem to match implementations #120

Closed
bzbarsky opened this issue Mar 13, 2017 · 11 comments
Assignees

Comments

@bzbarsky
Copy link

bzbarsky commented Mar 13, 2017

The spec says to just append the values of the (double) attributes to the string. But implementations seem to be appending as floats or something; they are certainly appending a much lower precision than a double would give.

@foolip
Copy link
Member

foolip commented Apr 25, 2017

@cabanier, any ideas for how to align spec with Blink and other implementations?

@gogag2
Copy link

gogag2 commented Apr 26, 2017

I'd like to change Blink following the spec.
Link: https://codereview.chromium.org/2846523002/

@zcorpan
Copy link
Member

zcorpan commented May 4, 2017

The patch has this change to a test

-  assert_equals(matrix2d.toString(), "matrix(1, 2, 3, 3.1, 2, 1)");
+  assert_equals(matrix2d.toString(), "matrix(1.000000, 2.000000, 3.000000, 3.100000, 2.000000, 1.000000)");

It is not clear to me that this is a desirable result.

https://html.spec.whatwg.org/multipage/infrastructure.html#best-representation-of-the-number-as-a-floating-point-number uses JS's ToString, which doesn't produce trailing zeros. But can still represent double precision, right?

Related is how to serialize non-finite values. NaN and Infinity are invalid CSS keywords, so it wouldn't successfully parse again...

@zcorpan
Copy link
Member

zcorpan commented May 4, 2017

There's also https://drafts.csswg.org/cssom/#serialize-a-css-component-value :

<number>
A base-ten number using digits 0-9 (U+0030 to U+0039) in the shortest form possible, using "." to separate decimals (if any), rounding the value if necessary to not produce more than 6 decimals, preceded by "-" (U+002D) if it is negative.
Note: scientific notation is not used.

@gogag2
Copy link

gogag2 commented May 8, 2017

Could I use String::NumberToStringECMAScript(double number) function?
I think it can represent double precision and serialize non-finite values.
Could you review my code?
https://codereview.chromium.org/2846523002/

Thanks.

@zcorpan zcorpan added the Agenda+ label May 9, 2017
@zcorpan zcorpan changed the title [geometry] stringifier behavior doesn't seem to match implementations [geometry] DOMMatrix stringifier behavior doesn't seem to match implementations May 9, 2017
@zcorpan
Copy link
Member

zcorpan commented May 9, 2017

I think using JS ToString is reasonable here, notwithstanding that non-finite values fail to roundtrip (that is already the case in Chromium and Gecko anyway). Adding Agenda+ (and emailed w3c-css-wg) to discuss this on the CSSWG telecon tomorrow.

@css-meeting-bot
Copy link
Member

css-meeting-bot commented May 10, 2017

The CSS Working Group just discussed DOMMatrix stringifier behavior doesn't seem to match implementations, and agreed to the following resolutions:

  • RESOLVED: In the case of NaN we would throw an exception during serialization.
The full IRC log of that discussion <dael> Topic: DOMMatrix stringifier behavior doesn't seem to match implementations
<dael> github topic: https://github.com//issues/120
<dael> zcorpan: DOM Matrix has [missed] for parsing a string. The string is CSS syntax. Values are unrestricted doubles so you can have non and infinity and it's unclear whaht the serialized is to do. If it teruns non and infitity with JS.
<smfr> s/non/NaN/
<dael> zcorpan: I think the most reasonable things is to just use script to string.
<zcorpan> s/script/ECMAScript/
<dael> TabAtkins: I asked in the related thread what's the benefit of allowing infinity and NaN in CSS and I'm not sure how the thread on DOMMRect you pointed to answer the question.
<dbaron> my telephone connection apparently isn't good enough for webex to understand the pin
<dael> TabAtkins: At the bottom of fxtf draft you reference the issue 1343 in CSS.
<dael> TabAtkins: You prop CSS syntax should allow infinity and NaN so DOMMatric can round trip. I asked what was the benefit and you pointed to another thread.
<dael> zcorpan: It does cover DOMMatrixin that thread. It's part of the thread.
<dael> zcorpan: It's with one level of quotation for the reason of using unrestricted double i n geometry APIs.
<smfr> TabAtkins is referring to https://github.com/w3c/csswg-drafts/issues/1343
<dael> TabAtkins: What transforms can produce a NaN or infinity in here?
<dael> zcorpan: I'm not sure if it's possible.
<dael> TabAtkins: We don't allow the values. It can't be possible b/c then we'd have transforms without matrix.
<dael> ChrisL: You can generate one and reserialize it.
<dael> zcorpan: Exactly. If you do 1/0 it's an error now but it could be infinity instead.
<dael> TabAtkins: It could in theory but it doesn't yet and we don't have a proposal to do it yet. afaict the only thigns that can produce NaN or infinity are operations we don't allow. I'm still not clear on the benefit of allowing CSS to recognize them because it doesn't mean anything in CSS> IF you gave this to transform what would it do?
<dael> zcorpan: I don't know. How matrix work is above my head, but you can end up with NaN and infinity so I wanted to get a resolution on how this would parse. I don't have a use case for it.
<dael> TabAtkins: Alternative is in other cases where you can get into a state with a value that cannot be represented we jsut give up and do the empty string when you serialize. Can we do that? If you as for the CSS Serialization of a string with NaN or infinity you get an empty string.
<dael> zcorpan: Reasonable. I can live with that.
<dael> smfr: I know we're getting an empty string...
<dael> zcorpan: If you parse it again you get an indednty matrix.
<dael> Rossen: zcorpan you're okay with TabAtkins resolution I think. Is there anything else to explore on this?
<dael> zcorpan: Consensus on TabAtkins proposal sounds good.
<smfr> smfr: ; I don’t think think that returning an empty string is developer-friendly
<smfr> authors won’t know why they got an empty string
<dael> TabAtkins: smfr is disagreeing. Thanks for clarifying.
<dael> TabAtkins: I don't know what else you can do, though. The to string of DOM produces CSS matrix. If you have a matrix that can't be a valid CSS transform, what are you supposed to do?
<dael> smfr: Throw exception.
<dael> zcorpan: What happens in Chrome & Gecko you serialize NaN. When you parse you get an error.
<dael> Rossen: I still don't get what happens when you try and roundtrip. What's the expected behavior when you parse back.
<dael> TabAtkins: Yeah, it fails to parse.
<zcorpan> SyntaxError
<dael> Rossen: So if this was an error on input, why not an error on output?
<dael> TabAtkins: I'm okay with throwing as well. Just something that indicates the to string failed.
<dael> Rossen: That's what I'd lean towards as well.
<dael> Rossen: Let's try to resolve. Prop: In the case of NaN we would throw an exception during serialization.
<dael> Rossen: Objections?
<dael> RESOLVED: In the case of NaN we would throw an exception during serialization.
<zcorpan> InvalidStateError?

@zcorpan
Copy link
Member

zcorpan commented May 10, 2017

Spec PR: #148

(I'll try to work on tests tomorrow.)

@AmeliaBR
Copy link

AmeliaBR commented May 10, 2017

Since it came up in the discussion, one practical case where a DOMMatrix would have infinite values is skewX(90deg) or skewY(90deg). That's a non-invertable operation, and in the transform property it causes the element to not be drawn (similar to scale(0)). But it is still a valid transform function, and could reasonably occur as a start or end point in an animation.

Note: I just tested, and neither Chrome nor Firefox currently actually use Infinity in the matrix new DOMMatrix().skewX(90), instead they use 1.63312e+16. But if the trigonometric functions were mathematically precise, it would be Infinity. (The c component of the skewX(90) matrix is equal to Math.tan(Math.PI/2).)

The other cases where you you'd get non-finite values in a matrix are where you tried to invert a non-invertable matrix (like scale(1,0)): as currently spec'd, this does not throw an error, but instead sets all values in the matrix to NaN. (In contrast SVG 1.1 threw an error when trying to invert a non-invertable matrix.) And of course, the author could be setting individual matrix or transform parameters using calculations that return Infinity (1/0) or NaN (0/0).

You wouldn't expect these matrices to have CSS-parse-able equivalents, though. Skews are a special case because the non-finite matrix representation has a finite transform-function representation.

@AmeliaBR
Copy link

AmeliaBR commented May 10, 2017

Also, so it doesn't get forgotten: The main substance of this issue still needs to be resolved.

When you stringify a matrix to CSS notation, do you follow the "6-decimals & no scientific notation rule" from CSSOM, or do you maintain full double precision, or what? Currently, Firefox and Chrome seem to use 6-digit precision (which isn't the same as 6-decimals) with scientific notation:

m = new DOMMatrix();
m.a = 1/3;
m.b = 1/300000;
m.c = 100000 + (1/3);
m.d = Number.MAX_SAFE_INTEGER;
"" + m;
// Chrome 58 & Firefox 54 both return: 
// "matrix(0.333333, 3.33333e-06, 100000, 9.0072e+15, 0, 0)"

@zcorpan
Copy link
Member

zcorpan commented May 11, 2017

@AmeliaBR thanks for the examples for Infinity and NaN!

I missed to bring up on the call the precision issue. In #148 I defined it in terms of JS's ToString, so that means something like this:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5149

It seems in Safari TP for WebKitCSSMatrix, it uses 6 digits and no sci-not:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5150

None of the current browser implementations match CSSOM's serialization rule for numbers.

I think JS ToString is reasonable because it makes it possible to preserve precision in a roundtrip, if the CSS parser supports parsing into a double value for DOMMatrix/'transform' etc.

@tabatkins tabatkins removed the ready label May 16, 2017
MXEBot pushed a commit to mirror/chromium that referenced this issue Jun 7, 2017
They are appending a much lower precision than a double. It should append the
value of the double atrributes to the string.

Link: w3c/fxtf-drafts#120

BUG=none

Review-Url: https://codereview.chromium.org/2846523002
Cr-Commit-Position: refs/heads/master@{#477328}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 11, 2019
Fixed the stringifier as the spec mandates [using full double precision](w3c/fxtf-drafts#120 (comment)).

Differential Revision: https://phabricator.services.mozilla.com/D35593

--HG--
extra : moz-landing-system : lando
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jul 11, 2019
Fixed the stringifier as the spec mandates [using full double precision](w3c/fxtf-drafts#120 (comment)).

Differential Revision: https://phabricator.services.mozilla.com/D35593
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
Fixed the stringifier as the spec mandates [using full double precision](w3c/fxtf-drafts#120 (comment)).

Differential Revision: https://phabricator.services.mozilla.com/D35593

UltraBlame original commit: 0263d9e2b7e0ed1e6414891a2c61186984bf9200
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
Fixed the stringifier as the spec mandates [using full double precision](w3c/fxtf-drafts#120 (comment)).

Differential Revision: https://phabricator.services.mozilla.com/D35593

UltraBlame original commit: 0263d9e2b7e0ed1e6414891a2c61186984bf9200
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
Fixed the stringifier as the spec mandates [using full double precision](w3c/fxtf-drafts#120 (comment)).

Differential Revision: https://phabricator.services.mozilla.com/D35593

UltraBlame original commit: 0263d9e2b7e0ed1e6414891a2c61186984bf9200
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

7 participants