New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 782948 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

CSS tables within a flex container are rendering differently

Project Member Reported by jpittenger@google.com, Nov 8 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.89 Safari/537.36

Example URL:
https://jsbin.com/bipozoyuhu/edit?html,css,output

Steps to reproduce the problem:
I've built a test case here; it assumes the rendered area is too small to fit the strings being displayed without scrolling/wrapping:
https://jsbin.com/bipozoyuhu/edit?html,css,output

What is the expected behavior?
With Chrome version 61.0.3163.100, the flex container size is increased to fit the strings without wrapping, and it becomes scrollable.

What went wrong?
With my current version of Chrome, the flex container displays the tables such that they overlap.

Does it occur on multiple sites: N/A

Is it a problem with a plugin? No 

Did this work before? Yes 61.0.3163.100

Does this work in other browsers? N/A

Chrome version: 62.0.3202.89  Channel: stable
OS Version: OS X 10.12.6
Flash Version: 

FWIW, I'm not convinced the new behavior is incorrect, but it's definitely different. At the time of writing, the latest version of Safari matches Chrome 61.0.3163.100 (content scrolls), whereas the latest version of Firefox wraps the text (rather than overlapping).
 
Components: Blink>CSS
Labels: Needs-Bisect Needs-Triage-M62
Cc: vamshi.k...@techmahindra.com
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision Triaged-ET M-62 OS-Linux OS-Windows Pri-1
Owner: cbiesin...@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on the reported chrome version stable 62.0.3202.89 and on the latest dev 64.0.3262.0 using Windows10, Ubuntu 14.04 and Mac 10.12.6.

Manual Bisect Info:
---------------------------
Good Build: 62.0.3169.0
Bad Build: 62.0.3170.0

You are probably looking for a change made after 490404 (known good), but no later than 490405 (first known bad).

CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/d1eb577c332b15ba22caf53ecbbd06093df5d633..05f74c2faaa2daac62a38d3d03b56835eff212c9

Reviewed-on: https://chromium-review.googlesource.com/590729

Suspecting same from changelog.

@cbiesinger:Please confirm the issue and help in re-assigning if it is not related to your change.

Labels: -M-62 ReleaseBlock-Stable M-64
Recent regression, can we get fix for M64? Please feel free to modify the RB label accordingly.

Comment 4 by gracec@chromium.org, Nov 10 2017

Labels: -Type-Compat Update-Weekly Type-Bug-Regression
At first sight it looks like Firefox is correct, but I'm still investigating
Components: -Blink>CSS Blink>Layout>Flexbox
Cc: dgro...@chromium.org
Components: Blink>Layout>Table
Oh this is a fun bug. dgrogan, please see question at the bottom.

Sooo, this is what happens: We compute a desired width for the flex items (ie. the tables) as half the container width each, like you'd intuitively expect (flex-basis is 0 for each and the flex-grow factor is the same).
We set this as the override main axis size (here, the override logical width)

BUT, it turns out that LayoutTable::UpdateLogicalWidth overrides the base class version of this function and ignores the override width (!)

Now, this kinda-sorta used to work (not really, because the size was still wrong). This is the important change:
-    LayoutUnit child_main_extent = MainAxisExtentForChild(*child);
+    LayoutUnit child_main_extent = flex_item.FlexedBorderBoxSize();

That is -- we used to check the actual child.Width() for positioning the children, whereas now we just use the size we computed that the child *should* have. Of course, they ought to be the same.

dgrogan, is there a reason why LayoutTable::UpdateLogicalWidth does not take the override width into account / should it be changed to do so?
Cc: divya.pa...@techmahindra.com cbiesin...@chromium.org
 Issue 784680  has been merged into this issue.
Status: Started (was: Assigned)
In the meantime I made https://chromium-review.googlesource.com/#/c/chromium/src/+/780462 to fix this regression/release blocker
Cc: mstensho@chromium.org
I do not know off the top of my head. Maybe Morten does?

Looks like the opposite problem was noticed but not solved yet due to compat issues in issue 667785.

We can talk more after the US holiday break?

/me updates OOO status

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c7a360904e8d506c17894c47cf482cb42335e426

commit c7a360904e8d506c17894c47cf482cb42335e426
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Tue Nov 21 05:26:39 2017

[css-flexbox] Re-read a flex item size after layout

...because tables ignore the override width, and if we don't re-check
the size we position the flex items incorrectly.

R=mstensho@chromium.org

Bug:  782948 
Change-Id: I86ecf63fe2d9cd6b27813436a842e15cfed783c6
Reviewed-on: https://chromium-review.googlesource.com/780462
Reviewed-by: David Grogan <dgrogan@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518135}
[add] https://crrev.com/c7a360904e8d506c17894c47cf482cb42335e426/third_party/WebKit/LayoutTests/css3/flexbox/bug782948.html
[modify] https://crrev.com/c7a360904e8d506c17894c47cf482cb42335e426/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

When a table is a flex item (or grid item?), it needs to give up at least some of its own sizing peculiarities, and let the flex (/grid?) algorithm decide more.

Would it work if LayoutTable::UpdateLogicalWidth() just calls super::UpdateLogicalWidth() and returns, if it's a flex (/grid?) item?
Labels: Merge-Request-63
Hmm interesting. That's probably a good idea, however I would suggest that we wait with that until after the upcoming branch so there's more time to catch any potential site breakage from that?

Meanwhile I'd like to take my patch for merging to the branch as that restores our previous behavior (and this is a release blocker)
Project Member

Comment 14 by sheriffbot@chromium.org, Nov 21 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-63 Merge-Rejected-63
This was regressed in M62 (not M63 regression). M63 is very close to Stable promotion and we're only taking critical merges in. Hence, I'm rejecting merge to M63. Please let me know if there is any concern here. 
Status: Fixed (was: Started)
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eee88582b5300e34aa2ea56bc5da17457e67aa00

commit eee88582b5300e34aa2ea56bc5da17457e67aa00
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Thu Dec 07 12:45:27 2017

Avoid table sizing peculiarities for flex and grid items.

Let the flex/grid layout algorithm determine the size of tables that
are flex/grid items. Don't let the intrinsic width of table columns
affect the width of the item.

Bug:  782948 , 667785
Change-Id: Ifcea6af51ce5bf74377f93b5cd0437272a5c7ef0
Reviewed-on: https://chromium-review.googlesource.com/781859
Reviewed-by: David Grogan <dgrogan@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522408}
[delete] https://crrev.com/8448affe3471114a8be3a25198083c6f8ab3f103/third_party/WebKit/LayoutTests/css3/flexbox/bug782948.html
[delete] https://crrev.com/8448affe3471114a8be3a25198083c6f8ab3f103/third_party/WebKit/LayoutTests/css3/flexbox/flexitem-expected.txt
[modify] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/LayoutTests/css3/flexbox/flexitem.html
[add] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/table-as-item-narrow-content.html
[add] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/table-as-item-wide-content.html
[add] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/LayoutTests/external/wpt/css/css-grid/grid-items/explicitly-sized-grid-item-as-table.html
[add] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/LayoutTests/external/wpt/css/reference/ref-filled-green-100px-square-only.html
[modify] https://crrev.com/eee88582b5300e34aa2ea56bc5da17457e67aa00/third_party/WebKit/Source/core/layout/LayoutTable.cpp

Sign in to add a comment