Closed Bug 984869 Opened 10 years ago Closed 8 years ago

Add support for display:flex/grid and columnset layout inside <button> elements

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: rik, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug)

Details

(Keywords: testcase, Whiteboard: [webcompat])

Attachments

(4 files)

Attached file Testcase
In the attached testcase, foo and bar should be on the same line.

The attached testcase works on Chrome and Safari (with prefix for Safari).
Attachment #8392843 - Attachment mime type: text/plain → text/html
Attached file Workaround
Wrapping the content of <button> in a <div> with display: flex works.
<button> is not implementable (by browsers) in pure CSS, so they are a bit of a black box, from the perspective of CSS. This means that they don't necessarily react in the same way that e.g. a <div> would.

This isn't specific to flexbox -- e.g. we don't render scrollbars if you put "overflow:scroll" on a button, and we don't render it as a table if you put "display:table" on it.

Stepping back even further, this isn't specific to <button>. Consider <fieldset> and <table> which also have special rendering behavior:
 data:text/html,<fieldset style="display:flex"><div>abc</div><div>def</div>
 data:text/html,<table style="display:flex"><div>abc</div><div>def</div>

In these cases, Chrome agrees with us and disregards the "flex" display mode. (as revealed by the fact that  "abc" and "def" end up being stacked vertically). The fact that they happen to do what you're expecting on <button style="display:flex"> is likely just due to an implementation detail.

In Gecko's button implementation, we hardcode <button> (and <fieldset>, and <table>) as having a specific frame class (and hence, a specific way of laying out the child elements), regardless of the "display" property.

If you want to reliably have the children reliably arranged in a particular layout mode in a cross-browser fashion, your best bet is to use a wrapper-div inside the button, just as you would need to inside of a <table> or a <fieldset>.
I actually think the Chrome/Safari behavior might not be spec-compliant. I'm not 100% familiar with how the WHATWG spec works, but it looks to me like they define <button> as having to render as an inline-block:

> When the button binding applies to a button element,
> the element is expected to render as an 'inline-block'
> box rendered as a button whose contents are the contents
> of the element.

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-button-element-0

That's a bit hand-wavy (with the "button binding" text, and the fact that e.g. "display:block" would make the button block-level (not an inline-block) from the perspective of its parent). But at least as far as how the <button> behaves *as a container for its children*, that says to me that the Firefox behavior is correct here.

Hence, resolving as INVALID, and CC'ing bz for a sanity-check on that.

(It might even be worth filing a blink bug on this -- though perhaps this territory is undefined enough that neither behavior is really in violation of any spec. (depending on your reading of the above spec text).)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
The spec on all this stuff is basically nonexistent...  hixie's spec is not only handwavy, it also doesn't match actual browser behavior last I checked.
That's unfortunate. The way I understood flex here, it only influences the children, not the element itself. The only thing I can see in the flexbox spec that affects the element is flex vs inline-flex.

As a web author, this introduces confusion. When I think about <button> elements, I treat them as <div tabindex=0 style="appearance: button">. So it's frustrating to see something like this not working. This is an annoying restriction to make authoring friendly.
The fact that Chrome can do it makes me think that this is just an implementation detail and that we should have specs that allow it.

Also, the more restrictions we put on <button>, the more authors will use workarounds that might be less accessible.

This is a bit ranty and off-topic. I get that it might not be trivial to change but marking this as invalid seems too much.
(In reply to Anthony Ricaud (:rik) from comment #5)
> That's unfortunate. The way I understood flex here, it only influences the
> children, not the element itself.

It influences both, on elements that support custom display values.  And old-timey HTML elements like <button> and <table> and <fieldset> simply do not support custom display values*, in Gecko (and Chrome agrees with us on the latter two), as I indicated above.

* other than for the purposes of answering the very high-level question of "is this element block-level or inline-level", for flowing other content around the element.

> The only thing I can see in the flexbox
> spec that affects the element is flex vs inline-flex.

The flexbox spec is not the thing to look at here. You're correct that it (and other CSS specs) don't make any special exceptions for buttons or tables or fieldsets or the like. If this behavior were defined anywhere, it would be in the HTML5 spec probably, but as bz indicates, it's not really.

> As a web author, this introduces confusion. When I think about <button>
> elements, I treat them as <div tabindex=0 style="appearance: button">.

That's close to how they're implemented in Gecko, except that the <div> is anonymous & is wrapped in the button-frame, which does some additional centering (both vertically and horizontally) under-the-hood.

> The fact that Chrome can do it makes me think that this is just an
> implementation detail

Yes. (on both sides -- they happen to implement <button> as something closer to your mental model than we do, apparently, but they don't for other elements like <table> and <fieldset>)

> and that we should have specs that allow it.

It would be great to have this stuff specified & interoperable -- agreed.

> I get that it might not be trivial to
> change but marking this as invalid seems too much.

Sorry if I offended with the INVALID resolution.  For a bug about "this behavior is broken", INVALID is the correct resolution if we're not actually violating a spec or doing anything that's clearly wrong.

If you intended to file this as an enhancement "it would be great if this unspecified thing could work" sort of bug, then feel free to reopen & perhaps clarify the summary to make that clearer. Just keep in mind that it may not be actionable without completely reimplementing how buttons work (which would itself be a bit scary, since <button> has existed since forever & hence it may break sites to fundamentally change how we render them). :)
Just for the record, <table> does in fact support custom display values; it's box is constructed purely based on display value, not on tag name.

The HTML tags that can contain arbitrary and have boxes constructed based on tag name are just <button>, <fieldset>, and <legend>.
(In reply to Boris Zbarsky [:bz] from comment #7)
> Just for the record, <table> does in fact support custom display values;
> it's box is constructed purely based on display value, not on tag name.

(Huh -- there does seem to be some quirkiness based on tag name -- at least, compare:
 data:text/html,<!DOCTYPE html><table style="display:flex"><div>aaa</div><div>bbb</div>
 data:text/html,<!DOCTYPE html><div style="display:flex"><div>aaa</div><div>bbb</div>

The <table> version renders as two lines, the other one renders as one line. Firefox/Chrome/Opera all agree on this. This is the extent to what I'd tested about this on <table> up until right now.

Digging slightly deeper, a frame dump from layout debugger seems to show that we do get a nsFlexContainerFrame for the <table>, but it's empty and it's a sibling of aaa/bbb instead of being their parent. All three of them have the <body>'s block frame as their parent. This seems odd; perhaps it has to do with table-fixup or something else table-specific that happens regardless of display value.)
The HTML parser will hoist non-table-row/section kids of <table> up to be siblings of the <table> in the DOM.  But that has nothing to do with the CSS/layout end of things.  If you create the DOM you wanted in that case via explicit insertBefore() calls on the table, you'll see that it does actually work correctly to change the <table>'s display value.
See Also: → 1047590
The resolution of this bug as 'invalid' is incorrect! Whenever WHATWG HTML talks about the rendering of the element, it means the *default* rendering, with UA styles or similar mechanism (https://html.spec.whatwg.org/multipage/rendering.html#introduction-16):

> In the absence of style-layer rules to the contrary (e.g. author style sheets),
> user agents are expected to render an element so that it conveys to the user
> the meaning that the element represents, as described by this specification.

It doesn't prohibit to change this default display of the element with author or user styles, e.g. to display a link as a block, a list as a table row or a button as a flex container.

Does the spec prohibit to set to the lists (https://html.spec.whatwg.org/multipage/rendering.html#lists) or to the `details` element (https://html.spec.whatwg.org/multipage/rendering.html#the-details-element-2) the left padding other than 40px? If no, why should the `display` of the `button` be any different?

I believe that this bug must be reopened.
(In reply to Ilya from comment #14)
> It doesn't prohibit to change this default display of the element with
> author or user styles, e.g. to display a link as a block, a list as a table
> row or a button as a flex container.

It doesn't prohibit changing the 'display', but in this case (buttons) it does actually say that it shouldn't do anything. (See my spec-quote below.)

> Does the spec prohibit to set to the lists
> (https://html.spec.whatwg.org/multipage/rendering.html#lists) or to the
> `details` element
> (https://html.spec.whatwg.org/multipage/rendering.html#the-details-element-
> 2) the left padding other than 40px?

No -- it doesn't prohibit changing the padding.  Note that those elements default-padding behavior is expressed in the spec using the "padding" property, so it makes sense that author-defined "padding" will override that.

> If no, why should the `display` of the
> `button` be any different?

The <button> default rendering is not expressed in the WHATWG spec using the "display" property. So, setting "display" doesn't change the button's default rendering.

The spec says:

   # A number of elements have their rendering defined in terms of the 'binding' property.
   # [...] Exactly how the bindings are implemented is not specified by this specification.
   # 14.5.2 The button element
   #   button { binding: button; }
   # When the button binding applies to a button element, the element is expected to
   # render as an 'inline-block' box rendered as a button whose contents are the
   # contents of the element.
https://html.spec.whatwg.org/multipage/rendering.html#the-button-element-2

Note that "binding" property is a bit of hand-wavy spec-craft, and I don't think anyone actually implements buttons directly that way. Still: the spec is saying here that a button (technically anything with a "button binding" [hand-wave]) "is expected to render as an 'inline-block box'.  There's no mention of the 'display' property.
(In reply to Daniel Holbert [:dholbert] from comment #15)
> It doesn't prohibit changing the 'display', but in this case (buttons) it
> does actually say that it shouldn't do anything. (See my spec-quote below.)
> ...
> The <button> default rendering is not expressed in the WHATWG spec using the
> "display" property. So, setting "display" doesn't change the button's
> default rendering.
> 
> The spec says:
> 
>    # A number of elements have their rendering defined in terms of the
> 'binding' property.
>    # [...] Exactly how the bindings are implemented is not specified by this
> specification.
>    # 14.5.2 The button element
>    #   button { binding: button; }
>    # When the button binding applies to a button element, the element is
> expected to
>    # render as an 'inline-block' box rendered as a button whose contents are
> the
>    # contents of the element.
> https://html.spec.whatwg.org/multipage/rendering.html#the-button-element-2
> 
> Note that "binding" property is a bit of hand-wavy spec-craft, and I don't
> think anyone actually implements buttons directly that way. Still: the spec
> is saying here that a button (technically anything with a "button binding"
> [hand-wave]) "is expected to render as an 'inline-block box'.  There's no
> mention of the 'display' property.

I still can't agree that the definition from the spec means that the "inline-block-like" default display of the button can't be changed with CSS.

First, the intro to the bindings section (https://html.spec.whatwg.org/multipage/rendering.html#introduction-17) says:

   # The rules then described for these bindings are only expected to apply
   # if the element's 'binding' property has not been overridden (e.g. by the author)
   # to have another value.
   #
   # Exactly how the bindings are implemented is not specified by this specification.
   # User agents are encouraged to make their bindings set the 'appearance' CSS property
   # appropriately to achieve platform-native appearances for widgets, and are expected
   # to implement any relevant animations, etc, that are appropriate for the platform.

It explicitly says that the rendering rules for the binding *can* be overridden by the author, and suggests that changing the `appearance` property can also change the binding-related rendering rules.

Second, the bindings section of the spec refers to the Behavior Extensions to CSS spec, also written by Hixie. The current version (http://www.w3.org/TR/becss/) is the Work Group Note basically saying that this proposal has failed, but the previous 2007 version (http://www.w3.org/TR/2007/WD-becss-20071019/#the-binding) defined the `binding` property as a way to use XBL bindings in CSS. The main idea was to have a mechanism to convert a single element to the rich UI widget. In 2015, the most standard approach for this is Web Components (Shadow DOM etc.). But I don't see any reason why we shouldn't be able to change the display of the root element of such widget.

I can understand why the `button` element may need to be presented as atomic inline-level box (although personally I often miss the possibility to style buttons as regular inline elements, for consistency with links and even images displaying the alt text). But what in the spec prevents us from changing the behavior of children of this element, especially if the `appearance` of the element was explicitly changed?
(In reply to Ilya from comment #16)
> I still can't agree that the definition from the spec means that the
> "inline-block-like" default display of the button can't be changed with CSS.

I didn't exactly say that it can't be changed with CSS; it just can't be changed directly by the "display" property.

> It explicitly says that the rendering rules for the binding *can* be
> overridden by the author, and suggests that changing the `appearance`
> property can also change the binding-related rendering rules.

Right; "appearance" changes how buttons are displayed.  And to the extent that the binding itself is implemented in CSS, you could presumably override those aspects of the binding. The implementation of "binding" is unspecified, though, so the extent to which any property has an effect on the binding's behavior is unspecified.

> But I don't see any reason why we shouldn't be able to change the display of the
> root element of such widget.

At a practical level, in Gecko, the reason you can't is: the root element *isn't actually the thing whose display you need to change*.  The thing that actually contains the button's children is an anonymous box, whose 'display' is independent of the <button> element's 'display'.

To get a button to behave like a flex container, that anonymous box is the thing that you need to restyle. (And you can't right now, because it's anonymous.)

> But what in the spec prevents us from
> changing the behavior of children of this element, especially if the
> `appearance` of the element was explicitly changed?

The sentence I quoted in comment 15.

(Note that that sentence doesn't make any requirements about the appearance of the button, so it's unsurprising that the 'appearance' property can change how the button renders, aesthetically.)
Also, even if I'm reading too much into the sentence from comment 15 w.r.t. spec requirements, the fact remains that "binding: button" is a black box (in our case, implemented with an anonymous block inside of the button), so the behavior of something like "display" as applied to it is, at best, unspecified.
(In reply to Daniel Holbert [:dholbert] from comment #18)
> the fact remains that "binding: button" is a black box (in our case,
> implemented with an anonymous block inside of the button), so the behavior
> of something like "display" as applied to it is, at best, unspecified.

I agree with this. But, if this extra box will be accessible via pseudo element (like `::-moz-focus-inner`), couldn't we give it `display:contents` and make the root of the "black box" interact with its children directly?
(In reply to Daniel Holbert [:dholbert] from comment #17)
> it's unsurprising that the 'appearance' property can
> change how the button renders, aesthetically.)

According to the current editor's draft of CSS UI Level 4 (http://dev.w3.org/csswg/css-ui-4/#appearance-switching),

   # All decorative visual aspects of a form control which are not caused
   # by a CSS style rule must be suppressed when the appearance is changed
   # using 'appearance', even if the UA’s internal representation for this element
   # was composed of multiple elements or pseudo elements combined together.

So the change of the appearance is clearly more than just aesthetics, it's also significant part of visual behavior.
If those semantics stabilize then when we implement them doing "appearance: none" will in fact remove all the buttonlike rendering behavior from it.  It will just start behaving like a non-replaced box completely controlled by its display in terms of its rendering behavior.

Of course then it would not look in any way like a button.
(In reply to Ilya from comment #19)
> if this extra box will be accessible via pseudo
> element (like `::-moz-focus-inner`), couldn't we give it `display:contents`
> and make the root of the "black box" interact with its children directly?

Yes -- and in fact, it *is* accessible *to Firefox internal code* via "::-moz-button-content".  We don't expose that class to unprivileged web content, though, because its existence is an implementation detail, and we don't want web content growing to depend on it (which would prevent us from changing the internals in the future, in ways that would be transparent to web content if we don't expose this detail to the web). Browser-specific prefixed hooks for implementation details also hurt interop, just like browser-specific prefixed CSS hurts interop.

So, you're right, and there already is such a selector internally -- but I don't see us exposing this class to the web.

Anyway -- if you're OK with targeting your CSS at that inner box instead of restyling the button directly, then surely it's not too much of a stretch to just provide your own wrapper-div inside the button (as suggested at the end of comment 2), which you can restyle to be a flex container or any sort of container that you like. :)
Someone just brought this up on IRC again as something they'd stumbled over.

Moreover, new data since this was RESOLVED|INVALID:
 - We decided to support display:flex/display:grid on <fieldset> (bug 1230207).
 - I've tested MS Edge 13, and they behave like Blink & WebKit here. So, we are the only modern web rendering engine to not have this particular behavior on <button style="display:flex">.  So from a web compatibility perspective, we should probably align with the other engines on this.

So, I'm going to reopen this, as a valid bug from a web-compat perspective if not a spec perspective.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Flags: needinfo?(dholbert)
Whiteboard: [webcompat]
Let's do this, patches coming up...
Assignee: nobody → mats
Summary: display: flex doesn't work for button elements → Add support for display:flex/grid and columnset layout inside <button> elements
@tn, do you have an ETA for the review here?
Or do you want me to ask someone else?
Flags: needinfo?(tnikkel)
Flags: needinfo?(tnikkel)
Attachment #8794799 - Flags: review?(tnikkel) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40fb8eae281b
Add support for display:flex/grid and columnset layout to <button>.  r=tn
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3af7fd9bfe
Reftests with display:flex/grid and columnset layout on <button>.
https://hg.mozilla.org/mozilla-central/rev/40fb8eae281b
https://hg.mozilla.org/mozilla-central/rev/1e3af7fd9bfe
Status: REOPENED → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Whenever I open a page including statistics of fritz.box, the browser crashes (if e10s enabled, the tabs only crash). Sample crash dump: https://crash-stats.mozilla.com/report/index/7dd8c317-355f-4e07-9e22-ea8792161009
Apparently this signature started appearing 2016-10-07.

Used mozregression now for the first time, with this result: 

2016-10-09T20:41:58: INFO : Narrowed inbound regression window from [6ad9a7fc, 1e3af7fd] (4 revisions) to [ffb38910, 1e3af7fd] (2 revisions) (~1 steps left)
2016-10-09T20:41:58: DEBUG : Starting merge handling...
2016-10-09T20:41:58: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=1e3af7fd9bfe507cdcff5e05a95c3a763617f25c&full=1
2016-10-09T20:42:00: DEBUG : Found commit message:
Bug 984869 - Reftests with display:flex/grid and columnset layout on <button>.

2016-10-09T20:42:00: INFO : The bisection is done.
2016-10-09T20:42:00: INFO : Stopped
Depends on: 1308793
Thanks for the report Bruno, I filed bug 1308793 to handle that.
Depends on: 1316649
Any hope of getting this in? Having `display:inline-flex` on a button would allow us to align words and icons perfectly. We're already an established design system, so we can't break everyone by adding a wrapper in our buttons. But Firefox is the only browser that makes this not possible.
It should be in Firefox 52 which release is March 6 I believe.
Fabulous news, thanks so much!
Hello,

Not sure that this bug is really fixed. I tried on a button with pseudo-elements, and it seems that justify-content doesn't appply. I tried with align-content too :(

https://codepen.io/raphaelgoetter/pen/VzOOJy

It's OK on Chrome
(In reply to Raphael from comment #39)

> I tried on a button with pseudo-elements, and it seems that justify-content doesn't apply.

It doesn't apply correctly in the vertical axis. I’ve created bug 1397768 for that.
Yeah, that's because the button internal wrapper-box has default styling/layout that makes it shrinkwrap its contents' height.  So in your codepen's case, justify-content isn't doing much because there's no extra space to do justification with, because the wrapper-box has shrinkwrapped the contents.

Chrome does something similar (though not quite the same) as shown on the first line of Florens's testcase over in bug 1397768.

If that vertical shrinkwrapping is not the behavior you want, your best bet is to wrap your button contents with an explicit wrapper div with "height:100%", and make that wrapper-div a flexbox.
Flags: needinfo?(dholbert)
> that makes it shrinkwrap its contents' height.

Note that this is rather necessary for the vertical centering buttons are supposed to do by default....
Depends on: 1394631
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: