Skip to content
This repository has been archived by the owner on Jul 30, 2019. It is now read-only.

stepUp/Down algorithm does not match reality #281

Closed
travisleithead opened this issue Apr 27, 2016 · 4 comments · Fixed by #392
Closed

stepUp/Down algorithm does not match reality #281

travisleithead opened this issue Apr 27, 2016 · 4 comments · Fixed by #392

Comments

@travisleithead
Copy link
Member

In my reading of http://w3c.github.io/html/sec-forms.html#dom-htmlinputelement-stepup

with the following input:

<input type="month" value="1970-03" min="1970-01" max="2000-01" step="0.5">

The algorithm seems to get all the way through to step 11, where it tries to convert value (3.5) to a month string. I think somewhere along the line it must be triggering the "suffering from a step-mismatch" because 3.5 does not convert to a legitimate month value, so rounding occurs.

I think step 7 is trying to do this, but as worded, the step base-minus-value (-2) is an integral multiple of the allowed value step (0.5) [using a integral multiple of -4].

The algorithm should probably do an explicit rounding, perhaps via a step 10 1/2 that does a ceil(value) if value suffers a step-mismatch and stepUp() was called, floor(value) in the stepdown() case. (Since small steps still result in an increment, e.g., step values of "0.2").

Test: https://jsfiddle.net/a3cafwv0/
Edge/Chrome: round-up to the next month.
Firefox/Safari: do not support type="month"

@travisleithead
Copy link
Member Author

step=2.6 scalar=2 => 6 month step --> https://jsfiddle.net/a3cafwv0/7/
step=2.1 scalar=2 => 6 month step --> https://jsfiddle.net/a3cafwv0/8/

So, Edge/Chrome both apply the ceil(step) then multiply by scalar for step up/down (to get 6 months either way).

@travisleithead
Copy link
Member Author

That last comment was not quite true, they do round the step value. Regardless, I believe I've now captured the logic in a PR. Please review.

@travisleithead
Copy link
Member Author

For those reviewing: The essence of the problem is that for both:
<input type="number" value="2" step="3.5">
<input type="month" value="2016-02" step="3.5">
The algorithms in the spec used the same units and same conversions for both of these--yet only the number input state can represent fractional values; months cannot. Therefore something needed to be done to adjust the algorithms to accommodate that.

My change adjusts the value ultimately used as the step value (before it is multiplied by the scalar value) so that for month, date, time, etc., the step snapping is done using whole integers which prevents it from landing on a fractional scale unit. This appears to match exactly what browsers are doing.

@travisleithead
Copy link
Member Author

Ran this by some internal folks at Microsoft who implemented Edge's new input controls (Scott & Co.) [info removed for privacy]:

I think that is “round to nearest + round half up” (or javascript’s Math.round()).

--scott

From: Travis Leithead
Sent: Friday, May 20, 2016 3:15 PM
To: Scott; Charles
Cc: Sermet; Brandon; Arron Eicholz
Subject: RE: Date/Time & Color Controls W3C Spec Questions

I think this should be the standard rounding (where you round up on 0.5), that seems to be what our code does in my testing (it’s not banker’s rounding or any other variant I can think of).

Is there a well-known term for the “standard rounding”?

What I said in the bug is wrong—it definitely needs to be standard rounding.

From: Scott
Sent: Friday, May 20, 2016 2:57 PM
To: Travis Leithead; Charles
Cc: Sermet; Brandon; Arron Eicholz
Subject: RE: Date/Time & Color Controls W3C Spec Questions

What type of rounding are you specifying?

  • In the spec update you say “round to nearest whole number” (but there are bunch of variants of this for 0.5). (You also mention “round up” in an example in the spec).
  • In the bug you say “round up”, which is something different than round to nearest, but simpler.

Otherwise looks fine to me.

--scott

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants