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

Issue 798478 link

Starred by 7 users

Issue metadata

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



Sign in to add a comment

CSS table-cell will not inherit height properly in Chrome 63

Reported by mcoz...@gmail.com, Jan 2 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce the problem:
1. Make a div with display:table; width:100%; height:100%
2. Make two nested divs with display:table-cell; width:50%; height:100%; vertical-align:middle
3. Add paragraph text to first cell
4. Add block element/div to second cell, with height:100%;

What is the expected behavior?
The div in the second cell should grow with the text content of the first cell. 

What went wrong?
The cell will not inherit the proper height of the table. A fixed height of the table is now required to make the block element fill the space. Image/div grows appropriately in Firefox and IE. Image used to grow appropriately in Chrome prior to V.63

Did this work before? Yes 

Chrome version: 63.0.3239.84  Channel: n/a
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: 

Code has not changed on sites afflicted with this problem so something about the old CSS is no longer valid in Chrome 63.
 
tables.html
2.1 KB View Download
tables-chrome.png
512 KB View Download
tables-firefox.png
852 KB View Download
tables-chrome-2.png
444 KB View Download
tables-firefox-2.png
690 KB View Download

Comment 1 by lucco...@gmail.com, Jan 3 2018

Same problem here. 
For now I had to do a JS workaround so after the page finishes loading I set the height of the right TD to be the same as the table. 

$( document ).ready(function() {
   $('.rightTD').height($('.table').height());
}
Cc: sc00335...@techmahindra.com
Labels: -Pri-2 hasbisect-per-revision ReleaseBlock-Stable Triaged-ET M-63 Needs-Triage-M63 OS-Linux OS-Mac Pri-1
Owner: robho...@gmail.com
Status: Assigned (was: Unconfirmed)
mcozart@ Thanks for the issue.

Able to reproduce this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on the latest Canary 65.0.3310.0 and Stable 63.0.3239.108 by following the steps mentioned in the original comment.

Bisect Information:
====================
Good Build: 63.0.3236.0
Bad Build : 63.0.3238.0

On executing the per-revision bisect script, below is the changelog URL.

Changelog URL:
---------------
https://chromium.googlesource.com/chromium/src/+log/20893ab9a294053d7ff10a4a17cb148aeb51545a..65d9417940fca2d9edc013f02dbc5ba3b7cccc3c

From the above Changelog URL, suspecting the below change for this issue.
Reviewed-on: https://chromium-review.googlesource.com/700520

robhogan@ Can you please check if this issue is related to your change, else help us in assigning to the right owner.

Adding ReleaseBlock-Stable as this is a recent Regression. Please feel to remove the same if this is not applicable.

Thanks...

Comment 3 by mcoz...@gmail.com, Jan 3 2018

Thank you, lucco. I am really trying to avoid a JS workaround as I have a template with this kind of layout across many, many domains that would need updating.

Please let me know if I can provide any more information in an effort to get this resolved.
Cc: e...@chromium.org
Labels: M-64 FoundIn-65 FoundIn-64 Target-64 FoundIn-63

Comment 5 by robho...@gmail.com, Jan 3 2018

Cc: francois...@outlook.com
This stems from changes discussed in  issue 687551  and issue 762330. Our intention was to match the spec (https://www.w3.org/TR/css-tables-3/#computing-the-table-height) and as far as I can tell from the test case supplied we are matching the spec. See https://bugs.chromium.org/p/chromium/issues/detail?id=762330#c9 and https://bugs.chromium.org/p/chromium/issues/detail?id=687551#c36.

In a nutshell we are treating the table cell as though it is auto. So for example if you remove the 'height:100%' from the table cell you will see it has the same rendering. This is for the reasons discussed at the links above.

That said, I would have expected our behaviour to match Edge here, since that is the other browser following the spec as closely as we are. I'm cc'ing the spec editor in the hope that he can confirm my understanding. (Sorry to drag you into yet another table cell height discussion Francois!)

Maybe the difference has to do with browser-specific treatment of the image inside the cell. Can't rule out that I've misunderstood something either.

Comment 6 by e...@chromium.org, Jan 3 2018

Cc: robho...@gmail.com
Owner: dgro...@chromium.org
Thanks Rob, Over to David to comment on our current behavior.

Comment 7 by mcoz...@gmail.com, Jan 3 2018

Hi Rob,
I'm sorry for it not being the cleanest example. I just want to make sure table-cells are not going to need to inherit fixed heights going forward. That defeats the purpose of using display:table and display:table-cell for equal height, responsive columns. 

Eager to hear more insight. Thanks.
Not sure if anyone looked at Edge yet. Edge 41.16299.15.0 matches IE and Firefox.

This might be obvious, but we can't change behavior that was interoperable with all the other browsers, even if our new behavior matches the current iteration of the spec. The spec will probably need more refinement for this case. (I learned this principle the hard way, trying to tackle the same bug in chrome 50.)

If we don't hear really good news* from Francois within 24 hours we'll have to revert and merge to 64 beta. 

And it looks like the commit that caused this regression was fixing a regression from an earlier change, so we'll probably have to revert that too.

* "Really good news": Edge is very soon going to match chrome's 63+ behavior.

Comment 9 by mcoz...@gmail.com, Jan 4 2018

The Firefox screenshots I attached displayed the same for Edge, which was very surprising to me. Chrome is the only odd-ball not inheriting the height as expected.

Thanks for keeping this moving!

Comment 10 Deleted

> Once the final size of the table and the rows has been determined, 
> the content of the table-cells must also go through a second layout pass, 
> where, if appropriate, percentage-based heights are this time resolved
> against their parent cell used height.
> 
> It is appropriate to resolve percentage heights on direct children of table cells 
> if the cell is considered to have its height specified explicitly 
> or the element is absolutely positioned, see CSS 2.
> 
> It is further clarified that a cell is considered to have its height specified explicitly
> if the computed height of the cell or any of its table ancestors
> is a length or percentage.

I think the condition chain works fine here until you get to "table{height:100%}" which should "behave as auto" per https://drafts.csswg.org/css-sizing-3/#behave-auto but apparently that fact is ignored in all browsers but Chrome.

The spec text doesn't mention the "behave as auto" behavior explicitly and based on what I see, the interoperable behavior seems to be to ignore it for the table, but take it into account for the cell. It's a bit messy, and I agree it would require an additional line in the spec to clarify that point.
Thanks for clearing that up Francois. I'll revert the change so that we match the other browsers.
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 9 2018

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

commit 9e7701a83830e5e68640af9dbb5932c73cf74ef3
Author: Robert Hogan <robhogan@gmail.com>
Date: Tue Jan 09 23:06:18 2018

Revert "Treat percent height cells with nothing to resolve against as auto"

This reverts commit 65d9417940fca2d9edc013f02dbc5ba3b7cccc3c.

Reason for revert: Per  crbug.com/798478  the behaviour introduced with this patch was correct per the spec but turns out to be not interoperable. So revert for now and check interoperability again when spec is clarified.

Original change's description:
> Treat percent height cells with nothing to resolve against as auto
>
> tables/mozilla/bugs/ bug113424 .html covers the quirky behaviour,
> in non-quirk mode we should continue to respect the rule that a
> percent-height cell with nothing to resolve against is auto.
>
> Bug: 762330
> Change-Id: I8c7d97d23c75b7eb56a3d93637444d0ccada8d9b
> Reviewed-on: https://chromium-review.googlesource.com/700520
> Commit-Queue: Emil A Eklund <eae@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#507427}

TBR=robhogan@gmail.com,eae@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 762330,  798478 
Change-Id: I07e0b9cfd8f0884f316ee98b51f75fa22b360229
Reviewed-on: https://chromium-review.googlesource.com/858017
Commit-Queue: Robert Hogan <robhogan@gmail.com>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Robert Hogan <robhogan@gmail.com>
Cr-Commit-Position: refs/heads/master@{#528156}
[modify] https://crrev.com/9e7701a83830e5e68640af9dbb5932c73cf74ef3/third_party/WebKit/LayoutTests/fast/table/percent-height-content-in-percent-height-cell-and-percent-height-table.html
[modify] https://crrev.com/9e7701a83830e5e68640af9dbb5932c73cf74ef3/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

Thanks for the revert. Is this something that needs to be targeted for M64? Since it's not as critical, my recommendation is to wait until M65. 
Cc: abdulsyed@chromium.org
We're not planning any further M63 releases. 

Please reply to comment #14 for M64.


Comment 16 by robho...@gmail.com, Jan 16 2018

Labels: Merge-Request-64
Project Member

Comment 17 by sheriffbot@chromium.org, Jan 16 2018

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Will this be a safe revert? Why is this critical?
We should not merge to beta given how close we are to the stable release.

But that's just the lesser of two evils. We're in a rough spot here because the reverted CL (65d9417940fca2d9edc013f02dbc5ba3b7cccc3c) fixed a different regression.

So the timeline of the page reported in issue 762330 will be:
worked since antiquity
regressed
worked
now regressed again in at least 65

The page reported here will have been broken in 63 and 64.
Labels: -Merge-Review-64 Merge-Rejected-64
Per comment#19, seems like we should not go ahead with the merge. Rejecting merge to M64. Let's target this for M65. If you think otherwise, please re-open with more clarifications. 
Labels: -M-64 -Target-64 M-65 Target-65
Moving to M65. 
I updated the spec based on your compat findings. 
https://github.com/w3c/csswg-drafts/commit/5b62f81c6ad732f1a0860414faa5950236682bd5
Status: Fixed (was: Assigned)
Thanks Francois.

Sign in to add a comment