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.

Sign up for GitHub

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

Jump to bottom

[css-sizing-4] How to handle 0 in aspect-ratio #5557

Closed
cbiesinger opened this issue Sep 29, 2020 · 10 comments
Closed

[css-sizing-4] How to handle 0 in aspect-ratio #5557

cbiesinger opened this issue Sep 29, 2020 · 10 comments
Labels
Closed Accepted by CSSWG Resolution css-sizing-4 Tested Memory aid - issue has WPT tests

Comments

@cbiesinger
Copy link

https://drafts.csswg.org/css-sizing-4/#aspect-ratio

I just noticed that the spec was changed to allow a lot more values. In particular, 0 is now valid for aspect-ratio, but the spec doesn't say how to handle that. I assume it behaves as "auto" but still gets computed as specified?
@cbiesinger
Copy link
Author

Why did you not make it compute to auto? (Also, I don't recall this entire change being discussed in the WG?)

@cbiesinger
Copy link
Author

@BorisChiou @bfgeek @tabatkins @fantasai

@BorisChiou
Copy link
Contributor

There is a comment about n/0 and 0/n for aspect-ratio in media query: #3757. And yes, it'd be great to address more about how to handle 0/n and n/0 in the spec.

@tabatkins
Copy link
Member

It doesn't say how to handle it because there's nothing special to do with it - it acts exactly like you'd expect, similar to values close to it. That is, 0/x acts similar to 0.000000001/x, and same for x/0. Nothing wrong with zeros or infinities here; they're the same as very-small nonzero values or very-large finite values.

Why did you not make it compute to auto?

They don't compute to auto because it would violate our 'no open ranges' restriction (giving a substantially different behavior between 0 and .000000000001, potentially exposing UA-specific rounding). Also, there's no reason to, since the behavior at 0 has a simple, clean definition as the limit behavior of it approaching zero.

(Also, I don't recall this entire change being discussed in the WG?)

It's just a side-effect of us changing ratios from <integer> to <number>, which we did discuss. I'd have to dig up the issue, but it was definitely discussed in a f2f as well. When it was <integer> we could disallow zero, but as soon as you allow numbers, you have to allow zero because of no-open-ranges.

@tabatkins tabatkins added the Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. label Sep 29, 2020
@tabatkins
Copy link
Member

(See #4954 for some other discussion on this, including in particular why we defined 0/0 the way we did.)

pull bot pushed a commit to Alan-love/chromium that referenced this issue Oct 1, 2020
@cbiesinger
[AspectRatio] Update the parsing of aspect-ratio
ff0aadc 
There have been a number of spec changes in parsing aspect-ratio:
https://drafts.csswg.org/css-sizing-4/#aspect-ratio
https://drafts.csswg.org/css-values-4/#ratio-value

In particular:
- "auto <ratio>" is now a supported syntax, where the ratio is only used as
  a fallback if there is no intrinsic ratio
- The ratio is now a non-negative number instead of a positive integer
- The second component of the ratio is optional

Note that I believe the last test in contain-intrinsic-size-valid to be
incorrect; by my reading only the computed value should be affected,
not the serialization.

Note open issues:
w3c/csswg-drafts#5557
w3c/csswg-drafts#5084

[email protected], [email protected], [email protected]

Bug: 1045668, 1083010, 1086606
Change-Id: Iba3588ed98beacd02f77f3bcc19e387b3add4a5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437698
Reviewed-by: Xiaocheng Hu <[email protected]>
Reviewed-by: Morten Stenshorne <[email protected]>
Commit-Queue: Christian Biesinger <[email protected]>
Auto-Submit: Christian Biesinger <[email protected]>
Cr-Commit-Position: refs/heads/master@{#812403}
@fantasai fantasai mentioned this issue Oct 9, 2020
Closed
@fantasai
Copy link
Collaborator

fantasai commented Oct 9, 2020

Reopened #4954 since as @cbiesinger points out, this wasn't resolved by the WG and the resolution isn't obvious.

@fantasai fantasai reopened this Nov 3, 2020
@fantasai fantasai removed the Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. label Nov 3, 2020
@fantasai
Copy link
Collaborator

fantasai commented Nov 3, 2020

RESOLVED: Treat aspect ratio of 0 or infinity as auto on Thursday 22 October 2020

@mozilla-apprentice mozilla-apprentice mentioned this issue Nov 3, 2020
Closed
@fantasai
Copy link
Collaborator

fantasai commented Nov 5, 2020

Minutes: https://lists.w3.org/Archives/Public/www-style/2020Nov/0003.html

@fantasai fantasai added Closed Accepted by CSSWG Resolution Tested Memory aid - issue has WPT tests labels Nov 5, 2020
@fantasai
Copy link
Collaborator

fantasai commented Nov 5, 2020

wpt: css/css-sizing/aspect-ratio/zero-or-infinity-*

@tabatkins
Copy link
Member

Fixed in 84a573d

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@cbiesinger @copybara-github
[AspectRatio] Update the parsing of aspect-ratio
2207d8b 
There have been a number of spec changes in parsing aspect-ratio:
https://drafts.csswg.org/css-sizing-4/#aspect-ratio
https://drafts.csswg.org/css-values-4/#ratio-value

In particular:
- "auto <ratio>" is now a supported syntax, where the ratio is only used as
  a fallback if there is no intrinsic ratio
- The ratio is now a non-negative number instead of a positive integer
- The second component of the ratio is optional

Note that I believe the last test in contain-intrinsic-size-valid to be
incorrect; by my reading only the computed value should be affected,
not the serialization.

Note open issues:
w3c/csswg-drafts#5557
w3c/csswg-drafts#5084

[email protected], [email protected], [email protected]

Bug: 1045668, 1083010, 1086606
Change-Id: Iba3588ed98beacd02f77f3bcc19e387b3add4a5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437698
Reviewed-by: Xiaocheng Hu <[email protected]>
Reviewed-by: Morten Stenshorne <[email protected]>
Commit-Queue: Christian Biesinger <[email protected]>
Auto-Submit: Christian Biesinger <[email protected]>
Cr-Commit-Position: refs/heads/master@{#812403}
GitOrigin-RevId: ff0aadc1166c6ed6508d726c01766ec242098136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Closed Accepted by CSSWG Resolution css-sizing-4 Tested Memory aid - issue has WPT tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tabatkins @fantasai @cbiesinger @BorisChiou