1740580 - Change sizing behavior of out-of-flow semi-replaced elements (absolutely positioned form fields, inputs, buttons, etc)

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Daniel Holbert [:dholbert]]

I'm preemptively filing this bug to cover whatever resolution comes out of https://github.com/w3c/csswg-drafts/issues/6789 (regarding how to size buttons and other semi-replaced elements when they're out-of-flow).

(I'm partly filing early to have a place where I can post a few testcases to exercise a variety of such elements, for testing/investigation purposes.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1: various abspos form controls with top/right/bottom/left:3px

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in Firefox Nightly 96 on Linux

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in Chrome dev 97 on Linux

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 in Safari 15.1 on MacOS Monterey (12.0.1)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: now with explicit `width:auto; height:auto` on the elements

IanK pointed out that some elements have a specified height/width in the UA stylesheet, which makes them appear to be doing shrink-to-fit when really they're just honoring that specified size.

Here's a version of the testcase with width:auto;height:auto to override those, where possible.

Comment 6

[Daniel Holbert [:dholbert]]

We've resolved on the 'stretch' behavior:
https://github.com/w3c/csswg-drafts/issues/6789#issuecomment-971805915

So, the expectation is that the green outline should grow to fill the black border (aside from any margin applied via UA stylesheet rules), in all of the cases here (except for radio/checkbox).

Comment 7

[Daniel Holbert [:dholbert]]

IanK says Blink is planning on implementing and shipping this soon (it's roughly a one-line change for them, I think), and they're going to watch for webcompat issues, though they're not anticipating much.

We might want to implement this soon after that, after it's been demonstrated to be web-compatible.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)

IanK says Blink is planning on implementing and shipping this soon (it's roughly a one-line change for them, I think), and they're going to watch for webcompat issues, though they're not anticipating much.

FWIW, Chrome did end up shipping this. I just checked Chrome 110, and it does indeed show the green outline growing to fill the black bordered area (aside from margin) in all parts of testcase 2. Same goes for all parts of testcase 1 except for a few widgets where they've got a UA-stylesheet-imposed default height and width (<input type="color">, progress, and meter).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (now with zero margin and zero offsets)

Here's a testcase that's a bit easier to mock up a reference case for.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: reference case 3

This is reference case for testcase 3 (I've just replaced auto with 100% for width and height, with box-sizing:border-box, which should fill the available space just like auto should in the testcase.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 3 in Chrome

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: screenshot of reference case 3 in Chrome

Comment 13

[Daniel Holbert [:dholbert]]

If you compare my Chrome screenshots side-by-side, you can see that the input type="image" (the first item in the second row) has a subtly different position, which I think is a Chrome bug (unless there's some subtle thing that my testcase should be accounting for that I'm not seeing).

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1405560 on that.

Comment 14

[Ian Kilpatrick [:iank]]

One small note - we also ended up changing our behaviour for radio/checkbox to make consistent with another bug - https://bugs.chromium.org/p/chromium/issues/detail?id=768999

Comment 17

[Daniel Holbert [:dholbert]]

[adding a few extra words to the bug summary for better discoverability when duping bugs here]

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1740580 - Align shrink-wrap behavior of semi-replaced elements with the spec. r=dholbert,#layout

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Daniel, there's some orange to look into here but I'd appreciate a sanity check on the approach if you have a few cycles.

Comment 22

[Daniel Holbert [:dholbert]]

I'm mostly AFK today, but I'll take a look when I've got a chance, on Monday if not sooner. Thanks for fixing this - this has been on my list of compat bugs to circle back to!

Comment 23

[Daniel Holbert [:dholbert]]

So skimming the patch, it looks like main non-cleanup changes are:
(1) Remove ComputeAutoSize overrides on semi-replaced elements
(2) Add some logic to the Reflow methods, to set the aDesiredSize outparam to the bp-inflated aReflowInput.ComputedSize(wm) value (with unconstrained block-size replaced with a frame-class specific default value (depending on some context), matching what the old ComputeAutoSize method would have done.

That seems reasonable; I'm not yet seeing how the proposed 'stretch' abspos auto-sizing behavior comes into play yet -- maybe that falls naturally out of the above, or maybe that's in a spot I didn't notice yet in my skimming.

Could you summarize the approach to fill in any nuance that I might be missing here (particularly relating to how this gets us the stretch behavior)?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yeah, sure. The magic bit is also removing the IsReplaced() check from this condition, which makes nsContainerFrame::ComputeAutoSize not shrink-wrap for all these on the inline axis.

So with those two things, all these elements no longer shrink-wrap by default, unless you pass the relevant ShrinkWrap flag. For abspos, we'd hit this code-path as needed.

I just realized I need to tweak this code to address some regressions that I just found out on try (e.g. with my patch as-is right now, <button style="display: block"> no longer shrink-wraps (even if not-abspos). But that should be trivial to address, it's just a matter of getting that condition right, the meat of the patch is there and should be right. Does that explain the situation? Sorry, the IsReplaced() check in ComputeAutoSize was easy to miss.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Should be green now.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/jobs?repo=try&revision=95a5ab49f0099059f4a09444975d7817cb3136aa fwiw

Comment 27

[Daniel Holbert [:dholbert]]

In a debug-opt build with this patch applied (from moz-phab patch D204797 --apply-to . just now), I get two copies of this assertion when I load each of the testcases here:

###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || r.width == nscoord_MAX || r.height == nscoord_MAX || HasAnyStateBits(NS_FRAME_SVG_LAYOUT) || r.Contains(nsRect(nsPoint(), aNewSize))', file layout/generic/nsIFrame.cpp:10224

(vs. no assertions in the same build with the patch popped off).

Could you take a look at that? Maybe we're failing to properly the set/inflate the overflow areas for some of these frames, at their new sizes?

Comment 28

[Daniel Holbert [:dholbert]]

I actually get that assertion failure when loading or running this WPT, too:
https://searchfox.org/mozilla-central/rev/f63ca2952da98e0817bdae0ddf1314281a497106/testing/web-platform/tests/css/css-position/position-absolute-semi-replaced-stretch-other.html#1

It looks like your Try run may not have hit it since it was just using opt builds. That WPT does fail-due-to-assertion-failures for me when I run it locally -- the output ends with:

~~~~~~~~~~~~~~~~~
Ran 2 checks (1 asserts, 1 tests)
Expected results: 1
Unexpected results: 1
  assert: 1 (1 fail)

So I think we have test coverage for the thing in comment 27, which is good; no need for new tests on that front. :)

Comment 29

[Daniel Holbert [:dholbert]]

I confirmed that (in a patched build) our rendering of the attached testcases is consistent with Chrome's implementation, with the exception of the progress and meter frame in testcase 1, which are smaller in Chrome because Chrome's UA stylesheet seems to assign those an explicit length-valued width (or inline-size) which disables the auto-sizing stretch behavior. (That's sort-of them exposing an implementation detail, and it's arguably better on our side that we're not exposing that.)

(Relatedly, the <input type="color"> button seems to stand out as being small for us and for Chrome in testcase 1, for the same reason; we both give that element an explicit size in the UA stylesheet -- here on our side -- and the presence of that style disables the width:auto stretch behavior that it would otherwise get.)

Comment 30

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #27)

Could you take a look at that? Maybe we're failing to properly the set/inflate the overflow areas for some of these frames, at their new sizes?

Hmm I don't recall anything like that from developing locally, probably an artifact of me refactoring the code to fix the comment about repetitiveness (which is totally fair). I bet I just accidentally removed a SetOverflowAereaToDesiredBounds somewhere. Will look tomorrow.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61155e32696f
Align shrink-wrap behavior of semi-replaced elements with the spec. r=dholbert,layout-reviewers

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c342123c6f
fix reftest failures on 307102-3.html. r=fix CLOSED TREE

Comment 33

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/61155e32696f
https://hg.mozilla.org/mozilla-central/rev/69c342123c6f