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

[css-flexbox] delete flex-minimum-width-flex-items-007.xht #26250

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 23, 2020

It's failing in the same way in all browsers we test, with a 60x100
green rectangle, with the 100x100 red showing behind it:
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-007.xht?run_id=711240001&run_id=697770002&run_id=737900001&run_id=715540002&run_id=705490001
https://storage.googleapis.com/wptd-screenshots/sha1:a523515765355d9b677ae16706b1832c4ef65830.png

The only difference between flex-minimum-width-flex-items-007.xht and
-008.xht is that -007.xht uses 60x60-green.png and -008.xht uses
100x100-green.png. -008.xht passes everywhere:
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-008.xht?run_id=711240001&run_id=697770002&run_id=737900001&run_id=715540002&run_id=705490001

The assumption of the test appeared to be that the image aspect ratio
would be preserved. It seems things are not that simple, but a number
of other tests around aspect ratio were added in
w3c/csswg-test#1028.

It's failing in the same way in all browsers we test, with a 60x100
green rectangle, with the 100x100 red showing behind it:
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-007.xht?run_id=711240001&run_id=697770002&run_id=737900001&run_id=715540002&run_id=705490001
https://storage.googleapis.com/wptd-screenshots/sha1:a523515765355d9b677ae16706b1832c4ef65830.png

The only difference between flex-minimum-width-flex-items-007.xht and
-008.xht is that -007.xht uses 60x60-green.png and -008.xht uses
100x100-green.png. -008.xht passes everywhere:
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-008.xht?run_id=711240001&run_id=697770002&run_id=737900001&run_id=715540002&run_id=705490001

The assumption of the test appeared to be that the image aspect ratio
would be preserved. It seems things are not that simple, but a number
of other tests around aspect ratio were added in
w3c/csswg-test#1028.
@foolip
Copy link
Member Author

foolip commented Oct 23, 2020

This screenshot of the test in Firefox DevTools might help clarify what's happening:
image

@foolip
Copy link
Member Author

foolip commented Oct 23, 2020

@mrego would you mind reviewing this? I'm really a Flexbox novice, so it's hard for me to figure out if this is properly tested by some other test.

@davidsgrogan maybe you can check this too? I see that flex-minimum-width-flex-items-009.html which you added in #19533 is a very similar test, except that it uses min-height: 100px instead of height: 100px.

@mrego
Copy link
Member

mrego commented Oct 23, 2020

TBH I haven't been following this part of the spec closely, my tests there are quite old and I'm sure things have changed spec wise.

I'd like to know what @dholbert or @aethanyc think about these tests, as there were some recent changes in Firefox related to this: https://bugzilla.mozilla.org/show_bug.cgi?id=1316534#c24

@mrego
Copy link
Member

mrego commented Oct 23, 2020

Note also that flex-minimum-height-flex-items-007.xht only pass on Firefox which is suspicious too: https://wpt.fyi/results/css/css-flexbox/flex-minimum-height-flex-items-007.xht?run_id=711240001&run_id=697770002&run_id=737900001&run_id=715540002&run_id=705490001

@mrego
Copy link
Member

mrego commented Oct 23, 2020

CC @svillar and @Loirooriol just in case you have a better understanding of the expected behavior for these tests.

@cbiesinger
Copy link
Contributor

I think this test is correct? See https://drafts.csswg.org/css-flexbox/#min-size-auto:

However, if the box has an aspect ratio and no specified size suggestion, its content-based minimum size is the smaller of its content size suggestion and its transferred size suggestion

In this case, the transferred size suggestion is 100px (from height:100px and aspect-ratio), and the content size suggestion is the same thing because the min-content size of a replaced element is the one calculated from the aspect ratio. (cc @fantasai -- why does the spec separate transferred size and content size suggestions when they should always be identical?)

@foolip
Copy link
Member Author

foolip commented Oct 23, 2020

@cbiesinger All implementations appear to be failing this test in the same way, but it's a correct interpretation of the spec, is that what you mean? Should the spec change in this case, or what should we do?

@Loirooriol
Copy link
Contributor

Note the test was passing in Firefox until https://bugzilla.mozilla.org/show_bug.cgi?id=1316534
Then it started failing because of https://bugzilla.mozilla.org/show_bug.cgi?id=1665532
So Mozilla seems to think the test is correct:

The root cause is that when we calculating content size suggestion of a flex item image, we take the image's intrinsic size as the min-content width, and the width is currently not affected by the image's intrinsic aspect ratio and height. See the attached testcase for comparision.
w3c/csswg-drafts#5032 has more discussion on this topic.

@tabatkins
Copy link
Contributor

Yes, this test is correct due to the 'min-width/height: auto' default; the 100px height gets transferred across the ratio and should thus resolve to effectively "min-width: 100px".

@tabatkins
Copy link
Contributor

(cc @fantasai -- why does the spec separate transferred size and content size suggestions when they should always be identical?)

I don't understand what you mean by this - I don't see any reason they should be identical?

@cbiesinger
Copy link
Contributor

@tabatkins :

I don't understand what you mean by this - I don't see any reason they should be identical?

Because:

  1. Transferred size suggestion is the size computed using the aspect ratio
  2. Content size suggestion is the min-content size. The min-content size for an element with aspect-ratio is also calculated using the aspect-ratio, as per [css-sizing-4] Should aspect-ratio affect the intrinsic size? w3c/csswg-drafts#5032 (comment)

So, whenever a transferred size suggestion exists, it should be identical to the content size suggestion. No?

@tabatkins
Copy link
Contributor

Ah, right, ok, the Flexbox text predates that Sizing resolution. ^_^

@cbiesinger
Copy link
Contributor

cbiesinger commented Oct 23, 2020 via email

@davidsgrogan
Copy link
Member

davidsgrogan commented Oct 23, 2020

I think both flex-minimum-width-flex-items-007.xht and flex-minimum-height-flex-items-007.xht are correct.

Today's chrome dev release passes flex-minimum-height-flex-items-007.xht.

I have a local patch that makes chrome pass flex-minimum-width-flex-items-007.xht.

But, similar to what cbiesinger mentioned above, I was going to submit a spec issue about the redundancy in the flex spec between transferred size suggestion and content size suggestion, so I am very interested in Tab's answer to #26250 (comment) because that could affect my understanding here.

@tabatkins
Copy link
Contributor

No, I'm saying the redundancy is due to it being written before the Sizing resolution made it always occur.

@aethanyc
Copy link
Contributor

aethanyc commented Oct 24, 2020

Re: @Loirooriol
As noted in Firefox bug 1665532, I think the test is correct, and we shouldn't delete it. It's failing in currently Firefox Nightly 84 because of Bug 1665532.

Re: @davidsgrogan @cbiesinger
Coincidentally, I'm also trying to make Firefox pass flex-minimum-width-flex-items-007.xht, and flex-minimum-width-flex-items-013.html now looks wrong to me.

I'm also interested in the spec issue your going to file regarding the redundancy between transferred size suggestion and content size suggestion. It seems to me per w3c/csswg-drafts#5032 (comment), content size suggestion is the transferred size suggestion clamped by the definite max main size property in today's spec, so it makes transferred size suggestion redundant, doesn't it?

@davidsgrogan
Copy link
Member

@aethanyc Agreed that flex-minimum-width-flex-items-013.html is wrong. I'm glad you came to the same conclusion :)

Also agreed that transferred size suggestion is redundant. I disabled it locally in Blink and no tests failed.

@davidsgrogan
Copy link
Member

@aethanyc I also have .flexbox 5 in flex-aspect-ratio-img-column-011.html being wrong. Have you seen that one?

@aethanyc
Copy link
Contributor

Also agreed that transferred size suggestion is redundant. I disabled it locally in Blink and no tests failed.

@davidsgrogan I'm now less sure whether the transferred size suggestion is redundant, because flexbox has extra rules about the definiteness of a flex item's cross-size per spec here. Consider the following example:

<div style="display:flex; width:0; height: 50px;">
  <img style="" src="https://placehold.it/300x100" >
</div>

The image's cross-size is not specified, but it is definite because the flex container has height:50px. So the image's transfer size suggestion is 150px, and content size suggestion is its natural size 300px. The final size of the image should be 150x50, but removing transfer size suggestion yields 300x50.

I also have .flexbox 5 in flex-aspect-ratio-img-column-011.html being wrong. Have you seen that one?

Yes, I have! Do you have any plan to fix it?

@davidsgrogan
Copy link
Member

Oh, interesting example. Today FF Nightly gives the image 150px width but Chrome gives it 300px width. For transferred size, the spec says:

If the item has a preferred aspect ratio and its computed cross size property is definite

In 9.8, the definite rule says:

the outer cross size of any stretched flex items ... is considered definite.

Are computed cross size property and outer cross size equivalent for the purposes of this example? I don't know. Chrome considers them different. Firefox considers them the same. csswg-drafts issue needed!

For the broken tests, I have them fixed locally but haven't uploaded the patch for review yet. Hopefully tomorrow, but feel free to fix before me if you want.

@aethanyc
Copy link
Contributor

For the broken tests, I have them fixed locally but haven't uploaded the patch for review yet. Hopefully tomorrow, but feel free to fix before me if you want.

@davidsgrogan I haven't looked closely to the failed test yet, still hacking my implementation. I'm happy to wait for your fix sync to wpt since you've invested some efforts fixing them.

@foolip Sorry for hijack this issue to discuss other things. Shall we proceed with not deleting the test and closing this issue?

@foolip
Copy link
Member Author

foolip commented Oct 26, 2020

@aethanyc this sort of discussion is the best I could hope for when sending a naive PR like this, much better than it just being approved and merged :)

It sounds like additional cases were discovered, are tests for that being added somewhere now? (Sorry, didn't everything carefully.)

@davidsgrogan
Copy link
Member

davidsgrogan commented Oct 26, 2020

Agreed, the discussion here was fantastic.

When w3c/csswg-drafts#5663 is resolved, I will ensure that the relevant test is added.

@foolip
Copy link
Member Author

foolip commented Oct 26, 2020

Thanks @davidsgrogan, I'll go ahead and close this now.

@foolip foolip closed this Oct 26, 2020
@foolip foolip deleted the foolip/rm-flex-minimum-width-flex-items-007 branch October 26, 2020 21:57
@davidsgrogan
Copy link
Member

@aethanyc Those test fixes were merged an hour ago in #26299

@aethanyc
Copy link
Contributor

@davidsgrogan Wonderful! The fixed tests now pass with my patch!

@foolip
Copy link
Member Author

foolip commented Oct 29, 2020

Wow, that went well. I'm encouraged, so I filed another naive issue making guesses about what might be wrong with another test: #26326

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

8 participants