1397768 - flex children of <button> are always vertically centered, despite align-items or justify-content

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Florens Verschelde :fvsch]

Attachment: button-flex-justify-content.html

Bug 984869 enabled display:flex (and inline-flex) for BUTTON elements.

When using flex layout to arrange the children of the BUTTON element, properties that distribute items on the vertical axis do not behave as expected:

- For flex-direction:row, flex items do not take the full height when using align-items:stretch
- For flex-direction:column, flex items are not distributed over the full height when using justify-content:space-between (or space-around or space-evenly).

This can create issues for authors who expect to lay out the contents of <button> elements the same way they would lay out the contents of a <a> or <span> element.

I’m attaching a test case, expanded from https://bugzilla.mozilla.org/show_bug.cgi?id=984869#c39

Comment 1

[Florens Verschelde :fvsch]

Attachment: Screenshot of test case in Firefox 57

Comment 2

[Florens Verschelde :fvsch]

Attachment: Screenshot of test case in Chrome 60

Comment 3

[Florens Verschelde :fvsch]

Attachment: Screenshot of test case in Safari 10.1

Comment 4

[Florens Verschelde :fvsch]

I’ve added screenshots of results in Firefox, Safari and Chrome.
Chrome seems to be the most flexible, Safari has the most issues, and Firefox is somewhere in between.

Comment 5

[Daniel Holbert [:dholbert]]

Yeah, I think what happens here is that we wrap the <button> contents in a helper box (which does *not* inherit width/height), and we manually center that box within the button.  Chrome clearly does something similar, on the first line of your testcase (the top two pinkish examples).

In Firefox, that button-internal layout happens here:
https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/layout/forms/nsHTMLButtonControlFrame.cpp#258-292

(I guess we do the centering differently for inline (horizontal) vs. block (vertical) axis, based on your oboservations -- though I don't remember precisely how/where.)

Unfortunately, the internal rendering/positioning of button contents is pretty underspecified, which is why this is so unpredictable & different between browsers.

If you want this to work reliably & work cross-browser, your best bet is to use an *explicit* wrapper div for the button's contents, with "height: 100%; width: 100%", and give that wrapper-div 100% height and width so that it explicitly fills the whole button.  I'll attach a testcase demonstrating that tweak.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase variant, using explicit wrapper div (which produces expected results)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (I guess we do the centering differently for inline (horizontal) vs. block
> (vertical) axis, based on your oboservations -- though I don't remember
> precisely how/where.)

Ah -- following up on this: we center the text horizontally in buttons via "text-align: center", here:
 https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/layout/style/res/forms.css#645,653

So the implicit wrapper box is, by default:
 - the full width of the button's content box (though it has text-align:center so its text contents still look centered)
 - only as tall as its contents (and vertically centered manually, via some C++ code rather than via CSS)
...because traditionally CSS had no way to reliably do vertical centering of arbitrary content.

Comment 8

[Oriol Brufau [:Oriol]]

When CSS Align is implemented, I think buttons should be styled with "align-content: safe center", and get rid of the magic wrapper.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Oriol Brufau [:Oriol] from comment #8)
> When CSS Align is implemented...

...for blocks (this is bug 1105571).

> ...I think buttons should be styled with
> "align-content: safe center", and get rid of the magic wrapper.

I think something like that could work, yes.  We'd still have the magic wrapper under the hood, but we could style it with "display:block; height:100%; align-content:center", which I think would center its contents vertically within it.  Or maybe we'd really want to give the *button* "align-content:center" and give the wrapper "align-content:inherit", so that authors could override the centering... That would be web-observable change, though (since authors can inspect the default style of buttons and could conceivably depend on it somehow), so this would involve some risk of breakage.

Comment 10

[Paul]

I just experienced the same issue. Here is a JSFiddle that shows the difference between a <div> and <button> parent element.

https://jsfiddle.net/ddvv6632/1/

Comment 12

[Daniel Holbert [:dholbert]]

We could perhaps fix this with a special case inside of nsHTMLButtonControlFrame to disable the anonymous box's vertical-centering behavior and replace it with "fill container height" behavior, depending on the type of the child box...  (For grid/flex, we'd want the anonymous box to fill the container height, where the container is the <button>.)

