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

Issue 682166 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Use other robhogan account instead.
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 3
Type: Bug-Regression



Sign in to add a comment

Regression percentage resolution on replaced elements inside a table cell

Reported by asier.lo...@openbravo.com, Jan 18 2017

Issue description

Chrome 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.

 
chome55.png
2.1 KB View Download
chrome56.png
2.1 KB View Download

Comment 1 by ajha@chromium.org, Jan 19 2017

Components: Blink
Labels: -Type-Bug Needs-Bisect Needs-Triage-M56 OS-Linux Type-Bug-Regression
Cc: rbasuvula@chromium.org r...@igalia.com
Labels: -Needs-Bisect -Needs-Triage-M56 hasbisect-per-revision M-57 OS-Mac OS-Windows
Owner: r...@chromium.org
Status: Assigned (was: Unconfirmed)
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.

Comment 3 by svil...@igalia.com, 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?
Hi, I already tried to simplify the example as much could :( 

It does not require of additional resources other than few images.

Comment 5 by r...@igalia.com, Jan 19 2017

Cc: svil...@igalia.com jfernan...@igalia.com
Components: -Blink Blink>Layout
Owner: r...@igalia.com
Summary: Regression percentage resolution on replaced elements (was: different visualization between chrome 55 and 56)
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
percentage-resolution-reduced.html
275 bytes View Download
percentage-resolution.png
48.7 KB View Download

Comment 6 by r...@igalia.com, Jan 19 2017

BTW the behavior in Edge 13 and IE 11 is the same than current Chrome behavior.

win10_edge_13.0.png
9.0 KB View Download
So which is the expected "correct" behavior?

Comment 8 by r...@igalia.com, Jan 19 2017

Cc: robhogan@chromium.org
Components: -Blink>Layout Blink>Layout>Table
Summary: Regression percentage resolution on replaced elements inside a table cell (was: Regression percentage resolution on replaced elements)
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.


Comment 9 by robho...@gmail.com, Jan 19 2017

Cc: francois...@outlook.com
Owner: robhogan@chromium.org
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.
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.

Comment 11 by r...@igalia.com, Jan 19 2017

Status: WontFix (was: Assigned)
Ok, thanks both for the information, it seems this is not a regression then but an improvement.

I'm closing this as WontFix.
Ok, thank you all for your time and detailed explanations.

Sign in to add a comment