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

Mapping to aspect-ratio with zero dimension. #6527

Closed
emilio opened this issue Mar 24, 2021 · 10 comments · Fixed by #6529
Closed

Mapping to aspect-ratio with zero dimension. #6527

emilio opened this issue Mar 24, 2021 · 10 comments · Fixed by #6529

Comments

@emilio
Copy link
Contributor

emilio commented Mar 24, 2021

https://html.spec.whatwg.org/#map-to-the-aspect-ratio-property uses https://html.spec.whatwg.org/#rules-for-parsing-non-zero-dimension-values, which means that width=0 or height=0 shouldn't map to aspect-ratio, but that doesn't match Chrome's implementation nor the WPT tests.

Given how this should work in CSS is well defined, and that in #4961 we probably want to use the attributes for <canvas> (whose width / height attributes have different parsing rules, https://html.spec.whatwg.org/#rules-for-parsing-non-negative-integers), I suggest the aspect-ratio mapping code should not re-parse the attribute value with different rules.

@emilio
Copy link
Contributor Author

emilio commented Mar 24, 2021

cc @cbiesinger @tabatkins

@emilio
Copy link
Contributor Author

emilio commented Mar 24, 2021

This is also relevant because some tokens are valid when parsing as non-negative integers but not vice versa, so dimensions aren't always a super-set of what the attribute will parse.

@emilio
Copy link
Contributor Author

emilio commented Mar 24, 2021

I'm ambivalent on whether zero should be allowed or not, but we should parse the attributes with the same rules we use for the element (this is relevant for <canvas>).

@annevk
Copy link
Member

annevk commented Mar 24, 2021

So interesting test cases here would be:

  • +1
  • 1.5

Are there any other differences?

@emilio
Copy link
Contributor Author

emilio commented Mar 24, 2021

10% is valid (and means 10) as per the integer parsing rules, but not valid as the dimension parsing rules.

@cbiesinger
Copy link

I agree that the rules should be the same for the two. But what do you mean with "re-parse"? Whether parsing happens twice is just an implementation issue, right?

I'm certainly happy to change the attribute mappings to disallow zero, and to use ParseHTMLNonNegativeInteger for canvas. Or whatever the outcome here is.

@emilio
Copy link
Contributor Author

emilio commented Mar 24, 2021

I agree that the rules should be the same for the two. But what do you mean with "re-parse"? Whether parsing happens twice is just an implementation issue, right?

Yeah, true. I generally mean "parsing with different algorithms for different purposes". I don't care about parsing multiple times if the result is the same of course :-)

I'm certainly happy to change the attribute mappings to disallow zero, and to use ParseHTMLNonNegativeInteger for canvas. Or whatever the outcome here is.

I don't mind so much about which direction we go in the zero issue, I just noticed it diverged from the spec. But the integer vs. dimension issue I think is worth solving.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 24, 2021
… and <video>.

As per https://html.spec.whatwg.org/#attributes-for-embedded-content-and-images:

> The width and height attributes map to the aspect-ratio property on
> img, canvas, and video elements, and input elements with a type
> attribute in the Image Button state.

See whatwg/html#6527 for the parsing issue
with canvas and zero. For now allow both behaviors in the tests.

We also remove the width-and-height-map-to-aspect-ratio pref, as it is
true everywhere and has been for a while.

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1700640
gecko-commit: 985b66b916980b78328063dfdccdc5e5b71318e5
gecko-reviewers: boris
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 25, 2021
…t type=image>, and <video>. r=boris

As per https://html.spec.whatwg.org/#attributes-for-embedded-content-and-images:

> The width and height attributes map to the aspect-ratio property on
> img, canvas, and video elements, and input elements with a type
> attribute in the Image Button state.

See whatwg/html#6527 for the parsing issue
with canvas and zero. For now allow both behaviors in the tests.

We also remove the width-and-height-map-to-aspect-ratio pref, as it is
true everywhere and has been for a while.

Differential Revision: https://phabricator.services.mozilla.com/D109618
@annevk
Copy link
Member

annevk commented Mar 25, 2021

Okay, so if we want to parse consistently, we'd indeed move to the zero-allowing dimension parser for img, video, etc. and use the non-negative integer parser for canvas. Is that the proposal?

I can do a PR if that sounds good as I see we already have some tests.

@emilio
Copy link
Contributor Author

emilio commented Mar 25, 2021

Yes, I think so. That'd be great. Note that the tests in web-platform-tests/wpt#28229 would need to be adjusted to not have the double expectation, but that's a very simple change.

annevk added a commit that referenced this issue Mar 25, 2021
Also map <canvas width height> to aspect-ratio consistently with how the attributes are parsed.

Tests: web-platform-tests/wpt#28229.

Follow-up: #6528.

Closes #4961 and closes #6527.
@annevk
Copy link
Member

annevk commented Mar 25, 2021

Created #6529.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 26, 2021
… and <video>.

As per https://html.spec.whatwg.org/#attributes-for-embedded-content-and-images:

> The width and height attributes map to the aspect-ratio property on
> img, canvas, and video elements, and input elements with a type
> attribute in the Image Button state.

See whatwg/html#6527 for the parsing issue
with canvas and zero. For now allow both behaviors in the tests.

We also remove the width-and-height-map-to-aspect-ratio pref, as it is
true everywhere and has been for a while.

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1700640
gecko-commit: 985b66b916980b78328063dfdccdc5e5b71318e5
gecko-reviewers: boris
annevk added a commit that referenced this issue May 12, 2021
Also map <canvas width height> to aspect-ratio consistently with how the attributes are parsed and clean up some wording.

Tests: web-platform-tests/wpt#28229 & web-platform-tests/wpt#28932.

Follow-up: #6528.

Closes #4961 and closes #6527.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants