Bug 234361 – [REGRESSION][[css-flexbox] child elements are shrunk to fit into container after r286206

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yury Semikhatsky]

Created attachment 447272 [details]
example page

After https://github.com/WebKit/WebKit/commit/bdb68ddedc91dba21084068424ed93bf58f54f0c flex box children are shrunk to fit into the container where they previously would keep their size.


Steps to reproduce: open attached example.html in a window 500x500

Previously: both frames would have width 300px
After the change: each has width 242

[Yury Semikhatsky]

Created attachment 447273 [details]
Before

[Yury Semikhatsky]

Created attachment 447274 [details]
After

[Sergio Villar Senin]

I guess you mean r286206 then in the title...

[Sergio Villar Senin]

Does it happen if you use <div>s instead of iframes with embedded documents?

[Sergio Villar Senin]

BTW I've just tried with ToT and I cannot reproduce the issue you mention. I resize the window and both boxes end up occupying the same space...

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #5)
> BTW I've just tried with ToT and I cannot reproduce the issue you mention. I
> resize the window and both boxes end up occupying the same space...

Ah OK I see it now, the layout seems inconsistent, sometimes they're equally sized but then you resize and one of the becomes larger.

[Yury Semikhatsky]

(In reply to Sergio Villar Senin from comment #4)
> Does it happen if you use <div>s instead of iframes with embedded documents?

No, I can only reproduce it with iframes. Apparently there was some intrinsic min-width (don't see it in the inspector though).

[Yury Semikhatsky]

According to [1] iframe has default width=300. Before the change when two iframes having 'flex-shrink:1' were put into a flex container with width=500 they would keep width 300 (iframe's default width was treated as min-width essentially) which was consistent with Chromium behavior. After the change they are both shrunk to respect 'flex-shrink:1'. I don't know whether this new behavior is correct by the spec but it definitely breaks some web pages.


[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-width

[Sergio Villar Senin]

(In reply to Yury Semikhatsky from comment #8)
> According to [1] iframe has default width=300. Before the change when two
> iframes having 'flex-shrink:1' were put into a flex container with width=500
> they would keep width 300 (iframe's default width was treated as min-width
> essentially) which was consistent with Chromium behavior. After the change
> they are both shrunk to respect 'flex-shrink:1'. I don't know whether this
> new behavior is correct by the spec but it definitely breaks some web pages.

Thanks for the info. I have a patch for this issue, I'd love to hear more about those sites that are broken, just to double check that the fix works in the wild.

[Sergio Villar Senin]

Created attachment 447606 [details]
Patch

[Sergio Villar Senin]

Note that I haven't included any test because I'm importing https://github.com/web-platform-tests/wpt/pull/32141 as soon as it lands in the WPT repo (I mean as part of this patch).

[Manuel Rego Casasnovas]

(In reply to Sergio Villar Senin from comment #11)
> Note that I haven't included any test because I'm importing
> https://github.com/web-platform-tests/wpt/pull/32141 as soon as it lands in
> the WPT repo (I mean as part of this patch).

You could include the tests in this patch, and they get reviewed here, and once you get them approved here, the WPT upstream PR gets approved automatically.

[Manuel Rego Casasnovas]

Comment on attachment 447606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447606&action=review

> Source/WebCore/ChangeLog:12
> +        regression.

Is there no other cases of replaced elements that need SVG images like behavior?

[Sergio Villar Senin]

(In reply to Manuel Rego Casasnovas from comment #12)
> (In reply to Sergio Villar Senin from comment #11)
> > Note that I haven't included any test because I'm importing
> > https://github.com/web-platform-tests/wpt/pull/32141 as soon as it lands in
> > the WPT repo (I mean as part of this patch).
> 
> You could include the tests in this patch, and they get reviewed here, and
> once you get them approved here, the WPT upstream PR gets approved
> automatically.

The last time I tried the exporter it failed due to naming. In webkit the reference has -expected suffix while WPT expects -ref suffix. That's why I preferred to upload it to WPT and once landed I'll include it in this patch.

[Sergio Villar Senin]

(In reply to Manuel Rego Casasnovas from comment #13)
> Comment on attachment 447606 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447606&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        regression.
> 
> Is there no other cases of replaced elements that need SVG images like
> behavior?

No that I know of. The original patch had initially the code that I'm changing now. At the very last minute I decided to generalize it a bit thinking that replaced elements were basically just images without realizing that things like iframes were also replaced elements.

For the rest of the images (raster images) they have intrinsic dimensions (1024x768, etc...). The thing with vector graphics (like SVG) is that they're scale independent, so it does not make much sense to assign them an intrinsic size. But it does make a lot of sense to have an intrinsic ratio, so resizing the image respects the proportions.

[Sergio Villar Senin]

Created attachment 447700 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 447700 [details]
Patch

r=me

[Sergio Villar Senin]

Committed r287355 (245497@trunk): <https://commits.webkit.org/245497@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/86810685>