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

Test aspect-ratio affects the min/max sizes #34738

Merged
merged 2 commits into from Jul 12, 2022
Merged

Test aspect-ratio affects the min/max sizes #34738

merged 2 commits into from Jul 12, 2022

Conversation

cathiechen
Copy link
Contributor

Hi,
This patch adds tests for the discussion in w3c/csswg-drafts#7461

According to the discussion, we have this priorities order list:

davidsgrogan
davidsgrogan approved these changes Jul 8, 2022
View reviewed changes
Copy link
Member

@davidsgrogan davidsgrogan left a comment

Choose a reason for hiding this comment

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

Behavior assertions all LGTM. The many review comments are all about the metadata and comments.

Thanks for writing these. (Hopefully our understanding is correct! But if not, these tests will give us something concrete to discuss.)

css/css-sizing/aspect-ratio/block-aspect-ratio-038.html

<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>

<div style="background: green; height: 100px; aspect-ratio: 2 / 1; max-width: 100px;">
Copy link
Member

Choose a reason for hiding this comment

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

height:100px transferred through aspect-ratio: 2 / 1 says to give it a 200px width, the auto min size says to give it a 200px width, but max-width should beat both of those. LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review:)

css/css-sizing/aspect-ratio/block-aspect-ratio-040.html Outdated
<!DOCTYPE html>
<title>CSS aspect-ratio: The definite max-height should win the transferred minimum height</title>
<link rel="author" title="Cathie Chen" href="mailto:cchen@igalia.com">
<link rel="help" href="https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum">
Copy link
Member

Choose a reason for hiding this comment

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

Change this reference to https://drafts.csswg.org/css-sizing-4/#aspect-ratio-size-transfers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

css/css-sizing/aspect-ratio/block-aspect-ratio-040.html Outdated
<link rel="author" title="Cathie Chen" href="mailto:cchen@igalia.com">
<link rel="help" href="https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum">
<link rel="match" href="../../reference/ref-filled-green-100px-square.xht" />
<meta name="assert" content="CSS aspect-ratio: The definite max-height should win the transferred minimum height.">
Copy link
Member

Choose a reason for hiding this comment

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

There's no transferred minimum height here. Maybe you meant this as transferred maximum height? Same applies to line 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

css/css-sizing/aspect-ratio/block-aspect-ratio-041.html Outdated
<!DOCTYPE html>
<title>CSS aspect-ratio: The definite max-width should win the transferred minimum width</title>
<link rel="author" title="Cathie Chen" href="mailto:cchen@igalia.com">
<link rel="help" href="https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum">
Copy link
Member

Choose a reason for hiding this comment

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

Change this reference as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

css/css-sizing/aspect-ratio/block-aspect-ratio-041.html Outdated
<link rel="author" title="Cathie Chen" href="mailto:cchen@igalia.com">
<link rel="help" href="https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum">
<link rel="match" href="../../reference/ref-filled-green-100px-square.xht" />
<meta name="assert" content="The definite max-width should win the transferred minimum width.">
Copy link
Member

Choose a reason for hiding this comment

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

s/minimum/maximum/ here and line 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

css/css-sizing/aspect-ratio/block-aspect-ratio-042.html Outdated
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<title>CSS aspect-ratio: The transferred minimum height beats the automatic content-based minimum height</title>
Copy link
Member

Choose a reason for hiding this comment

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

Here and Line 6 below, I think you mean "transferred maximum".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

css/css-sizing/aspect-ratio/block-aspect-ratio-043.html Outdated
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<title>CSS aspect-ratio: The transferred minimum width beats the automatic content-based minimum width</title>
Copy link
Member

Choose a reason for hiding this comment

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

Transferred maximum here and line 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cathiechen cathiechen merged commit d169dde into web-platform-tests:master Jul 12, 2022
@cathiechen cathiechen deleted the aspect-ratio-and-min-max-sizes branch July 12, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidsgrogan davidsgrogan davidsgrogan approved these changes

@fantasai fantasai Awaiting requested review from fantasai

@tabatkins tabatkins Awaiting requested review from tabatkins

@rwlbuis rwlbuis Awaiting requested review from rwlbuis

@BorisChiou BorisChiou Awaiting requested review from BorisChiou

Assignees

@tabatkins tabatkins

Labels
css-sizing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cathiechen @davidsgrogan @tabatkins @wpt-pr-bot