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
New flex aspect ratio test #26617
Conversation
There was a problem hiding this 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" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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.