This is still hacky and underspecified, but it'd get us closer to interop/webcompat, and that would be a win...

Comment 13

[Daniel Holbert [:dholbert]]

(FWIW, Comment 12 would make us match Edge on the original testcase -- attachment 8905515 [details]. Chrome does something weird where they do fill the button's full height for a column-oriented flex container, but don't for a row-oriented flex-container.  I think it'd make sense to do all or nothing, though, and I don't see any reasonable justification for what they're doing.)

Comment 14

[Daniel Holbert [:dholbert]]

mats: I'd like to get a second opinion (beyond my own) -- would you be OK with the special case discussed in comment 12?  Or do you have any alternate ideas here?

Comment 15

[Mats Palmgren (inactive)]

Ideally, the button should behave as if it has no anonymous box
since it's an implementation detail.  So making the anonymous box
fill the button (in both axes) when it supports CSS Align seems
like the right thing to do.
I guess we need to add 'button { place-content: safe center }'
to the UA sheet too?  (IIRC, the anonymous box already inherits
all the relevant Align/Grid/Flex properties)

(I'm not really sure why we'd need an anonymous box in the first
place, but that's probably a more invasive change to consider
later...)

Comment 17

[Daniel Holbert [:dholbert]]

Tentatively assigning to mihir, to work on after his current bug. (Thanks, mihir!)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: screenshot of test case in Edge 17

For completeness, here's a screenshot of the original testcase, in Edge 17. As noted in comment 13, I think this is the most coherent behavior & the thing we want to match.

(This is almost equivalent to what Chrome does, except that Chrome renders the two red boxes in the first row with the contents being too short.)

Comment 19

[Daniel Holbert [:dholbert]]

I suspect we'll want to fix this by inserting a check for flex/grid frames here:
https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/layout/forms/nsHTMLButtonControlFrame.cpp#245-256
...and copying the button's ReflowInput's BSize (and min/max-BSize) onto the child's ReflowInput:

Something like:
 const LayoutFrameType t = aFirstKid->Type();
 if (t == LayoutFrameType::FlexContainer ||
     t == LayoutFrameType::GridContainer) {
   contentsReflowInput.ComputedBSize() = aButtonReflowInput.ComputedBSize();
   // ... same for ComputedMaxBSize and ComputedMinBSize ...
 }

That might be all we need, though there may be other things needed, too.

Comment 20

[Mihir Iyer [:mihir]]

Attachment: Bug1397768v1.patch

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 9002011 [details] [diff] [review]
Bug1397768v1.patch

Review of attachment 9002011 [details] [diff] [review]:
-----------------------------------------------------------------

There are a number of things we can simplify in the tests here -- leaving comments on the flex one, but they mostly apply to all of the test files I think:

::: layout/reftests/forms/button/button-display-flex-fullsize-1.html
@@ +18,5 @@
> +      margin: 10px 20px 10px 0;
> +      padding: 0;
> +      border: solid 1px;
> +      border-color: black;
> +      background-color: #ddd;

This background-color never gets to take effect since it's overridden by the other one that currently lives on button.button. Let's simplify by getting rid of it.

@@ +20,5 @@
> +      border: solid 1px;
> +      border-color: black;
> +      background-color: #ddd;
> +      font: inherit;
> +      font-size: 14px;

This font styling isn't needed, particularly if we get rid of the examples that have text (as I think we can/should, per below) -- let's get rid of it.

@@ +23,5 @@
> +      font: inherit;
> +      font-size: 14px;
> +    }
> +
> +    .button>* {

This should just be "button >"

@@ +37,5 @@
> +
> +    button.button {
> +      color: #A00;
> +      background-color: lightblue;
> +    }

Don't bother specifying "color" since that just controls the text and you probably don't need the buttons that have text (the 3rd and 4th ones), as noted below.

@@ +41,5 @@
> +    }
> +    body {
> +      margin: 20px;
> +    }
> +    @media (min-width:1000px) {

Get rid of this whole @media(){...} chunk -- it's unused/unneeded.

And get rid of the "body" rule above it, too -- similarly unneeded.

@@ +57,5 @@
> +    <div></div>
> +    <div></div>
> +    <div></div>
> +  </button>
> +  <button class="button pseudo vertical">

The "pseudo" class isn't used/referenced anywhere, so let's get rid of it.

And the "button" class is unnecessary as noted above for its CSS, so let's get rid of it too.

@@ +65,5 @@
> +  </button>
> +  <br>
> +  <button class="button">
> +    <span>Left</span>
> +    <span>Right</span>

I think you can simplify by getting rid of the 3rd and 4th button here.  Given that class="pseudo" doesn't do anything here, it looks like they're the same as the first and second ones, but with 2 items instead of 3 and with text instead of no-text (neither of which are really relevant to what's being tested)

Comment 22

[Daniel Holbert [:dholbert]]

> And the "button" class is unnecessary as noted above for its CSS, so let's get rid of it too.

Sorry, disregard this comment about the button class -- that one does actually seem to be useful after all. (I initially thought we should remove it but then realized it's kinda handy for keeping similar styles between testcase & reference case, so I removed another note about removing it, but forgot to adjust this additional note.)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> > +    .button>* {
> 
> This should just be "button >"

(similarly, this part is actually just fine as-is for testcase::reference-case consistency, as I later realized RE the .button class being useful after all. So it's fine with or without the period here (matching based on tagname or classname).

Comment 24

[Mihir Iyer [:mihir]]

Attachment: Bug1397768v2.patch

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 9002028 [details] [diff] [review]
Bug1397768v2.patch

Review of attachment 9002028 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/forms/button/button-display-flex-fullsize-1.html
@@ +17,5 @@
> +      vertical-align: top;
> +      margin: 10px 20px 10px 0;
> +      padding: 0;
> +      border: solid 1px;
> +      border-color: black;

Let's just collapse the border styling to:
 "border: solid 1px black"
in all of the files here.

(Right now some of the files don't specify border-color, and some do. Let's just specify it consistently and on the same line.)

I see a few other minor simplification opportunities but they don't really matter, so r=me with this. :)

Comment 26

[Mihir Iyer [:mihir]]

Attachment: Bug1397768v3.patch

Comment 27

[Mihir Iyer [:mihir]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8091cf18a72cb889f75ad8fe1b612abd60153caf

Comment 28

[Daniel Holbert [:dholbert]]

The reftest run hit some failures, which are due to us inserting a little extra space above the button's contents, in the reference case.  This is because its contents (its wrapper-div) is display:inline-flex there, and we're doing some silly baseline alignment between that element and the container (the button).  Really, we want to just use display:flex -- we don't want any inline-layout activity to be happening on that box inside the button.

I've tweaked the patch locally to make that change and a few other changes while I'm at it, and I'll post that shortly with a Try run.

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: fix v4

Comment 30

[Daniel Holbert [:dholbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fdb66c1db26df23e6aa12872567a929dff39e32

Comment 31

[Daniel Holbert [:dholbert]]

The first Try run (comment 27) also had some Android R33 failures with fuzzy pixel mismatches on all 4 corners of the button. I'm guessing this is from some sort of Android-specific subtle bug/behavior around compositing of partially-transparent borders/backgrounds.

I've tweaked the test locally to use a fully opaque border & background on the items - here's a try run (Android-only) with that change -- hopefully that addresses these R33 failures.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af23a3b9c073ef8328255659c98cae3f3af2c70

Comment 32

[Daniel Holbert [:dholbert]]

That's still got antialiasing issues around the border.  Once more with no borders at all:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5306a09e617453e6edff45c2ebc823ab945f32ba

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5477aa895b6c
part 1: Make 'display: flex/grid' in a button take up all the available space. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b7d002a6d5
part 2: Adjust reftests for clarity & to reliably pass on all platforms. (test-only)

Comment 34

[Daniel Holbert [:dholbert]]

Green Try push for final (landed) tweaks to tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b98c685607301ae6a632ba29bede5e1eb2459b0e

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5477aa895b6c
https://hg.mozilla.org/mozilla-central/rev/f0b7d002a6d5

Comment 36

[Daniel Holbert [:dholbert]]

For reference: I filed bug 1484841 on the weird Android button-corner quirks that caused reftest oranges in the initial Try runs here. (the Android reftest failures in comment 27, and comment 30 & comment 32)