Closed Bug 1071994 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

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
Attached patch patchSplinter Review
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 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.
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)
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).
Flags: needinfo?(jwatt)
(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 on attachment 8494100 [details] [diff] [review]
patch

Need some detailed explanation here, sorry.
Attachment #8494100 - Flags: review?(bugs)
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)
Blocks: 1089841
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1118348
Blocks: 1780302
You need to log in before you can comment on or make changes to this bug.