Changing slider values do not work properly when percent change of a step is less than one.

Changing slider values do not work properly when percent change of a step is less than one.

Upgrade incrementing and decrementing of slider values. Change a way of getting
a step size. We could call a createStepRange to get the current step if it's explicitly defined
if not, default step will be calculated instead of a five percent change.

BUG=475053
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193722

[k.czech, 2015-04-09 15:05:14]

k.czech@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2015-04-09 15:11:24]

https://codereview.chromium.org/1071873004/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/AXNodeObject.cpp (left):

https://codereview.chromium.org/1071873004/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXNodeObject.cpp:2085: value += range *
(percentChange / 100);
i'm not sure I quite understand why this code didn't work before. If the min was
0 and the max was 5, wouldn't this move by 10%, or 0.5, and give values of 0.5,
1.0, 1.5, etc.?

If these were being truncated to integers, where did that happen?

[k.czech, 2015-04-09 22:36:30]

On 2015/04/09 15:11:24, dmazzoni wrote:
>
https://codereview.chromium.org/1071873004/diff/1/Source/modules/accessibilit...
> File Source/modules/accessibility/AXNodeObject.cpp (left):
> 
>
https://codereview.chromium.org/1071873004/diff/1/Source/modules/accessibilit...
> Source/modules/accessibility/AXNodeObject.cpp:2085: value += range *
> (percentChange / 100);
> i'm not sure I quite understand why this code didn't work before. If the min
was
> 0 and the max was 5, wouldn't this move by 10%, or 0.5, and give values of
0.5,
> 1.0, 1.5, etc.?
The default percentChange is (5/-5) so in this case the step would be 0.25 and
move 5%. When step is rounded to 1, move would be 20%.
> 
> If these were being truncated to integers, where did that happen?

Exactly values are rounded to integers. It happens while value of slider is
refreshed.
Seems like the conversion rounds to 1 when value is <1, 1.5), otherwise it
rounds to 2.

[dmazzoni, 2015-04-10 06:34:22]

Aha! I figured out the issue - it's not that slider values are rounded to the
nearest integer, they're rounded to the nearest *step size*.

So if you have this slider:

<input type=range min=1 max=5>

...then you can't set it to 2.1, it will change back to 2.

But if you set the step size to 0.1, that's different:

<input type=range min=1 max=5 step=0.1>

Now you can set it to 2.1 or 3.7, etc. and it will work.

So your code is basically correct but instead of checking if the percentage
change is less than 1, check if it's less than the step size and always
increase/decrease by at least the step size.

[k.czech, 2015-04-10 13:28:06]

On 2015/04/10 06:34:22, dmazzoni wrote:
> Aha! I figured out the issue - it's not that slider values are rounded to the
> nearest integer, they're rounded to the nearest *step size*.
You are right.
> 
> So if you have this slider:
> 
> <input type=range min=1 max=5>
> 
> ...then you can't set it to 2.1, it will change back to 2.
> 
> But if you set the step size to 0.1, that's different:
> 
> <input type=range min=1 max=5 step=0.1>
> 
> Now you can set it to 2.1 or 3.7, etc. and it will work.
Seems that how it works where step is explicitly defined.
> 
> So your code is basically correct but instead of checking if the percentage
> change is less than 1, check if it's less than the step size and always
> increase/decrease by at least the step size.
In case where step is not defined, it will always be 0.
I assuming default step value is 1 and then increase/decrease by using this
value only when percentage change is less.

[dmazzoni, 2015-04-10 17:14:29]

This is better, but I think it'd be best if we didn't have to assume what
happens when "step" is empty, that seems like it's duplicating logic that's
elsewhere in the code.

https://codereview.chromium.org/1071873004/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1071873004/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:132: float step =
stepValueForRange();
It looks like stepValueForRange is implemented naively, it just returns the step
attribute.

I think better would be to get it from HTMLInputElement so that it follows the
spec correctly.

At first glance it looks like you should get it from
toHTMLInputElement(node())->createStepRange(RejectAny).step() (with appropriate
error checking, of course).

Yet another way to do it would be to try to set the value by the percentage, and
if it doesn't change, call HTMLInputElement::stepUp or stepDown.

[k.czech, 2015-04-13 11:57:47]

On 2015/04/10 17:14:29, dmazzoni wrote:
> This is better, but I think it'd be best if we didn't have to assume what
> happens when "step" is empty, that seems like it's duplicating logic that's
> elsewhere in the code.
I'm wondering, seems like createStepRange returns one when step is not
explicitly defined,
otherwise it returns actual value of a step. It implies that we do not really
need to have
this 5 percent default change, I guess ?. Like you said, probably logic would be
duplicated.
> 
>
https://codereview.chromium.org/1071873004/diff/20001/Source/modules/accessib...
> File Source/modules/accessibility/AXNodeObject.cpp (right):
> 
>
https://codereview.chromium.org/1071873004/diff/20001/Source/modules/accessib...
> Source/modules/accessibility/AXNodeObject.cpp:132: float step =
> stepValueForRange();
> It looks like stepValueForRange is implemented naively, it just returns the
step
> attribute.
> 
> I think better would be to get it from HTMLInputElement so that it follows the
> spec correctly.
It's true, thanks.
> 
> At first glance it looks like you should get it from
> toHTMLInputElement(node())->createStepRange(RejectAny).step() (with
appropriate
> error checking, of course).
Exactly, Let's the engine calculate this step by itself following the spec.
> 
> Yet another way to do it would be to try to set the value by the percentage,
and
> if it doesn't change, call HTMLInputElement::stepUp or stepDown.

[dmazzoni, 2015-04-14 15:52:45]

On 2015/04/13 11:57:47, k.czech wrote:
> On 2015/04/10 17:14:29, dmazzoni wrote:
> > This is better, but I think it'd be best if we didn't have to assume what
> > happens when "step" is empty, that seems like it's duplicating logic that's
> > elsewhere in the code.
> I'm wondering, seems like createStepRange returns one when step is not
> explicitly defined,
> otherwise it returns actual value of a step. It implies that we do not really
> need to have
> this 5 percent default change, I guess ?. Like you said, probably logic would
be
> duplicated.

Yes, that'd be fine with me to increment by the step amount rather than 5%.

[dmazzoni, 2015-04-14 15:54:40]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-04-14 15:54:41]

lgtm

[commit-bot: I haz the power, 2015-04-14 15:54:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071873004/60001

[commit-bot: I haz the power, 2015-04-14 18:35:19]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193722