New issue
Advanced search Search tips

Issue 869869 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 869867



Sign in to add a comment

[LayoutNG] Incorrect cell heights

Project Member Reported by kojii@chromium.org, Aug 1

Issue description

May need more categorizations, but following test failures are about incorrect cell heights.

# Incorrect cell height: not quirks.
fast/css-intrinsic-dimensions/height-positioned.html

# Incorrect cell height: can WontFix if quirks only?
fast/block/basic/quirk-percent-height-table-cell.html
tables/mozilla/bugs/ bug101674 .html
tables/mozilla/bugs/ bug131020 -2.html
tables/mozilla/bugs/ bug137388 -3.html
tables/mozilla/bugs/bug149275-1.html
tables/mozilla/bugs/bug149275-2.html
tables/mozilla/bugs/ bug23235 .html
tables/mozilla/bugs/ bug50695 -2.html
tables/mozilla/bugs/ bug55527 .html

Maybe this one is different cause? From David's comment:
> tables/mozilla/bugs/ bug137388 -3.html
> Looks like we're too eager to apply the %height-of-indefinite-height-is-auto rule for this iframe
 
Blocking: 869867
Owner: dgro...@chromium.org
Status: Assigned (was: Available)
IIUC you said you'll look into them, but if any of these turned out to be non-tables, or you're too busy, let's discuss how to get them down.
When these 4 are set to standards mode, ng matches legacy, so I suppose they are lower priority:
tables/mozilla/bugs/ bug101674 .html
tables/mozilla/bugs/ bug137388 -3.html
tables/mozilla/bugs/ bug50695 -2.html
tables/mozilla/bugs/ bug23235 .html (with NG having different kerning or word spacing)

These have % height and only sizes are wrong:
tables/mozilla/bugs/ bug131020 -2.html
tables/mozilla/bugs/bug149275-1.html
fast/css-intrinsic-dimensions/height-positioned.html

These have % height and both size and position are wrong:
fast/block/basic/quirk-percent-height-table-cell.html
tables/mozilla/bugs/bug149275-2.html

no % height, no clue where height difference is coming from, possibly text sizing?
tables/mozilla/bugs/ bug55527 .html

Notes from tables/mozilla/bugs/bug149275-1.html

FF matches NG, which makes td>div auto height so that the Line # entries are shown. Edge matches legacy, which gives td>div 0 height and Line # entries are NOT shown.

Reducing test case for legacy (chrome stable):

#dvFundos overflow:
overflow:hidden -- NG behavior (auto height)
overflow:visible -- NG behavior
overflow:scroll -- legacy behavior (0 height)
overflow:auto -- legacy behavior

#dvFundos overflow-x:
overflow-x:hidden -- legacy behavior (switched from above!)
overflow-x:visible -- NG behavior
overflow-x:scroll -- legacy behavior
overflow-x:auto -- legacy behavior

Reduction at http://jsfiddle.net/dgrogan/761mud58/1/
<table height='100%' border=1>
  <tr>
    <td>
      <div id=dvFundos style='height:100%;overflow-x:hidden;border: 5px dotted lightblue;'>
        some text
      </div>
    </td>
  </tr>
</table>

To make Edge/Legacy show the text, which matches NG and FF:<br>
 - Remove %height from table<br>
 - Change overflow-x:hidden to overflow-x:visible or removing it altogether<br>
 - Remove %height from #dvFundos

Notes from tables/mozilla/bugs/ bug131020 -2.html

NG/Edge/FF agree on the dimensions of the blue box; legacy makes it too short.
NG and FF paint the blue box in the same place, aligning its bottom border with that of the table. Legacy and Edge center it vertically. This is probably an improvement over legacy and should be rebased.

http://jsfiddle.net/dgrogan/6wuj3cv8/2/
fast/css-intrinsic-dimensions/height-positioned.html

Edge/FF look nothing like Legacy/NG. MDN reports they don't implement height:fill-available. caniuse agrees.

Given that I don't have another browser to verify NG or legacy as more correct, and this isn't a table (but does look like one!), I'm going to punt.
bug149275-2.html

Two things going on.

(1) When resizing %height descendants of table cells, legacy apparently treated the resulting height as border box height, not content height. NG correctly does this as content height, to the extent that it should non-auto, which I am unclear on. Firefox treats the %height as auto, presumably because the parent has auto height. There's been on-and-off discussion of this issue over the last few years but I'm not caught up on where it stands now.

(2) The content of the cell is not honoring vertical-align:middle in NG, but I do not know why.

http://jsfiddle.net/dgrogan/kz0bq6rv/7/
 bug55527 .html is due to a rounding issue with calculating the height of the cell using 1ex as top and bottom margins, but only when running layout tests. When running content_shell without --run-web-tests, the results are identical between legacy and NG.
To add to comment 8, when --run-web-tests is specified, the row height in NG changes, legacy is the same. row height is usually 36, but with --run-web-tests --enable-blink-features=LayoutNG, row height is 37.
Owner: ----
Status: Available (was: Assigned)
It happens because of fractional block-side padding on table cells. See attachment. LayoutNG treats table cell padding as any other padding, i.e. with sub-pixel support. Legacy, on the other hand, rounds block-side padding down (but apparently not inline-side padding).

Fix here: https://chromium-review.googlesource.com/c/chromium/src/+/1372632
demo.html
1.4 KB View Download
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 17

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

commit e865d9cff6864f415556dd8db58cd3f575f975d4
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Mon Dec 17 10:27:39 2018

[LayoutNG] Floor table cell padding on the block sides.

This matches legacy layout, which generally doesn't support subpixels
for table layout.

Bug: 869869
Change-Id: Ib0d9c61cee27568388b3bc5fef81bf173ce8f77f
Reviewed-on: https://chromium-review.googlesource.com/c/1372632
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617076}
[modify] https://crrev.com/e865d9cff6864f415556dd8db58cd3f575f975d4/third_party/blink/renderer/core/layout/ng/ng_length_utils.cc
[modify] https://crrev.com/e865d9cff6864f415556dd8db58cd3f575f975d4/third_party/blink/web_tests/FlagExpectations/enable-blink-features=LayoutNG

Sign in to add a comment