Page MenuHomePhabricator

OO.ui.NumberInputWidget only accepts multiples of step value
Closed, ResolvedPublic

Description

OO.ui.NumberInputWidget only accept numbers as input, that are multiples of the step value (or actually the diff to the minimum must be a multiple of the step value). This is consistent with native number inputs (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number), but I think it is unexpected and probably unintentional:

  1. It's not documented.
  2. There is an allowInteger option that defaults to false, but as the default step is 1, only integers are allowed anyway.
  3. The example https://doc.wikimedia.org/oojs-ui/master/demos/?page=widgets&theme=wikimediaui&direction=ltr&platform=desktop#NumberInputWidget-0-1-step-by-1-page-by-25 has step 0.1, but pageStep 0.25. So using "Page up" once will make the input invalid. (Actually, due to rounding errors, even with the simple step you'll end up with an invalid value sooner or later, T102128: Decimal step on NumberInputWidget results in cumulative binary rounding error)
  4. The tooltip that tells me about the invalid input is in German, even though the rest of the page is English.

So I think this behaviour hasn't been the case from beginning. Either revert to make NumberInputWidgets accept any number no matter what the step is, or document the behaviour properly and make sure the default options and examples make sense.

Event Timeline

Schnark created this task.Sep 7 2018, 9:39 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 7 2018, 9:39 AM

It seems that before rGOJU31e24f02508b: Add an infusable PHP implementation of the NumberInputWidget we weren't setting the step attribute on the input, so there was no validation for it. We rely on the browser's validation here (that's why the error message is in German for you).

I think ideally we would use the same semantics as HTML. Let's see if we can get away with changing it. I guess what we need to do is to remove the default value for step (and make sure not setting it works correctly).

We should probably put something on the demo page about having incompatible step and pageStep values is silly and broken for users, too.

matmarex claimed this task.Sep 7 2018, 7:38 PM

Change 458884 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] NumberInputWidget: Rethink 'step' semantics

https://gerrit.wikimedia.org/r/458884

Volker_E moved this task from Backlog to Reviewing on the OOUI board.Sep 8 2018, 12:40 AM

Change 458884 merged by jenkins-bot:
[oojs/ui@master] NumberInputWidget: Rethink 'step' semantics

https://gerrit.wikimedia.org/r/458884

Volker_E moved this task from Reviewing to OOUI-0.28.2 on the OOUI board.Sep 11 2018, 8:31 PM
Volker_E edited projects, added OOUI (OOUI-0.28.2); removed OOUI.
Volker_E closed this task as Resolved.Sep 11 2018, 10:47 PM
Volker_E triaged this task as High priority.
Volker_E removed a project: Patch-For-Review.
Volker_E removed a subscriber: gerritbot.

Change 459879 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] Update OOUI to v0.28.2

https://gerrit.wikimedia.org/r/459879

Change 459879 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.28.2

https://gerrit.wikimedia.org/r/459879