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

Support zero dimension values for aspect-ratio #6529

Merged
merged 3 commits into from May 12, 2021

Conversation

annevk
Copy link
Member

@annevk annevk commented 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.
@domenic
Copy link
Member

domenic commented Mar 31, 2021

Ping @cbiesinger @emilio for their subject-expert review; I can do editorial review but I apparently didn't spot previous flaws in this area so your help would be much appreciated to make sure we get it right this time :)

@annevk
Copy link
Member Author

annevk commented Apr 12, 2021

@cbiesinger @emilio ping.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

annevk added a commit to web-platform-tests/wpt that referenced this pull request May 10, 2021
@annevk
Copy link
Member Author

annevk commented May 10, 2021

I created web-platform-tests/wpt#28932 to update the tests and filed https://bugs.chromium.org/p/chromium/issues/detail?id=1207330 against Chrome.

Safari has a lot more failures, despite https://bugs.webkit.org/show_bug.cgi?id=201641 being marked fixed. Is that expected @smfr? (Edit: filed https://bugs.webkit.org/show_bug.cgi?id=225648 meanwhile.)

@annevk annevk requested a review from domenic May 10, 2021 11:55
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, would be good to clean up the source paragraph while you're there.

source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented May 11, 2021

That sentence is rather weird. I wish we had made that flow from the img element itself (as the picture element works in general), rather than in this roundabout way. As in, it seems this should have been a change to the source set to also list these sizes.

@annevk annevk requested a review from domenic May 11, 2021 11:17
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression as to why this sentence is a bit weird is that it's a statement of fact about the normative requirements in the rendering section. Since it's in the source section, it's source-element centric and not super-precise.

@zcorpan
Copy link
Member

zcorpan commented May 11, 2021

@domenic yep. It could technically be removed altogether without changing what the spec requires, and so from that perspective there's flexibility in how to word it. It could also be changed to a note to further signal its non-normative nature.

source Outdated Show resolved Hide resolved
annevk added a commit to web-platform-tests/wpt that referenced this pull request May 12, 2021
@annevk annevk merged commit e2f08b4 into main May 12, 2021
@annevk annevk deleted the annevk/aspect-ratio-mapping branch May 12, 2021 13:06
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 14, 2021
…s, a=testonly

Automatic update from web-platform-tests
HTML: update aspect-ratio rendering tests

For whatwg/html#6529.

--

wpt-commits: 8d656c56e3fe778ed251a866290529b369ddd939
wpt-pr: 28932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants