1552287 - [css-contain] contain:layout on various frames doesn't fully suppress baseline

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

[Spinning this off from bug 1508441 comment 1 / 2]

"contain:layout" doesn't currently suppress baseline alignment on a few form controls and other frames:

• <input> type=button, submit, text, number, date, time
• <textarea>
• inline-grid
• inline-table

Filing this bug on fixing those. I think these reftests should pass when this bug is fixed:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/w3c-css/submitted/contain/contain-layout-suppress-baseline-002.html
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/w3c-css/submitted/contain/contain-layout-suppress-baseline-002-ref.html

https://hg.mozilla.org/mozilla-central/raw-file/tip/testing/web-platform/tests/css/css-contain/contain-layout-baseline-005.html
https://hg.mozilla.org/mozilla-central/raw-file/tip/testing/web-platform/tests/css/css-contain/reference/contain-layout-baseline-005-ref.html

Comment 1

[Daniel Holbert [:dholbert]]

And I'm guessing this will also fix
https://hg.mozilla.org/mozilla-central/raw-file/tip/testing/web-platform/tests/css/css-contain/contain-layout-baseline-004.html
https://hg.mozilla.org/mozilla-central/raw-file/tip/testing/web-platform/tests/css/css-contain/reference/contain-layout-baseline-004-ref.html

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1552287 part 1: [css-contain] Fix some CSS layout-containment web-platform-tests to make their assumptions more valid. r?TYLin

In particular:
*** XXXdholbert Fill this in ***

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1552287 part 2: [css-contain] Adjust various reflow & baseline methods so that layout-contained frames behave as if they have no baseline. r?TYLin

We previously (in bug 1491235) adjusted some utility code to make
layout-contained frames behave as if they have no baseline.

But that's not sufficient. To make frames fully report lack-of-a-baseline,
we now do the following for layout-contained frames, as of this patch:
(a) We now leave the ReflowOutput outparam's BlockStartAscent member at its
default value.
(b) We now return 'false' from more bool-returning baseline-getter-methods
(where 'false' indicates 'no baseline').
(c) We now return the margin-box-bottom position from more nscoord-returning
baseline-getter-methods.

Depends on D32182

Comment 4

[Daniel Holbert [:dholbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6087ca39f098d155a55ac5c9ae0a4355ceddc9b5

Comment 5

[Daniel Holbert [:dholbert]]

Try run was good -- the orange was from unrelated intermittents and from me forgetting to remove the failure annotations for contain-layout-suppress-baseline-002.html (which has a copy and a failure annotation in reftests & web-platform-tests).

I've updated the patch to remove the annotations for contain-layout-suppress-baseline-002.html now. New smaller Try run as a sanity-check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=715da3afe4f90bdc6bb70bcfc16df69c64f7902e

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/888c32d2a32e
part 1: [css-contain] Fix some CSS layout-containment web-platform-tests to make their assumptions more valid. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/815c6657d164
part 2: [css-contain] Adjust various reflow & baseline methods so that layout-contained frames behave as if they have no baseline. r=TYLin

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16982 for changes under testing/web-platform/tests

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 10

[Daniel Holbert [:dholbert]]

This is getting backed due to a failure in reftest contain-layout-suppress-baseline-002.html. The failure is shown in
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/P4G6jqlDR1-Qwh6mHy5kFQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

The 4th whole line is positioned 1px lower in the testcase as compared to the reference case, for some reason. It's a subtle difference and probably is just something I need to work around in the test. (We're still aligning things correctly with each other using the correct baseline, but just at a different position for some reason.)

Comment 11

[Daniel Holbert [:dholbert]]

(Note: the fact that this is getting backed out and relanded might mean that we need to do some manual work to get the testing/web-platform/tests changes merged. Last I checked, the WPT bot cancels pull requests for a backout, and doesn't automatically re-attempt to merge after a backout has happened. Hopefully jgraham can help with that once we've got an updated patch-series landed here.)

Comment 12

[Andreea Pavel [:apavel]]

Backed out for failing contain-layout-suppress-baseline-002.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&selectedJob=248083297&revision=ee890b30dbe3126fa7c0c55b911de8dc65ef3867

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248083297&repo=autoland&lineNumber=45655

Backout: https://hg.mozilla.org/integration/autoland/rev/3b14313df83b02bcbc04a4b788d63f6e9a20becb

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 14

[Daniel Holbert [:dholbert]]

It looks like the issue is that the textarea is slightly taller than the canvas (which is expecting to be the tallest thing on the line & to establish the baseline). In particular, there's a canvas with height:50px, and then a textarea which (including its 2px of border on each side) has a used height of 50.8px (0.8px taller than the canvas).

I see this same 50.8px height with a simple testcase like e.g. data:text/html,<textarea style="border:2px solid teal">. It looks like, by default, a textarea is tall enough to fit ~4 lines of text -- so this weird height presumably comes from 4 * [line-height] + border + padding or something.

Anyway, I'll address this in a reasonably-robust way by:

• explicitly specifying a smallish font-size for all of the tested content, since I'm kind of depending on the size (if we had font-size:60px, then various parts of this test would probably fail in weird ways)
• explicitly specifying a smallish height on that textarea, like e.g. height: 2em

Each of these changes will reduce the height of the textarea on their own, so the combination of the two should be even better / more-robust.

Comment 15

[Daniel Holbert [:dholbert]]

Actually:

• the 1px of space at the top of the textarea in the testcase snapshot was from the default margin:1px styling on textarea.
• that space wasn't present in the reference case because in the reference case, everything is end-aligned and the textarea was big enough that the reference-case-hardcoded margin-bottom:20px plus the 50.8px size of the textarea were sufficient to push it all the way up to (or past) the top of the 70px-tall flex container.

Instead of doing what I described in the previous comment, I ended up simplifying the test by:

• removing the hardcoded height on the flex container, so that its sizing is purely based on the collective height of its baseline-aligned children
• manually specifying a (small) margin on all the elements so that they're spaced a bit and so that textarea isn't awkwardly offset from the rest

Note: I'm making this change in the layout/reftests/w3c-css copy of the test (the "upstream-most" copy of the test), and I'm leaving our reimported copy untouched. So, our reimported copy will fail on windows for the moment, and so I've adjusted "part 2" so that it refines the meta/.../*.ini annotation accordingly, rather than deleting that .ini file. We can delete the .ini file soon (and indeed, we'll notice the unexpected-pass and be prompted to delete it) once the change propagates through the layout-reftests --> github-wpt --> our-wpt-import pipeline.

Comment 16

[Daniel Holbert [:dholbert]]

Green run (including windows/mac/linux, reftests + wpt) with the new test fixes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e4831eef9a975b065928adb9346f143b6cfa435

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9360fef47426
part 1: [css-contain] Fix some CSS layout-containment web-platform-tests to make their assumptions more valid. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/3eda0c8cf975
part 2: [css-contain] Adjust various reflow & baseline methods so that layout-contained frames behave as if they have no baseline. r=TYLin

Comment 18

[Daniel Holbert [:dholbert]]

Assuming this sticks, I think we might need some manual intervention to get the WPT changes merged, since there was a backout.

@jgraham, could you take care of whatever manual steps are needed? (if any)

If it's useful: the changes in https://github.com/web-platform-tests/wpt/pull/16982 (from the first landing) are actually still fine -- my relanding-adjustments didn't affect anything in testing/web-platform/tests.

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9360fef47426
https://hg.mozilla.org/mozilla-central/rev/3eda0c8cf975

Comment 21

[Julien Cristau [:jcristau]]

AIUI contain-layout isn't going to be enabled until 69.

Comment 22

[James Graham [:jgraham]]

@jgraham, could you take care of whatever manual steps are needed? (if any)

Apparently the bot did the right thing here :)

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to James Graham [:jgraham] from comment #22)

Apparently the bot did the right thing here :)

Woohoo! Well done bot, & sorry for the noise. :)

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

WPT import (for layout/reftests/w3c-css/submitted/) in https://github.com/web-platform-tests/wpt/pull/17012