Closed Bug 1552287 Opened 4 months ago Closed 4 months ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- disabled
firefox69 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Depends on: 1553548

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

Attachment #9066800 - Attachment description: Bug 1552287 part 1: [css-contain] Fix some CSS layout-containment web-platform-tests to make their assumptions more valid. → Bug 1552287 part 1: [css-contain] Fix some CSS layout-containment web-platform-tests to make their assumptions more valid. r?TYLin
Attachment #9066801 - Attachment description: Bug 1552287 part 2: [css-contain] Adjust various reflow & baseline methods so that layout-contained frames behave as if they have no baseline. → 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

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

Duplicate of this bug: 1539586
Pushed by dholbert@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16982 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

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

(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.)

Upstream PR was closed without merging

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.

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.

Flags: needinfo?(dholbert)

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

Pushed by dholbert@mozilla.com:
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

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.

Flags: needinfo?(james)
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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

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

Apparently the bot did the right thing here :)

Flags: needinfo?(james)
Upstream PR merged

(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. :)

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

Duplicate of this bug: 1539589
You need to log in before you can comment on or make changes to this bug.