947365 - Make <input type=number>'s spinner the same height as its text input control

Status: NEW

(Core :: Layout: Form Controls, defect)

Description

[Jonathan Watt [:jwatt]]

Ground rules: <input type=number> and <input type=text> are supposed to look identical (both native themed and not) with the exception of the former having a spinner slapped on it. That spinner should not change the dimensions of the number control though.

Background: nsNumberControlFrame creates a native anonymous content tree for <input type=number> that looks like this:

  <input type=number>
    <div>                - outer wrapper with "display:flex" by default
      <input type=text>  - text input field
      <div>              - spinner box wrapping up/down arrow buttons
        <div>            - spin up (up arrow button)
        <div>            - spin down (down arrow button)

Right now the spinner often isn't as tall as it could be (it's less than the height of the text input field), which makes its buttons smaller and harder to use than they ideally would be. This is happening because we have to set the height of the spinner to zero to center it vertically and to stop it from affecting the height of the number control if the combined height of its buttons is more than the height of the spinner's text field sibling. We have to do this because native theming forces the size of the buttons to a minimum size that is too tall, particularly if the number control itself is not themed. If the buttons are too tall, we want them to just overflow rather than for them to increase the height of the number control.

This works fine to prevent buttons that are too tall from increasing the height of the number control. However it's suboptimal when the buttons are less than half the height of the text field. In this latter case the buttons just take on their minimum height because their spinner parent has a height of zero. What we actually want to have happen is that the buttons size as big as possible up to but no bigger than the height of the text field.

In other words the height of the <input type=number> and wrapper <div> should shrink wrap to the height of the <input type=text>, and then the spinner <div> should expand to the resulting height of the wrapper <div>.

What I don't understand is why setting |height:100%| on the spinner doesn't set its height to the height we want. In fact whatever percentage value I set seems no be interpreted as the height of the text field plus 2px. If I try and use calc(100% - 2px) that fails too (still the same height).

I'm not sure, but I think that bug 527285 would allow us to fix this.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #0)
> What I don't understand is why setting |height:100%| on the spinner doesn't
> set its height to the height we want. In fact whatever percentage value I
> set seems no be interpreted as the height of the text field plus 2px. If I
> try and use calc(100% - 2px) that fails too (still the same height).

I think that's just because it doesn't have anything to resolve the percentage against, right? (since its containing block -- the flexbox -- doesn't have a fixed height. It derives its height from its children, and we're one of the children)

IsAutoHeight, defined here...
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h?rev=9ceffd9514da&mark=1097-1097#1092
...says we treat percent heights against unresolved (nscoord_MAX) parent heights as "auto".

(I think the relevant caller of that function is
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=9ceffd9514da#3973
which is what we end up using to populate the reflow state's mComputedHeight value.)

Comment 2

[Daniel Holbert [:dholbert]]

Per IRC discussion, I think something like this should satisfy the requirements in comment 0:
 http://jsfiddle.net/bsrP5/2/

In particular: We sort of want the spinner to have the default "align-self: stretch" (instead of "align-self: center" like it currently is in forms.css), so that it can grow when the control is large. But, we can't just directly make that change, or else the spin-buttons' required heights will impact the minimum size of the form control (which we don't want).

So, that fiddle wraps the spinner in an additional vertical flexbox, and *that wrapper* is "align-self:stretch". That wrapper doesn't impact the minimum size of the control, because it has only one child (the spinner) which I've given "flex: 1 0px" (i.e. start at 0, and absorb any space that we're given).

Then, *inside* the spinner, the spin buttons themselves can maintain their own minimum sizes, and they'll overflow from the spinner in a balanced way when the form control (and consequently the spinner) are short, due to "justify-content: center" on the spinner.

Comment 4

[Greg Edwards]

Attachment: Screen Shot 2014-06-30 at 3.17.20 PM.png

They aren't scaling to DPI either. The spin control is unusably tiny on Windows high DPI.

(The lorem ipsum is 9 point Arial, for reference.)

Comment 5

[Mikhail Ryazanov]

Attachment: appearance in 34.0.5 64-bit version for Linux at various zoom levels

The current implementation seems to be completely broken. As can be seen in the attached picture (34.0.5, Linux, 64-bit), even in the default state it has wrong alignment (shifted slightly lower and significantly from the right edge). When the page is zoomed in or out (Ctrl++, Ctrl+-), the spinner just floats around unaffected.

Comment 6

[Mikhail Ryazanov]

Attachment: appearance in 34.0.5 version for Windows (classic theme) at various zoom levels

Comment 7

[Mikhail Ryazanov]

Attachment: appearance in 34.0.5 version for Windows (Win7 basic theme) at various zoom levels

Comment 8

[Mikhail Ryazanov]

> The current implementation seems to be completely broken.

The situation in Windows is not better: with the Win7 theme the spinner tries to scale horizontally (although the right margin is wrong) but not vertically, with the classic theme it does not scale at all and just has the right margin changing even worse than in Linux. In neither case the appearance is quite close to that of the native control (the Win7 case being worse).

Moreover, changing the default font size (Preferences → Content → Default font Size) apparently has no effect on the size of the spinner.

Comment 9

[voracity]

Having tried type=number for the first time, I would have thought this is a serious enough issue to disable the spinner buttons altogether until they're fixed. (As it stands, I have to hide them because of how broken they are. That's a pretty good sign it's not ready for release.)

Comment 11

[Sebastian Zartner [:sebo]]

I added the simplistic example from bug 1108469. Regarding comment 9, bug 978320 seems to be related.

Sebastian

Comment 12

[brohit4]

Is there any alternative way to scale the spin buttons.

Comment 13

[Greg Edwards]

You could try jQuery UI's Spinner control.

https://jqueryui.com/spinner/

Comment 14

[James B]

I believe this bug is also related to bug 1158435 and bug 1287681 -- maybe it would make sense to maintain a single issue to track?  Spinner arrows are totally unusable in their current state (on Windows, high-DPI).

Comment 15

[Leho Kraav (:macmaN @lkraav)]

Attachment: Screenshot_2021-01-27 HTML Examples Number Field Components Vaadin.png

Indeed, number field controls are in a sad sad state.

Design systems often go to much user-friendlier +/- buttons around the number input, illustrated in the attachment.

Why couldn't we have such sanity directly in the platform instead of fiddling with these micro-sized arrows?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

The good part is that this is getting fixed to some extent with widget.disable-native-theme-for-content=true. There should be more reasonably-sized spinners with that pref on.

Comment 17

[h-h]

The situation isn't as bad anymore as it once was (tiny, barely usable spinners -- at least with DPI scaling). But when I use the devtools on this MDN page to set the CSS font-size of the <input> to something bigger or smaller, the spinner stays the same size.

BTW: I don't have widget.disable-native-theme-for-content set; it doesn't exist.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 11 votes.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Behavior is much better now with the non-native theme.