843725 - Add support for the up/down/left/right/pgup/pgdn/home/end keys to <input type=range>

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Users need to be able to change the value of <input type=range> using the up/down/left/right/pgup/pgdn/home/end keys.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

The forms.css change is necessary because otherwise we end up with a binding calling preventDefault() on the 'press' events for up/down/left/right and never reach the point in the code where I've added handling for those events.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 717139 [details] [diff] [review]
patch

Review of attachment 717139 [details] [diff] [review]:
-----------------------------------------------------------------

What about RTL? I guess home goes on the right (should be checked maybe) so it should just work but maybe letf/right behaviour should be inversed?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2864,5 @@
> +               keyEvent->keyCode == NS_VK_DOWN ||
> +               keyEvent->keyCode == NS_VK_PAGE_UP ||
> +               keyEvent->keyCode == NS_VK_PAGE_DOWN ||
> +               keyEvent->keyCode == NS_VK_HOME ||
> +               keyEvent->keyCode == NS_VK_END)) {

Should we ignore those events if there is a modifier?

@@ +2873,5 @@
> +            if (minimum < maximum) {
> +              double value = GetValueAsDouble();
> +              double step = GetStep();
> +              if (step == kStepAny) {
> +                step = 1.0;

step = GetDefaultStep();

But actually, you could ust remove all that I believe...

@@ +2880,5 @@
> +                         MOZ_DOUBLE_IS_FINITE(step));
> +              double valueToSet;
> +              switch (keyEvent->keyCode) {
> +                case  NS_VK_HOME:
> +                  valueToSet = minimum;

SetValue(GetMinimum());

@@ +2883,5 @@
> +                case  NS_VK_HOME:
> +                  valueToSet = minimum;
> +                  break;
> +                case  NS_VK_END:
> +                  valueToSet = maximum;

SetValue(GetMaximum());

@@ +2885,5 @@
> +                  break;
> +                case  NS_VK_END:
> +                  valueToSet = maximum;
> +                  break;
> +                case  NS_VK_UP:

I'm not sure how 'up' should behave... Did you try a few applications?

@@ +2887,5 @@
> +                  valueToSet = maximum;
> +                  break;
> +                case  NS_VK_UP:
> +                case  NS_VK_RIGHT:
> +                  valueToSet = value + step;

ApplyStep(1);

@@ +2889,5 @@
> +                case  NS_VK_UP:
> +                case  NS_VK_RIGHT:
> +                  valueToSet = value + step;
> +                  break;
> +                case  NS_VK_DOWN:

Ditto for 'down'.

@@ +2891,5 @@
> +                  valueToSet = value + step;
> +                  break;
> +                case  NS_VK_DOWN:
> +                case  NS_VK_LEFT:
> +                  valueToSet = value - step;

ApplyStep(-1);

@@ +2899,5 @@
> +                  // requires us to jump more.
> +                  valueToSet = value + std::max(step, 0.1 * (maximum - minimum));
> +                  break;
> +                case  NS_VK_PAGE_DOWN:
> +                  valueToSet = value - std::max(step, 0.1 * (maximum - minimum));

Merge PAGE_UP and PAGE_DOWN and get the value and do all the crazy stuff here.

@@ +2905,5 @@
> +              }
> +              MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(valueToSet));
> +              nsAutoString val;
> +              ConvertNumberToString(valueToSet, val);
> +              SetValueInternal(val, true, true);

You should be able to remove all of that. It's only needed for PAGE_UP and PAGE_DOWN.

@@ +2910,5 @@
> +              nsIFrame* frame = GetPrimaryFrame();
> +              if (frame) {
> +                // Trigger reflow to update the thumb's position:
> +                frame->AttributeChanged(kNameSpaceID_None,
> +                                        nsGkAtoms::value,

I'm not a big fan of that because the attribute hasn't actually changed...
Usually, form related frames inherit from nsIFormControlFrame and then we use 'SetFormProperty'. But nsRangeFrame doesn't inherit from nsIFormControlFrame...

It's interesting that there is no way for the content to say to its frame "you will have to reflow/draw yourself, I have changed".

::: layout/style/forms.css
@@ +725,5 @@
>    height: 1.3em;
>    background: none;
>    border: none;
>    margin: 0 0.7em;
> +  -moz-binding: none; // we don't want any of platformHTMLBindings.xml#inputFields

I think there are a few properties that you are setting to override what "input" rule do. Could you group them like:
input[type='range'] {
  // Override some rules that apply on all input types.
  cursor: default;
  background:none;
  border: none;
  -moz-binding: none;
  [...]

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> What about RTL? I guess home goes on the right (should be checked maybe) so
> it should just work but maybe letf/right behaviour should be inversed?

Home should work. Left and right should maybe be inverted. I'll investigate.

> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +2864,5 @@
> > +               keyEvent->keyCode == NS_VK_DOWN ||
> > +               keyEvent->keyCode == NS_VK_PAGE_UP ||
> > +               keyEvent->keyCode == NS_VK_PAGE_DOWN ||
> > +               keyEvent->keyCode == NS_VK_HOME ||
> > +               keyEvent->keyCode == NS_VK_END)) {
> 
> Should we ignore those events if there is a modifier?

No, I don't think so. If the modifier has OS behavior, we'll never see it. If the <input> is focused then we should handle it I think and prevent anything further up the DOM tree from handling it.

For what it's worth Chrome does not pay attention to modifiers.

> @@ +2873,5 @@
> > +            if (minimum < maximum) {
> > +              double value = GetValueAsDouble();
> > +              double step = GetStep();
> > +              if (step == kStepAny) {
> > +                step = 1.0;
> 
> step = GetDefaultStep();
> 
> But actually, you could ust remove all that I believe...

GetDefaultStep? I guess we could, but I'd rather use kDefaultStep since we know were handling for type=range here.

> @@ +2880,5 @@
> > +                         MOZ_DOUBLE_IS_FINITE(step));
> > +              double valueToSet;
> > +              switch (keyEvent->keyCode) {
> > +                case  NS_VK_HOME:
> > +                  valueToSet = minimum;
> 
> SetValue(GetMinimum());

SetValue does not do the correct thing, since it does not pass |true, true|.

> I'm not sure how 'up' should behave... Did you try a few applications?

Yes, it seems to consistently increase the value, which is what I have.

> @@ +2905,5 @@
> > +              }
> > +              MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(valueToSet));
> > +              nsAutoString val;
> > +              ConvertNumberToString(valueToSet, val);
> > +              SetValueInternal(val, true, true);
> 
> You should be able to remove all of that. It's only needed for PAGE_UP and
> PAGE_DOWN.

SetValue() doesn't do what we want, as noted above, so it's also needed for UP and DOWN.

> @@ +2910,5 @@
> > +              nsIFrame* frame = GetPrimaryFrame();
> > +              if (frame) {
> > +                // Trigger reflow to update the thumb's position:
> > +                frame->AttributeChanged(kNameSpaceID_None,
> > +                                        nsGkAtoms::value,
> 
> I'm not a big fan of that because the attribute hasn't actually changed...
> Usually, form related frames inherit from nsIFormControlFrame and then we
> use 'SetFormProperty'. But nsRangeFrame doesn't inherit from
> nsIFormControlFrame...
> 
> It's interesting that there is no way for the content to say to its frame
> "you will have to reflow/draw yourself, I have changed".

It's pretty common in other parts of the code to use AttributeChanged in this way.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to Mounir Lamouri (:mounir) from comment #5)
> > SetValue(GetMinimum());
> 
> SetValue does not do the correct thing, since it does not pass |true, true|.

nsAutoString minimum;
ConvertNumberToString(GetMinimum(), minimum); // you can assert that the return value isn't false.
SetValueInternal(minimum, false, false);

> > @@ +2910,5 @@
> > > +              nsIFrame* frame = GetPrimaryFrame();
> > > +              if (frame) {
> > > +                // Trigger reflow to update the thumb's position:
> > > +                frame->AttributeChanged(kNameSpaceID_None,
> > > +                                        nsGkAtoms::value,
> > 
> > I'm not a big fan of that because the attribute hasn't actually changed...
> > Usually, form related frames inherit from nsIFormControlFrame and then we
> > use 'SetFormProperty'. But nsRangeFrame doesn't inherit from
> > nsIFormControlFrame...
> > 
> > It's interesting that there is no way for the content to say to its frame
> > "you will have to reflow/draw yourself, I have changed".
> 
> It's pretty common in other parts of the code to use AttributeChanged in
> this way.

Oh... :(

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 717139 [details] [diff] [review]
patch

># HG changeset patch
># Parent d8d83c379e4881a3cbe3cdfdd76355fa6ac2ce9c
># User Jonathan Watt <[e-mail]>
>Bug 843725 - Add support for changing the value of <input type=range> using the up/down/left/right/pgup/pgdn/home/end keys. r=mounir.
>
>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -2851,16 +2851,77 @@ nsHTMLInputElement::PostHandleEvent(nsEv
>                keyEvent->keyCode == NS_VK_ENTER) &&
>                (IsSingleLineTextControl(false, mType) ||
>                 IsExperimentalMobileType(mType))) {
>             FireChangeEventIfNeeded();
>             rv = MaybeSubmitForm(aVisitor.mPresContext);
>             NS_ENSURE_SUCCESS(rv, rv);
>           }
> 
>+          if (aVisitor.mEvent->message == NS_KEY_PRESS &&
>+              mType == NS_FORM_INPUT_RANGE &&
>+              (keyEvent->keyCode == NS_VK_LEFT ||
>+               keyEvent->keyCode == NS_VK_RIGHT ||
>+               keyEvent->keyCode == NS_VK_UP ||
>+               keyEvent->keyCode == NS_VK_DOWN ||
>+               keyEvent->keyCode == NS_VK_PAGE_UP ||
>+               keyEvent->keyCode == NS_VK_PAGE_DOWN ||
>+               keyEvent->keyCode == NS_VK_HOME ||
>+               keyEvent->keyCode == NS_VK_END)) {
I think we should not handle these events in case there are some modifiers.


>+              nsIFrame* frame = GetPrimaryFrame();
>+              if (frame) {
>+                // Trigger reflow to update the thumb's position:
>+                frame->AttributeChanged(kNameSpaceID_None,
>+                                        nsGkAtoms::value,
>+                                        nsIDOMMutationEvent::MODIFICATION);
This is really odd. Better to call FrameNeedsReflow or some such?

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch after comments and IRC discussion

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 718402 [details] [diff] [review]
patch after comments and IRC discussion

Review of attachment 718402 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments applied but I would like to have Olli's eyes on the patch before landing.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2577,5 @@
> +    return frame->StyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;
> +  }
> +  // at least for HTML, directionality is exclusively LTR or RTL
> +  return aElement->GetDirectionality() == eDir_LTR;
> +}

I would prefer to have this living in Element. Could you open a follow-up?

@@ +2860,5 @@
> +            double minimum = GetMinimum();
> +            double maximum = GetMaximum();
> +            MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(minimum) &&
> +                       MOZ_DOUBLE_IS_FINITE(maximum));
> +            if (minimum < maximum) { // else the value is locked to the minimum

I wish bug 841946 were fixed.

@@ +2871,5 @@
> +                         MOZ_DOUBLE_IS_FINITE(step));
> +              double newValue;
> +              switch (keyEvent->keyCode) {
> +                case  NS_VK_LEFT:
> +                  newValue = value + (IsLTR(this) ? -step : step);

nit: I guess you can remove the parenthesis.

@@ +2874,5 @@
> +                case  NS_VK_LEFT:
> +                  newValue = value + (IsLTR(this) ? -step : step);
> +                  break;
> +                case  NS_VK_RIGHT:
> +                  newValue = value + (IsLTR(this) ? step : -step);

nit: ditto

::: content/html/content/test/forms/Makefile.in
@@ +17,5 @@
>  		test_mozistextfield.html \
>  		test_input_attributes_reflection.html \
>  		test_input_list_attribute.html \
>  		test_input_email.html \
> +		test_input_range_key_events.html \

nit: fwiw, this list doesn't pretend to be alphabetically ordered. There is no problem in trying to make it so, just telling ;)

::: content/html/content/test/forms/test_input_range_key_events.html
@@ +17,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test for Bug 843725 **/

Explain in a line or two what the test does here.

@@ +38,5 @@
> +}
> +
> +function maximum(element) {
> +  var maximum = Number(element.max || defaultMaximum);
> +  return Math.max(minimum(element), maximum);

maximum = minimum? Did you do that because of defaultValue()? Don't think it makes things very clear...

@@ +112,5 @@
> +  ["VK_DOWN",      RTL, valueMinusStep],
> +  ["VK_PAGE_UP",   LTR, valuePlusTenPctOrStep],
> +  ["VK_PAGE_UP",   RTL, valuePlusTenPctOrStep],
> +  ["VK_PAGE_DOWN", LTR, valueMinusTenPctOrStep],
> +  ["VK_PAGE_DOWN", RTL, valueMinusTenPctOrStep],

Could you add a test in your main loop with step=20 for example so you can check that the value actually goes higher?

@@ +127,5 @@
> +
> +  var content = document.getElementById("content");
> +  content.appendChild(elem);
> +  elem.focus();
> +  

nit: trailing whitespaces

::: layout/style/forms.css
@@ +723,4 @@
>    width: 12em;
>    height: 1.3em;
> +  margin: 0 0.7em;
> +  // Override some rules that apply on all input types:

"//" isn't a valid comment in CSS.

@@ +728,3 @@
>    background: none;
>    border: none;
> +  -moz-binding: none; // we don't want any of platformHTMLBindings.xml#inputFields

ditto

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> I would prefer to have this living in Element. Could you open a follow-up?

Bug 846688.

> @@ +2860,5 @@
> > +            double minimum = GetMinimum();
> > +            double maximum = GetMaximum();
> > +            MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(minimum) &&
> > +                       MOZ_DOUBLE_IS_FINITE(maximum));
> > +            if (minimum < maximum) { // else the value is locked to the minimum
> 
> I wish bug 841946 were fixed.

That would make no difference, since whether we are in an error state or not, we still have to handle the case where |minimum >= maximum|.

> @@ +2871,5 @@
> > +                         MOZ_DOUBLE_IS_FINITE(step));
> > +              double newValue;
> > +              switch (keyEvent->keyCode) {
> > +                case  NS_VK_LEFT:
> > +                  newValue = value + (IsLTR(this) ? -step : step);
> 
> nit: I guess you can remove the parenthesis.

I think the code is easier to read with them. I'll remove them if you really, really want though (let me know). :)

> nit: fwiw, this list doesn't pretend to be alphabetically ordered. There is
> no problem in trying to make it so, just telling ;)

Yeah, I know. I thought I might as well move it in that direction though.

> Explain in a line or two what the test does here.
> 
> @@ +38,5 @@
> > +}
> > +
> > +function maximum(element) {
> > +  var maximum = Number(element.max || defaultMaximum);
> > +  return Math.max(minimum(element), maximum);
> 
> maximum = minimum? Did you do that because of defaultValue()? Don't think it
> makes things very clear...

I did that to simplify the places where we check the maximim, since otherwise they also need to have code along the lines of |if (maximum < minimum) { /* handle bad case */ }|. That isn't actually all that bad I guess, so I've changed the test to have those checks.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch [r=mounir]

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 719895 [details] [diff] [review]
patch [r=mounir]

Some rtl user should review this. smontagu or ehsan perhaps, since I don't
know what is expected in rtl UI.

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #13)
> Some rtl user should review this. smontagu or ehsan perhaps, since I don't
> know what is expected in rtl UI.

I discussed the behavior with smontagu over email, but sure, I can ask him for review too. I've gone ahead and pushed anyway so I can get eyes on it asap, and it's probably good for smontagu to be able to test the behavior rather than just have to imply it from the code.

Comment 15

[(no longer active)]

So we do visual navigation here as opposed to logical, right?

Comment 16

[Jonathan Watt [:jwatt]]

In a RTL context, the value of the range increases as the slider moves to the left. As a result the left arrow key is mapped to increase the value of the range, not decrease it as it will in a LTR context.

For vertical slider in an LTR context, the right and up arrows will increase its value. In a RTL context the left and up arrows will increase its value.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9853045b46d4

https://tbpl.mozilla.org/php/getParsedLog.php?id=20224120&tree=Mozilla-Inbound

08:24:43     INFO -  168401 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking form15 input
08:24:43     INFO -  168402 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Starting test #407
08:24:43     INFO -  168403 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu
08:24:43     INFO -  168404 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking form16 input - got 30, expected 32

Comment 18

[(no longer active)]

(In reply to comment #16)
> In a RTL context, the value of the range increases as the slider moves to the
> left. As a result the left arrow key is mapped to increase the value of the
> range, not decrease it as it will in a LTR context.
> 
> For vertical slider in an LTR context, the right and up arrows will increase
> its value. In a RTL context the left and up arrows will increase its value.

That sounds good.

Comment 19

[Jonathan Watt [:jwatt]]

Re-pushed with trivial fix to test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae1781d5e18

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6ae1781d5e18

Comment 21

[Simon Montagu :smontagu]

Comment on attachment 719895 [details] [diff] [review]
patch [r=mounir]

Review of attachment 719895 [details] [diff] [review]:
-----------------------------------------------------------------

The RTL behaviour looks right to me (based on testing, without looking at the code)