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 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 left a comment

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 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}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1700782 branch from 262ec8c to 7e78497 Jul 13, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 75bb16c into master Jul 13, 2019
15 checks passed
15 checks passed
manifest-build-and-tag manifest-build-and-tag
Details
website-build-and-publish website-build-and-publish
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20190713.39 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1700782 branch Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.