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

New flex aspect ratio test #26617

Merged
merged 3 commits into from Nov 24, 2020
Merged

Conversation

svillar
Copy link
Contributor

@svillar svillar commented Nov 23, 2020

This was inspired by the case case in this WebKit bug. Basically we have a flex item with aspect ratio with specified percentage width (the percentage is actually not relevant as the width of the flex container is fixed). WebKit was stretching the image on the block axis till reaching the viewport size because the height of the container was auto and it was not considering the aspet ratio.

Copy link
Member

@mrego mrego left a comment

Choose a reason for hiding this comment

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

The test looks good, but it needs some changes as it's pointing to the wrong reference file.

<title>CSS Flexible Box Test: Aspect ratio handling of images</title>
<link rel="author" title="Sergio Villar Senin" href="mailto:svillar@igalia.com" />
<link rel="help" href="https://drafts.csswg.org/css-flexbox-1/#algo-cross-item" />
<link rel="match" href="reference/flex-aspect-ratio-img-row-014.xht" />
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't match the name of the expected file. Please rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See most recent patch

@@ -0,0 +1,26 @@
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
Copy link
Member

Choose a reason for hiding this comment

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

-expected is common in WebKit, but not really in WPT.

BTW, why are you using XHTML here instead of just HTML with '` like in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm just because I use some other as reference to create the expectation.

<style type="text/css"><![CDATA[
span
{
display: inline-block;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Really weird format/indentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, using similar style as other flex-aspect-ratio-img-*tests. I'll change that.

Copy link
Member

@mrego mrego left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for applying the changes.

@svillar svillar merged commit 69ec2f6 into web-platform-tests:master Nov 24, 2020
@svillar svillar deleted the flex-ratio-row-14 branch November 24, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants