1437641 - Migrate <textbox type=numberbox> consumers to <input type=number> and remove the binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[:Gijs (he/him)]

Off-hand I don't see any real reason not to just move them to the HTML equivalent and remove all the extra gunk. I don't think there's any features the very few consumers in the tree need that the numberbox provides that html:input type=number doesn't - and if there are, they can add custom JS on the consumer side.

Comment 1

[Tim Nguyen :ntim]

The main difference between textbox[type=number] and input[type=number] is the fact that you can't type invalid values inside textbox[type=number]. Eg. you can type letters and out of bound numbers in input[type=number] and the input would just get the invalid state without preventing the values from being typed. textbox[type=number] however, prevents you from typing invalid values.

Comment 2

[:Gijs (he/him)]

(In reply to Tim Nguyen :ntim from comment #1)
> The main difference between textbox[type=number] and input[type=number] is
> the fact that you can't type invalid values inside textbox[type=number]. Eg.
> you can type letters and out of bound numbers in input[type=number] and the
> input would just get the invalid state without preventing the values from
> being typed. textbox[type=number] however, prevents you from typing invalid
> values.

I think for the consumers we have (IIRC, the proxy port fields in the advanced network dialog and one or two items in the print preview toolbar), we can either add that behaviour in custom JS or just omit it and validate that the field is valid when accepting the dialog / trying to print (and ensuring styling / form field warnings etc. work correctly).

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #2)
> (In reply to Tim Nguyen :ntim from comment #1)
> > The main difference between textbox[type=number] and input[type=number] is
> > the fact that you can't type invalid values inside textbox[type=number]. Eg.
> > you can type letters and out of bound numbers in input[type=number] and the
> > input would just get the invalid state without preventing the values from
> > being typed. textbox[type=number] however, prevents you from typing invalid
> > values.
> 
> I think for the consumers we have (IIRC, the proxy port fields in the
> advanced network dialog and one or two items in the print preview toolbar),
> we can either add that behaviour in custom JS or just omit it and validate
> that the field is valid when accepting the dialog / trying to print (and
> ensuring styling / form field warnings etc. work correctly).

In fact, it seems input[type=number] behaves this way already in Blink/Chromium, though it works "our" way in Edge. From the spec ( https://html.spec.whatwg.org/multipage/input.html#number-state-(type=number) ) it seems like what we do is correct:

--

If the element is mutable, the user agent should allow the user to change the number represented by its value, as obtained from applying the rules for parsing floating-point number values to it. User agents must not allow the user to set the **value** to a non-empty string that is not a valid floating-point number.

--

The link for **value** offers https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#concept-fe-value:

--

A control's value is its internal state. As such, it might not match the user's current input.

For instance, if a user enters the word "three" into a numeric field that expects digits, the user's input would be the string "three" but the control's value would remain unchanged. Or, if a user enters the email address "  [e-mail]" (with leading whitespace) into an email field, the user's input would be the string "  [e-mail]" but the browser's UI for email fields might translate that into a value of "[e-mail]" (without the leading whitespace).

--

I don't know if there are ongoing discussions about this aspect of rendering input type=number or if aligning with Blink would be an option, or if there are significant reasons that our implementation behaves the way it does. Jonathan, do you know?

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to :Gijs (he/him) from comment #3)
> I don't know if there are ongoing discussions about this aspect of rendering
> input type=number or if aligning with Blink would be an option, or if there
> are significant reasons that our implementation behaves the way it does.
> Jonathan, do you know?

That's covered by bug 1398528. We could potentially do that, but we'd need to consider what unintended consequences there may be.

Comment 5

[Mats Palmgren (inactive)]

I don't think this bug needs to wait for bug 1398528, since it's clear
the HTML control doesn't accept invalid values other than temporarily
before the value is validated.

Comment 6

[Brian Grinstead [:bgrins]]

If we want to maintain the current behavior, perhaps we could make a customized built-in HTML Custom Element (https://html.spec.whatwg.org/multipage/custom-elements.html#customized-built-in-element), that was in charge of hooking up necessary event listeners.

Comment 7

[Tim Nguyen :ntim]

Here's a minimal customized built-in HTML custom element that emulates the <textbox type="number"> behaviour: https://jsfiddle.net/ntim/tpkme4zc/

```
class MozNumberInput extends HTMLInputElement {
  constructor() {
    super();
    this.setAttribute("type", "number");

    this.addEventListener("input", (event) => {
      this._validateValue(this.value);
    });

    const value = this.value || 0;
    this._validateValue(value);
  }

  _validateValue(value) {
    const {min, max} = this;
    if (min && value < min) {
      value = min;
    } else if (max && value > max) {
      value = max;
    }

    this.value = value;
  }
}

customElements.define("moz-number", MozNumberInput, { extends: "input" });
```

Comment 8

[Tim Nguyen :ntim]

Attachment: Bug 1437641 - Convert numberbox binding to customized input element. r=bgrins,dao

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(keeping needinfo, will investigate)

So I was told the current patch posted have failed to generate custom element from the markups.

What I can do I think is to look at how the parser parses the markup and how it prevented us from picking up the value of the is attribute. Either with a debugger or the old faithful printf.

This [1] in fact already look pretty suspicious:

https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/dom/xml/nsXMLContentSink.cpp#436

If so I guess my patch from bug 1462806 is guilty of the problem.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

See also https://searchfox.org/mozilla-central/source/dom/tests/mochitest/webcomponents/test_custom_element_namespace.xul from bug 1480465 which didn't test for built-in customized HTML elements.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've got a patch and will address it in bug 1512696.

Comment 12

[Tim Nguyen :ntim]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)
> I've got a patch and will address it in bug 1512696.

Thanks Tim!

Comment 13

[Tim Nguyen :ntim]

Note that this bug is ready for review.

Comment 14

[Tim Nguyen :ntim]

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

Comment 15

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> If we want to maintain the current behavior,

I don't think we need to... deferring to bug 1398528 and dropping the current XUL behavior until then seems fine to me.

Comment 16

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > If we want to maintain the current behavior,
> 
> I don't think we need to... deferring to bug 1398528 and dropping the
> current XUL behavior until then seems fine to me.

I'd like to land something without UX changes first to avoid having to back out this enormous patch.

The change from <input is="moz-number"> to <input type="number"/> is very trivial to do in a follow up if UX is OK with it.

Comment 17

[Dão Gottwald [:dao]]

We can ask UX before moving forward here, but FWIW, if it's good enough for the entire web it should be good enough for our few uses cases, and otherwise we should just prioritize bug 1398528.

Comment 18

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #17)
> We can ask UX before moving forward here, but FWIW, if it's good enough for
> the entire web it should be good enough for our few uses cases, and
> otherwise we should just prioritize bug 1398528.


Here's a list of behaviours associated to the current numberbox binding that are not in input[type=number]:
1) It doesn't allow entering characters that are not numeric
2) Always validates the initial value and corrects it and sets the initial value to 0 if not present
3) It hides the spinbuttons (none of the current usages have hidespinbuttons="false", so it should be ok to make this default)

2 requires having extra JS anyway, so I don't think it's a bad idea to land this.

This is a definite code improvement over the old XBL binding, so it would be unfortunate to have this wait on UX or/and platform work (bug 1398528). And as I said, this bug makes it easier to move to input[type=number] later if we want to.

Comment 19

[Tim Nguyen :ntim]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5120dac9d3a3e84b5f9e2cc9f09158e2fa0d027

Comment 20

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #18)
> Here's a list of behaviours associated to the current numberbox binding that
> are not in input[type=number]:
> 1) It doesn't allow entering characters that are not numeric
> 2) Always validates the initial value and corrects it and sets the initial
> value to 0 if not present
> 3) It hides the spinbuttons (none of the current usages have
> hidespinbuttons="false", so it should be ok to make this default)
> 
> 2 requires having extra JS anyway, so I don't think it's a bad idea to land
> this.

I don't think we need 2). The initial value usually comes from a pref that is guaranteed to be a number, right?

> This is a definite code improvement over the old XBL binding, so it would be
> unfortunate to have this wait on UX or/and platform work (bug 1398528).

I wasn't suggesting that we should wait for bug 1398528, but rather that <input type="number"/> is likely good enough here, for now, if it's good enough for the web, and if we care enough we should look into addressing bug 1398528 independently.

Comment 21

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #20)
> > This is a definite code improvement over the old XBL binding, so it would be
> > unfortunate to have this wait on UX or/and platform work (bug 1398528).
> 
> I wasn't suggesting that we should wait for bug 1398528, but rather that
> <input type="number"/> is likely good enough here

Can we move that to a new bug ? 

I'm not opposed to using <input type="number"> but I feel like this bug already has enough scope: removal of the old binding/tests, stop using the XUL textbox binding as base binding (which has a bunch of custom behaviour associated to it), making hidespinbuttons="true" the default behaviour (since none use hidespinbuttons="false"), tweaking the existing CSS for <input type="number"/>.

I'd like to make sure those changes stick before actually dealing with any regressions related to removing the custom behaviour. There might be some unknown behaviour here, eg. what happens if the user enters letters inside the input, how does that affect the pref, etc. I don't have time to figure those things out, but in any case, having this bug landed as it is makes it easier later on to switch to a plain <input type=number>.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #21)
> (In reply to Dão Gottwald [::dao] from comment #20)
> > > This is a definite code improvement over the old XBL binding, so it would be
> > > unfortunate to have this wait on UX or/and platform work (bug 1398528).
> > 
> > I wasn't suggesting that we should wait for bug 1398528, but rather that
> > <input type="number"/> is likely good enough here
> 
> Can we move that to a new bug ? 
> 
> I'm not opposed to using <input type="number"> but I feel like this bug
> already has enough scope: removal of the old binding/tests, stop using the
> XUL textbox binding as base binding (which has a bunch of custom behaviour
> associated to it), making hidespinbuttons="true" the default behaviour
> (since none use hidespinbuttons="false"), tweaking the existing CSS for
> <input type="number"/>.
> 
> I'd like to make sure those changes stick before actually dealing with any
> regressions related to removing the custom behaviour. There might be some
> unknown behaviour here, eg. what happens if the user enters letters inside
> the input, how does that affect the pref, etc. I don't have time to figure
> those things out, but in any case, having this bug landed as it is makes it
> easier later on to switch to a plain <input type=number>.

I don't think there are many unknowns here. AFAIK the input's value property would be an empty when entering something non-numeric. Either way, even if it passed on the invalid string, that would translate to 0 in the pref. This seems okay for the connection port prefs and is irrelevant for the print preview. Do we have other users?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #22)
> AFAIK the input's value property
> would be an empty when entering something non-numeric.

*empty string

Comment 24

[Brian Grinstead [:bgrins]]

Doing this in two steps as Tim suggests in Comment 21 makes sense to me. When it's possible and doesn't add extra complexity, it's easier to land a behavior-compatible de-XBL bug separately. This makes it easy to track regressions: it shouldn't change behavior, so any regression will be easy to identify and track. Then in the [type=number] bug, there will be behavioral changes that we may or may not be OK with, but can be decided and tracked separately.

Comment 25

[Dão Gottwald [:dao]]

That makes sense in principle, in practice we have only a handful of these inputs (most of them in a single dialog that most users won't ever see), so presenting this as if there was a significant amount of risks and uncertainties here doesn't make sense. I appreciate Tim's efforts, but I think this is just overengineered; I don't see a scenario where we'd make use of this.

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #21)
> making hidespinbuttons="true" the default behaviour
> (since none use hidespinbuttons="false")

I also think you should drop this. The port inputs hide the spin buttons because users usually enter specific numbers there rather than increasing or decreasing values, and the print preview window hides them because it has its own buttons next to the input. The next number input we add may very well want to not hide these buttons.

Comment 26

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #25)
> That makes sense in principle, in practice we have only a handful of these
> inputs (most of them in a single dialog that most users won't ever see), so
> presenting this as if there was a significant amount of risks and
> uncertainties here doesn't make sense.

The fact that those dialogs are fairly well hidden doesn't really help here. Most regressions in bug 1429573 were noticed by Thunderbird engineers, due to their prominent use there. They've now ported the old numberbox binding due to all of those regressions, so we won't have the reports from TB anymore.

> I think this is just overengineered; I don't see a scenario where we'd make
> use of this.

That custom element class is ~30 lines of code, so it can be killed off easily if wanted. Plus the benefit of it being a customized built-in element is that we can use <input> directly rather than a custom tag name, making it easier to convert to input[type=number].

I'm again open to using input[type=number], but I think the assumption that if input[type=number] is good enough for the web, then it should be good enough for XUL document is a bit optimistic. The regressions that arised from bug 1429573 are just a proof of that.

Maybe the change to input[type=number] is risk-free, but if it isn't, I won't have time to fix the fallout from it.

> (In reply to Tim Nguyen :ntim (please use needinfo?) from comment #21)
> > making hidespinbuttons="true" the default behaviour
> > (since none use hidespinbuttons="false")
> 
> I also think you should drop this. The port inputs hide the spin buttons
> because users usually enter specific numbers there rather than increasing or
> decreasing values, and the print preview window hides them because it has
> its own buttons next to the input. The next number input we add may very
> well want to not hide these buttons.

The spinbuttons are pretty much broken since bug 1437302 (due to weird XUL/HTML flexbox interaction), this is the major reason I've completely removed support for them and simply hide them. See bug 1453371.

Comment 27

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #26)
> (In reply to Dão Gottwald [::dao] from comment #25)
> > That makes sense in principle, in practice we have only a handful of these
> > inputs (most of them in a single dialog that most users won't ever see), so
> > presenting this as if there was a significant amount of risks and
> > uncertainties here doesn't make sense.
> 
> The fact that those dialogs are fairly well hidden doesn't really help here.
> Most regressions in bug 1429573 were noticed by Thunderbird engineers, due
> to their prominent use there. They've now ported the old numberbox binding
> due to all of those regressions, so we won't have the reports from TB
> anymore.

I'm not sure what exactly you're suggesting. If we won't discover esoteric bugs due to low exposure, why worry about them? It's totally fine for Thunderbird to have different needs. We don't need to meet those, especially if Thunderbird already moved to a custom implementation.

> > I think this is just overengineered; I don't see a scenario where we'd make
> > use of this.
> 
> That custom element class is ~30 lines of code, so it can be killed off
> easily if wanted.

I think we have all the data we need to decide this now.

Comment 28

[Brian Grinstead [:bgrins]]

I still prefer landing this in two pieces, although I'm mostly keen to get _something_ landed to make progress and since this can provide a blueprint for migrating the other textbox bindings.

Dão, are you suggesting this patch could be modified to go directly to [type=number] without additional testing or UX input? If so, that'd be fine with me. But if not, it sounds like Tim doesn't have the cycles to do that step right now given Comment 26. And what he has is strictly an improvement over the status quo, and I'm not seeing any harm in spinning that work into another bug.

Comment 29

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> Dão, are you suggesting this patch could be modified to go directly to
> [type=number] without additional testing or UX input? If so, that'd be fine
> with me.

Yep. For our limited use cases, numberinput.js doesn't add significant UX value over plain <input type="number"/>.

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #29)
> (In reply to Brian Grinstead [:bgrins] from comment #28)
> > Dão, are you suggesting this patch could be modified to go directly to
> > [type=number] without additional testing or UX input? If so, that'd be fine
> > with me.
> 
> Yep. For our limited use cases, numberinput.js doesn't add significant UX
> value over plain <input type="number"/>.

Tim mentioned issues like expected behavior when entering letters into print preview inputs. I don't know what does / should happen in that case, but it would be easier to figure that out in a follow up (coordinating with UX as needed). It's not clear to me that we will see a benefit by doing this all at once.

Since the patch here solves the original request and leaves the widget in a much simplified state, I still prefer to split the behavioral change into another bug.

Comment 31

[Brian Grinstead [:bgrins]]

I talked with Tim, who's going to make a part 2 that drops the CE and migrates just to [type=number]. This could then either be attached to a follow up bug, or get folded together for landing here depending on what we decide to do.

Comment 32

[Tim Nguyen :ntim]

Attachment: Bug 1437641 - Change customized input element to raw input[type=number]. r=bgrins

Depends on D13825

Comment 33

[Tim Nguyen :ntim]

Attachment: Bug 1437641 - Remove numberbox binding and convert usages to input[type=number]. r=bgrins,dao

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00d33e7058c1
Remove numberbox binding and convert usages to input[type=number]. r=bgrins,dao

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e26702537a3b
Stop packaging arrow-{dn,up}.gif on Linux to fix browser_all_files_referenced.js failures. r=bustage-fix

Comment 36

[Tim Nguyen :ntim]

Hi Richard, this bug does 2 changes that might potentially break TB:

• Gets rid of the common.css styling for numberbox-input spin buttons.
• arrow-dn.gif and arrow-up.gif will no longer be packaged on Linux.

Comment 37

[Richard Marti (:Paenglab)]

Thank you Tim for the heads up. I'll do the needed changes in TB.

Comment 38

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/00d33e7058c1
https://hg.mozilla.org/mozilla-central/rev/e26702537a3b