Update our implementation of HTMLInputElement's stepping algorithm to the current HTML5 text

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

While trying to review bug 1028712 I just tied myself in knots trying to reconcile the desired behavior there into our current code. It raised way more difficult questions in my head that demanded to be answered than it answered in itself. Eventually I just came to the conclusion that trying to press ahead with fixing that bug with the code as it currently is is a fools errand, and that we should update our current code to more closely align with the current spec text at:

https://html.spec.whatwg.org/multipage/forms.html#dom-input-stepup
Created attachment 8494100 [details] [diff] [review]
patch
Attachment #8494100 - Flags: review?(bugs)
FWIW this passes Try, including the new tests added in bug 1028712 that his bug fixes:

https://tbpl.mozilla.org/?tree=Try&rev=93ebbaa0acc7

Comment 3

3 years ago
Comment on attachment 8494100 [details] [diff] [review]
patch

>   if (!maximum.isNaN()) {
>     // "max - (max - stepBase) % step" is the nearest valid value to max.
>-    maximum = maximum - NS_floorModulo(maximum - GetStepBase(), step);
>+    maximum = maximum - NS_floorModulo(maximum - stepBase, step);
>+    if (!minimum.isNaN()) {
>+      if (minimum > maximum) {
>+        // Either the minimum was greater than the maximum prior to our
>+        // adjustment to align maximum on a step, or else (if we adjusted
>+        // maximum) there is no valid step between minimum and the unadjusted
>+        // maximum.
Tiny bit hard to follow when you combine two steps from the spec to one.
I'd prefer if you didn't do that.

>+  if (!valueWasNaN && // value="", resulting in us using "0"
>+      ((aStep > 0 && value < valueBeforeSteping) ||
>+       (aStep < 0 && value > valueBeforeSteping))) {
Isn't it valueBeforeStepping

But I don't see this in the spec. Please explain.

Updated

3 years ago
Flags: needinfo?(jwatt)
(In reply to Olli Pettay [:smaug] from comment #3)
> But I don't see this in the spec. Please explain.

Trying to remember...following the spec there's basically situations that can causes a step up request to step down, and visa versa. One example is if we have something like this:

  <input type=number max=-3>

the value of the input is "", which defaults to zero for stepping, and with the spec rules step up would set the value to 1, I seem to recall. So if the user clicks the up arrow we'd go into overflow. We could set the value to the maximum, but that may result in a step mismatch. Some of agi's tests in bug 1028712 test for this, and the following tests in content/html/content/test/forms/test_stepup_stepdown.html fail without it:

  The value for type=number should be 1 - got 10, expected 1
  The value for type=number should be 1 - got 2, expected 1
  The value for type=date should be 2012-01-01 - got 2012-01-10, expected 2012-01-01
  The value for type=date should be 1970-01-01 - got 1970-01-02, expected 1970-01-01
  The value for type=time should be 17:10 - got 17:30, expected 17:10
  The value for type=number should be 1 - got 0, expected 1
  The value for type=number should be 1 - got -10, expected 1
  The value for type=number should be -3 - got -5, expected -3
  The value for type=date should be 2012-01-02 - got 2012-01-01, expected 2012-01-02
  The value for type=date should be 1969-01-02 - got 1969-01-01, expected 1969-01-02
  The value for type=time should be 17:15 - got 17:13, expected 17:15
  The value for type=time should be 17:15 - got 17:13, expected 17:15
  The value for type=time should be 17:22 - got 17:11, expected 17:22
  The value for type=time should be 17:22 - got 17:17, expected 17:22
  The value for type=time should be 17:22 - got 17:18, expected 17:22
  The value for type=time should be 17:22 - got 17:18, expected 17:22
  The value for type=time should be 17:22 - got 17:19, expected 17:22
Flags: needinfo?(jwatt)

Comment 5

3 years ago
But why do we need to add any steps to the algorithm defined in the spec to pass the tests?
Either the tests are buggy or the spec is buggy (and we should implement some different algorithm).

Updated

3 years ago
Flags: needinfo?(jwatt)

Comment 6

3 years ago
(In reply to Jonathan Watt [:jwatt] (away until Tuesday 14th) from comment #4)
> (In reply to Olli Pettay [:smaug] from comment #3)
> > But I don't see this in the spec. Please explain.
> 
> Trying to remember...following the spec there's basically situations that
> can causes a step up request to step down, and visa versa.

Yes, that is what the spec seems to require.
"If the element has a maximum, and value is greater than that maximum, then set value to the largest value that, when subtracted from the step base, is an integral multiple of the allowed value step, and that is less than or equal to maximum."

Comment 7

3 years ago
Comment on attachment 8494100 [details] [diff] [review]
patch

Need some detailed explanation here, sorry.
Attachment #8494100 - Flags: review?(bugs)

Comment 8

3 years ago
ni ping
The failing tests are all pre-existing tests:

https://mxr.mozilla.org/mozilla-central/source/dom/html/test/forms/test_stepup_stepdown.html?force=1&rev=8ccc36e1d05c&mark=137-138,284-284,303-303,348-348,463-466,602-602,605-650,670-671,679-679,681-684

These tests are explicitly testing that a current invalid value is left unchanged when a stepUp would otherwise move the value above the maximum, or a stepDown would move it below the minimum.
Flags: needinfo?(jwatt)
Comment on attachment 8494100 [details] [diff] [review]
patch

I still think we should take this patch and not change that current aspect of our behavior. We can then in a follow-up bring up on the list the question of whether the spec text intends to have the result that it does in these cases, and from there decide whether we're going to change that aspect or have the spec changed. Personally I think the spec should change.
Attachment #8494100 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Blocks: 1089841
(Assignee)

Updated

3 years ago
Blocks: 1101007
Could you perhaps file a spec bug?
Comment on attachment 8494100 [details] [diff] [review]
patch

>   if (!maximum.isNaN()) {
>     // "max - (max - stepBase) % step" is the nearest valid value to max.
>-    maximum = maximum - NS_floorModulo(maximum - GetStepBase(), step);
>+    maximum = maximum - NS_floorModulo(maximum - stepBase, step);
>+    if (!minimum.isNaN()) {
>+      if (minimum > maximum) {
>+        // Either the minimum was greater than the maximum prior to our
>+        // adjustment to align maximum on a step, or else (if we adjusted
>+        // maximum) there is no valid step between minimum and the unadjusted
>+        // maximum.
>+        return NS_OK;
>+      }
This is still a bit hard to read, since it is combining steps 3 and 4.


>+  if (!valueWasNaN && // value="", resulting in us using "0"
>+      ((aStep > 0 && value < valueBeforeSteping) ||
>+       (aStep < 0 && value > valueBeforeSteping))) {
>+    // We don't want step-up to effectively step down, or step-down to
>+    // effectively step up, so return;
>+    return NS_OK;
>+  }

s/valueBeforeSteping/valueBeforeStepping/


Need some test.
Attachment #8494100 - Flags: review?(bugs) → review+
Comment on attachment 8494100 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf063d26159a

(In reply to Olli Pettay [:smaug] from comment #13)
> This is still a bit hard to read, since it is combining steps 3 and 4.

Unfortunately it's not clear to me how best to separate it, and I don't really have time to work it out. Landed as-is for now since this is still a huge improvement to clarity over the previous code.

> Need some test.

Landed the tests from bug 1028712 as discussed on IRC.
Attachment #8494100 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/bf063d26159a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Updated

3 years ago
Blocks: 1118348

Updated

3 years ago
Duplicate of this bug: 1118348
You need to log in before you can comment on or make changes to this bug.