Skip to content

[css-values] Calc serialization algorithm is not web compatible #1731

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

Closed
emilio opened this issue Aug 17, 2017 · 13 comments
Closed

[css-values] Calc serialization algorithm is not web compatible #1731

emilio opened this issue Aug 17, 2017 · 13 comments

Comments

@emilio
Copy link
Collaborator

emilio commented Aug 17, 2017

In https://drafts.csswg.org/css-values/#calc-serialize, section 3 says:

Otherwise, serialize as a calc() containing the summation, with the units ordered ASCII case-insensitive alphabetically, the number (if present) coming before all units, and the percentage (if present) coming after all units.

Servo implements this, and when testing it on Firefox, we've come across web compat bugs, in particular: https://bugzilla.mozilla.org/show_bug.cgi?id=1390496

That's a case where something has calc(100% - 2px).

Blink and Gecko serialize it as written. Servo serializes it as calc(-2px + 100%), following that section of the spec, and the page tries to parse that and messes it up, causing flickering. Not sure about what's Edge's behavior.

Needs like we need another algorithm for serializing calc() values?

@tabatkins
Copy link
Member

Hmm, I wonder if we can hack around this and just swap % to the front? The relative ordering is completely unimportant to me, it just sounded nice when I wrote it.

@emilio
Copy link
Collaborator Author

emilio commented Aug 17, 2017

We could try, I guess, though if we find any other issue we'll have to go with the more conservative approach of what other browsers do. cc @canaltinova

@canova
Copy link
Member

canova commented Aug 17, 2017

Well, this workaround seems simple enough but are we sure about not doing what other major browsers do? I can't think of an issue about that but may cause problems in the future?

@emilio
Copy link
Collaborator Author

emilio commented Aug 17, 2017

Let's try that. We also need to fix other similar calc serialization issues, so if changing that brings other issues we'll fall back to preserve the order of expressions as other browsers.

It'd be good to file bugs for other browsers too...

@tabatkins
Copy link
Member

The "what other browsers do" is just relaying back the precise value the author entered, right? I think we just need to capture that in general, maybe; it seems like browsers do it for most specified values.

@emilio
Copy link
Collaborator Author

emilio commented Aug 17, 2017

Not really, Gecko does this to some extent, Blink simplifies, but only adjacent expressions... It's quite a mess :)

@tabatkins
Copy link
Member

Ah, then I think the best approach is indeed to try and do something simple that just happens to be web-compatible. ^_^

bors-servo pushed a commit to servo/servo that referenced this issue Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18131)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18131)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18131)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 18, 2017
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 0432018b2542dbbba240aeda1c59e0002ad9e6f9
bholley pushed a commit to bholley/gecko that referenced this issue Aug 19, 2017
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Aug 21, 2017
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519
@emilio
Copy link
Collaborator Author

emilio commented Sep 1, 2017

@tabatkins, could we get the spec updated?

Thanks!

@tabatkins
Copy link
Member

To the workaround I noted (just swapping % to the front)?

@emilio
Copy link
Collaborator Author

emilio commented Sep 1, 2017

Yup!

@foolip
Copy link
Member

foolip commented Oct 2, 2017

Is there a test for this in web-platform-tests now? This seems especially important for web compat issues.

@emilio
Copy link
Collaborator Author

emilio commented Dec 1, 2017

@foolip I didn't upstream it in the beginning because the spec was under discussion... But I guess I'll do, just opened servo/servo#19449

@foolip
Copy link
Member

foolip commented Dec 1, 2017

Thanks @emilio!

bors-servo pushed a commit to servo/servo that referenced this issue Dec 1, 2017
style: Upstream calc serialization test.

And test w3c/csswg-drafts#1731 too, since it was fixed.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19449)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519

UltraBlame original commit: c7832d380dcee31f16bbf2da98ee6cac0002cca4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519

UltraBlame original commit: c7832d380dcee31f16bbf2da98ee6cac0002cca4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…emilio:calc-serialization); r=canaltinova

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

Source-Repo: https://github.com/servo/servo
Source-Revision: 92176d1152257e57e81dcd9204d59348ce9ff519

UltraBlame original commit: c7832d380dcee31f16bbf2da98ee6cac0002cca4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants