Regression percentage resolution on replaced elements inside a table cell
Reported by
asier.lo...@openbravo.com,
Jan 18 2017
|
||||||
Issue descriptionChrome Version : Version 56.0.2924.59 beta (64-bit) URLs (if applicable) : https://jsfiddle.net/alostale/pkavLrd3/ Other browsers tested: Chromium Version 55.0.2883.87 Built on Ubuntu , running on Ubuntu 16.04 (64-bit): OK Chrome Version 57.0.2984.0 dev (64-bit): FAIL Firefox 50.1.0: OK What steps will reproduce the problem? 1. Open https://jsfiddle.net/alostale/pkavLrd3/ 2. check the visualization. What is the expected result? Visualization is expected to be as it was in chrome 55 (and firefox). See attachment chrome55.png. What happens instead? Visualization differs when opened in chrome 56 and 57. See attachment chrome56.png.
,
Jan 19 2017
Tested in chrome stable #55.0.2883.87 beta #56.0.2924.67 and Canary #57.0.2985.0 on Ubuntu 14.04 and was able to reproduce the issue. Below are the Bisect Details: ----------------------------- Good build: 56.0.2903.0 (Revision: 428231) Bad build: 56.0.2904.0 (Revision: 428574) You are probably looking for a change made after 428511 (known good), but no later than 428512 (first known bad). CHANGE-LOG URL: --------------------------------------- https://chromium.googlesource.com/chromium/src/+log/685f5586bab6fc956a133dd84de505ea5483833f..c4dc5e50ffa3ca4bef23a105ae8dc8304926b312 From the CL above, assigning the issue to the concern owner @rego: ------------------ Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner. Review-Url: https://codereview.chromium.org/2458023002 Note : Able to reproduce the issue in Mac 10.12.2 & Win 7.
,
Jan 19 2017
Hi Asier, could you please upload a minimized test case (i.e. no JS... no unnecessary styles, etc...) so that we could focus on the grid issue?
,
Jan 19 2017
Hi, I already tried to simplify the example as much could :( It does not require of additional resources other than few images.
,
Jan 19 2017
It seems r428512 introduced the regression. I managed to reduce the test case and it seems related to the percentage resolution, so I'll review it. Thanks for reporting. The issue only happens on tables as you can check in the following example: http://jsbin.com/tuqinar/edit?html,output
,
Jan 19 2017
BTW the behavior in Edge 13 and IE 11 is the same than current Chrome behavior.
,
Jan 19 2017
So which is the expected "correct" behavior?
,
Jan 19 2017
I don't know yet, I need to check in detail the spec.
The following code in LayoutBox::computePercentageLogicalHeight() makes me wonder what's the expected behavior:
} else if (cb->isTableCell()) {
if (!skippedAutoHeightContainingBlock) {
// Table cells violate what the CSS spec says to do with heights.
// Basically we don't care if the cell specified a height or not. We just
// always make ourselves be a percentage of the cell's current content
// height.
if (!cb->hasOverrideLogicalContentHeight()) {
// https://drafts.csswg.org/css-tables-3/#row-layout:
// For the purpose of calculating [the minimum height of a row],
// descendants of table cells whose height depends on percentages
// of their parent cell's height are considered to have an auto
// height if they have overflow set to visible or hidden or if
// they are replaced elements, and a 0px height if they have not.
LayoutTableCell* cell = toLayoutTableCell(cb);
if (style()->overflowY() != EOverflow::Visible &&
style()->overflowY() != EOverflow::Hidden &&
!shouldBeConsideredAsReplaced() &&
(!cell->style()->logicalHeight().isAuto() ||
!cell->table()->style()->logicalHeight().isAuto()))
return LayoutUnit();
return LayoutUnit(-1);
}
availableHeight = cb->overrideLogicalContentHeight();
}
} else {
Specifically the part of the text from the spec (https://drafts.csswg.org/css-tables-3/#row-layout):
> For the purpose of calculating this height,
> descendants of table cells whose height depends on
> percentages of their parent cell' height are considered
> to have an auto height if they have overflow set
> to visible or hidden or if they are replaced elements,
> and a 0px height if they have not.
So in the example I'm not sure if the "height: 100%" in the <div>,
should be resolved against the fixed height of the cell/row or as "auto".
Adding @robhogan in CC just in case he can bring some light to this topic.
,
Jan 19 2017
Francois is better at me than parsing these cases, so involving him in the discussion. Our intention in issue 671010 was to make Chrome conform to the latest evolution of the spec - though I'm not sure we're done yet. Francois, what do you make of the test case in #5? I believe our new behaviour is correct.
,
Jan 19 2017
I just had a look and Chrome's new behavior is correct per spec. I don't think the spec needs an update in this case, the behavior looks by design. During the first pass, percentages are considered auto, and they resolve during the second pass based on the height computed in previous pass. That's for the technical explanation. I would note that this is similar to what was resolved for CSS Grid last week at the CSSWG F2F <https://log.csswg.org/irc.w3.org/css/2017-01-13/#e761591>. Now, for the semantic explanation, the difference between tables and div can be explained by the fact the "height" of a table-something is actually more understood as a "minimum height" than an actual height, especially when applied to cells. You can verify that changing height to min-height on the div yields identical results in the provided testcase. Here is another example of this behavior: https://jsfiddle.net/hezmjshd/ vs https://jsfiddle.net/hezmjshd/1/ Firefox has a long-standing issue where heights applied to cells really apply to the cell instead of being a min-height for the row, which causes them to apply the percentage on the explicit height of the cell in this case, but this is definitely a bug.
,
Jan 19 2017
Ok, thanks both for the information, it seems this is not a regression then but an improvement. I'm closing this as WontFix.
,
Jan 20 2017
Ok, thank you all for your time and detailed explanations. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by ajha@chromium.org
, Jan 19 2017Labels: -Type-Bug Needs-Bisect Needs-Triage-M56 OS-Linux Type-Bug-Regression