Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LayoutNG] Honor table cell descendants' min heights #17819

Merged
merged 1 commit into from Jul 13, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Jul 13, 2019

LayoutNG ignored them for row height sizing (Measure phase) when the
descendant had a % height and overflow auto/scroll.

The fix turned out to be simple but getting there was not.

I think our existing NG behavior was correct according to the spec, but
if my interpretation is correct, the spec is missing this case.

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 (see
section below) 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.

In these test cases the descendants of table cells' heights do depend on
their parent cell' height and have overflow set to neither visible nor
hidden, so should get 0. (They depend on their parent's height because
the descendant height is max(resolved min-height, resolved height) where
resolving height needs the cell height.)

But sizing the row such that the descendants don't fit doesn't make
sense (and isn't what engines do today). This was happening in NG when
the min-height of the descendant was larger than the height of the row,
after calculating the height of the row by indiscriminately using 0px as
the height contribution from % height scroller descendants.

The fix is to use % height scroller descendants' min heights as their
contribution to the row height instead of 0px.

We still get a related case wrong in legacy and NG
(percentage-sizing-of-table-cell-children-005.html) -- when a scroller
descendant has % height but the cell height is indefinite, we should
treat the descendant's % height as auto, per css2, even for row sizing
purposes. Instead, we currently treat it as a % height such that it
contributes 0px to the height instead of its post-layout height.

I also discovered some suspected dead code that used to have some of
this relayout-depending-on-scrolling logic.

Bug: 982323, 982312
Change-Id: Iff5210f6bf53c8f7e4b29ca32f8401d0eb738317
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1700782
Commit-Queue: David Grogan <dgrogan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677211}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1700782 branch 2 times, most recently from 8aaeaf9 to 262ec8c Compare July 13, 2019 21:31
LayoutNG ignored them for row height sizing (Measure phase) when the
descendant had a % height and overflow auto/scroll.

The fix turned out to be simple but getting there was not.

I think our existing NG behavior was correct according to the spec, but
if my interpretation is correct, the spec is missing this case.

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 (see
section below) 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.

In these test cases the descendants of table cells' heights do depend on
their parent cell' height and have overflow set to neither visible nor
hidden, so should get 0. (They depend on their parent's height because
the descendant height is max(resolved min-height, resolved height) where
resolving height needs the cell height.)

But sizing the row such that the descendants don't fit doesn't make
sense (and isn't what engines do today). This was happening in NG when
the min-height of the descendant was larger than the height of the row,
after calculating the height of the row by indiscriminately using 0px as
the height contribution from % height scroller descendants.

The fix is to use % height scroller descendants' min heights as their
contribution to the row height instead of 0px.

We still get a related case wrong in legacy and NG
(percentage-sizing-of-table-cell-children-005.html) -- when a scroller
descendant has % height but the cell height is indefinite, we should
treat the descendant's % height as auto, per css2, even for row sizing
purposes. Instead, we currently treat it as a % height such that it
contributes 0px to the height instead of its post-layout height.

I also discovered some suspected dead code that used to have some of
this relayout-depending-on-scrolling logic.

Bug: 982323, 982312
Change-Id: Iff5210f6bf53c8f7e4b29ca32f8401d0eb738317
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1700782
Commit-Queue: David Grogan <dgrogan@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677211}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants