Bug 92640 - Slider should snap to datalist tick marks
Summary: Slider should snap to datalist tick marks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords: WebExposed
Depends on: 93074
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 05:41 PDT by Keishi Hattori
Modified: 2012-08-03 00:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.36 KB, patch)
2012-07-31 04:35 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (15.14 KB, patch)
2012-08-01 19:09 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (14.72 KB, patch)
2012-08-02 01:34 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (14.15 KB, patch)
2012-08-02 02:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2012-08-02 03:36 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-07-30 05:41:42 PDT
Slider maybe should snap to datalist tick marks.
Comment 1 Keishi Hattori 2012-07-30 05:46:23 PDT
Here is a Mac build with snapping turned on so we can discuss whether we do this.
https://docs.google.com/open?id=0BwRi59l_ri74ZUtkTHZuLUhKUTA
Comment 2 Keishi Hattori 2012-07-31 04:35:35 PDT
Created attachment 155490 [details]
Patch
Comment 3 Kent Tamura 2012-07-31 07:17:29 PDT
Comment on attachment 155490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review

> Source/WebCore/html/shadow/SliderThumbElement.cpp:277
> +#if ENABLE(DATALIST_ELEMENT)

We should have a new function for this code.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:283
> +        for (unsigned i = 0; Node* node = options->item(i); i++) {

Please follow an idiom; for (unsigned i = 0; i < options->length(); ++i) {
Your current code disturbs code readers.

BTW, this code is called whenever mousemove is dispatched during the thumb dragging, and this code is O(N).  We should improve the complexity.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:285
> +            ASSERT(node->hasTagName(optionTag));
> +            HTMLOptionElement* optionElement = static_cast<HTMLOptionElement*>(node);

We have toHTMLOptionElement().

> Source/WebCore/html/shadow/SliderThumbElement.cpp:306
> +&& closest.isFinite() && !closest.isNaN())

wrong indentation
Comment 4 yosin 2012-07-31 18:33:34 PDT
Comment on attachment 155490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review

> Source/WebCore/html/shadow/SliderThumbElement.cpp:287
> +            if (!input->isValidValue(optionValue))

Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows:
const Decimal rawTickValue = parseToDecimalForNumberType(optionValue);
if (!rawTickValue.isFinite())
    continue;
const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum());

> Source/WebCore/html/shadow/SliderThumbElement.cpp:291
> +            if (difference > 0 && tickValue < closestRight)

You can use Decimal::isPositive() and isNegative() functions instead of comparing to zero.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:292
> +                closestRight = tickValue;

We can write this

if (difference.isPositive())
    closestRight = std::min(closestRight, tickValue)
else if (difference.isNegative())
    closestLeft = std::max(closestLeft, tickValue)
else
    closestLeft = closestRight = tickValue;

> Source/WebCore/html/shadow/SliderThumbElement.cpp:301
> +        double closestFraction = stepRange.proportionFromValue(closest).toDouble();

* Can we do decimal arithmetic instead of double arithmetic?
* It is better to check closest.isFinite() and skip L301-L307 if !closest.isFinite(). We can see closest is ininity when !options.length().

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:306
>> +&& closest.isFinite() && !closest.isNaN())
> 
> wrong indentation

closes.isFinite() implies !closes.isNaN(). So, we don't need to have "&& !closes.isNan()".
Comment 5 yosin 2012-07-31 18:50:33 PDT
Comment on attachment 155490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:287
>> +            if (!input->isValidValue(optionValue))
> 
> Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows:
> const Decimal rawTickValue = parseToDecimalForNumberType(optionValue);
> if (!rawTickValue.isFinite())
>     continue;
> const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum());

Oops, tickValue should be sanitized, so we can do
const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue());
const Decimal tickValue = stepRange.clampValue(originalTickValue);
Comment 6 Kent Tamura 2012-07-31 19:30:48 PDT
Comment on attachment 155490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:287
>>> +            if (!input->isValidValue(optionValue))
>> 
>> Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows:
>> const Decimal rawTickValue = parseToDecimalForNumberType(optionValue);
>> if (!rawTickValue.isFinite())
>>     continue;
>> const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum());
> 
> Oops, tickValue should be sanitized, so we can do
> const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue());
> const Decimal tickValue = stepRange.clampValue(originalTickValue);

We should skip the step-mismatched values, and shouldn't alter the option value here.  Skip the value with isValidValue() is ok.
Comment 7 yosin 2012-08-01 05:22:31 PDT
(In reply to comment #6)
> (From update of attachment 155490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155490&action=review
> 
> >>> Source/WebCore/html/shadow/SliderThumbElement.cpp:287
> >>> +            if (!input->isValidValue(optionValue))
> >> 
> >> Do we need to use HTMLInputElement functions? Because of we have stepRange for input element. We can do as follows:
> >> const Decimal rawTickValue = parseToDecimalForNumberType(optionValue);
> >> if (!rawTickValue.isFinite())
> >>     continue;
> >> const Decimal tickValue = std::max(std::min(rawTickValue, stepRange.maximum()), stepRange.minimum());
> > 
> > Oops, tickValue should be sanitized, so we can do
> > const Decimal originalTickValue = parseToDecimalForNumberType(optionValue, stepRange.defaultValue());
> > const Decimal tickValue = stepRange.clampValue(originalTickValue);
> 
> We should skip the step-mismatched values, and shouldn't alter the option value here.  Skip the value with isValidValue() is ok.

HTMLInputElement::isValidValue() creates StepRange class many times.

How about adding bool StepRange::isValidValue(const Decimal&)?

It can be

bool StepRange::isValidalue(const Decimal& value)
{
    if (!value.isFinite())
        return false;
    if (value < m_minimum || value > m_maximum)
        return false;
    return value.remainder(m_step).isZero();
}
Comment 8 Keishi Hattori 2012-08-01 19:09:30 PDT
Created attachment 155950 [details]
Patch
Comment 9 Keishi Hattori 2012-08-01 19:17:22 PDT
> HTMLInputElement::isValidValue() creates StepRange class many times.

I tried to avoid this by moving the code into RangeInputType and doing the equivalent to isValidValue with one StepRange.
Comment 10 Build Bot 2012-08-01 19:24:02 PDT
Comment on attachment 155950 [details]
Patch

Attachment 155950 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13411697
Comment 11 Build Bot 2012-08-01 19:24:08 PDT
Comment on attachment 155950 [details]
Patch

Attachment 155950 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13411695
Comment 12 Kent Tamura 2012-08-01 20:30:36 PDT
Comment on attachment 155950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155950&action=review

> Source/WebCore/html/HTMLInputElement.cpp:320
> +Decimal HTMLInputElement::findClosestTickMarkValue(Decimal value)
> +{
> +    return m_inputType->findClosestTickMarkValue(value);
> +}
> +

Please move it into the existing #if ENABLE(DATALIST_ELEMENT) block.

> Source/WebCore/html/HTMLInputElement.h:77
> +#if ENABLE(DATALIST_ELEMENT)
> +    Decimal findClosestTickMarkValue(Decimal);
> +#endif

Please move the declaration into the existing #if ENABLE(DATALIST_ELEMENT) block.

> Source/WebCore/html/RangeInputType.cpp:58
> +#if ENABLE(TOUCH_EVENTS)

Maybe DATALIST_ELEMENT ?

> Source/WebCore/html/RangeInputType.cpp:352
> +    Vector<Decimal> tickValues;

This is not used.

> Source/WebCore/html/RangeInputType.cpp:366
> +        String optionValue = optionElement->value();
> +        if (typeMismatchFor(optionValue))
> +            continue;
> +        Decimal rawOptionValue = parseToNumber(optionValue, Decimal::nan());
> +        if (!rawOptionValue.isFinite()
> +            || stepRange.stepMismatch(rawOptionValue)
> +            || rawOptionValue < stepRange.minimum()
> +            || rawOptionValue > stepRange.maximum())

We should use HTMLInputElement::isValidValue() because
 - This is a duplication of HTMLInputElement::isValidValue(). It's hard to maintain both to be synchronized forever.
 - Now we have a cache of datalist values. The cost of StepRange creation is not important.

If you'd like to improve the performance, you should improve isValidValue().

> Source/WebCore/html/RangeInputType.cpp:385
> +        middle = (left + right) / 2;

possible integer overflow.

> Source/WebCore/html/RangeInputType.h:80
>  #endif
> +
> +    bool m_tickMarkValuesDirty;
> +    Vector<Decimal> m_tickMarkValues;

They should be in #if ENABLE(DATALIST_ELEMENT)

> Source/WebCore/html/shadow/SliderThumbElement.cpp:241
>  
> +
> +

this change is unnecessary.
Comment 13 Keishi Hattori 2012-08-02 01:34:20 PDT
Created attachment 156004 [details]
Patch
Comment 14 yosin 2012-08-02 01:42:09 PDT
Comment on attachment 156004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review

> Source/WebCore/html/HTMLInputElement.cpp:317
> +Decimal HTMLInputElement::findClosestTickMarkValue(Decimal value)

nit: Is it better to use const Decimal& as other functions which take Decimal?

> Source/WebCore/html/RangeInputType.cpp:340
> +static bool decimalCompare(Decimal a, Decimal b)

Please use "const Decimal&" to avoid copying Decimal objects.

> Source/WebCore/html/RangeInputType.cpp:363
> +        m_tickMarkValues.append(stepRange.clampValue(parseToNumber(optionValue, stepRange.defaultValue())));

We don't need to call StepRange::clampValue, because isValidValue is true.

> Source/WebCore/html/RangeInputType.cpp:395
> +    Decimal closestLeft = middle ? m_tickMarkValues[middle - 1] : Decimal::infinity(Decimal::Negative);

nit: const Decimal

> Source/WebCore/html/RangeInputType.cpp:396
> +    Decimal closestRight = middle != m_tickMarkValues.size() ? m_tickMarkValues[middle] : Decimal::infinity(Decimal::Positive);

nit: const Decimal

> Source/WebCore/html/shadow/SliderThumbElement.cpp:278
> +    Decimal closest = input->findClosestTickMarkValue(value);

nit: const Decimal

> Source/WebCore/html/shadow/SliderThumbElement.cpp:280
> +        double closestFraction = stepRange.proportionFromValue(closest).toDouble();

Is it better to do in decimal arithmetic?
Could you tell me why do we use double arithmetic? For speed?
Comment 15 Kent Tamura 2012-08-02 01:50:20 PDT
Comment on attachment 156004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review

> Source/WebCore/html/InputType.cpp:914
> +    return Decimal::nan();

ASSERT_NOT_REACHED().

> Source/WebCore/html/RangeInputType.cpp:380
> +        middle = (static_cast<long>(left) + static_cast<long>(right)) / 2;

Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long).

middle = left + (right - left) / 2;

> LayoutTests/fast/forms/datalist/range-snap-to-datalist.html:66
> +<script>
> +    var input = document.getElementById("input");
> +    var thumbWidth = 15;
> +    var halfThumbWidth = (thumbWidth - 1) / 2;
> +    function clickSlider(offsetLeft) {
> +        var centerY = input.offsetTop + input.offsetHeight / 2;
> +        eventSender.mouseMoveTo(input.offsetLeft + offsetLeft, centerY);
> +        eventSender.mouseDown();
> +        eventSender.mouseUp();
> +    }
> +    function resetSliderPosition() {
> +        clickSlider(0);
> +        shouldBe('input.value', '"0"');
> +    }
> +    function expectedValueForPosition(pos) {
> +        clickSlider(pos);
> +        var value = Math.round((pos - halfThumbWidth - 0.5) / (input.offsetWidth - thumbWidth) * (input.max - input.min) + input.min);
> +        value = Math.max(Math.min(value, input.max), input.min);
> +        return value;
> +    }
> +    resetSliderPosition();
> +    clickSlider(45);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(46);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(47);
> +    shouldBeTrue('parseInt(input.value, 10) < 500');
> +    resetSliderPosition();
> +    clickSlider(48);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(49);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(50);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(51);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(52);
> +    shouldBe('input.value', "'500'");
> +    resetSliderPosition();
> +    clickSlider(53);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +    resetSliderPosition();
> +    clickSlider(54);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +    resetSliderPosition();
> +    clickSlider(55);
> +    shouldBeTrue('parseInt(input.value, 10) > 500');
> +</script>

nit: Indenting everything is not helpful.
Comment 16 Kent Tamura 2012-08-02 01:55:40 PDT
Comment on attachment 156004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review

> LayoutTests/fast/forms/datalist/range-snap-to-datalist.html:32
> +    function expectedValueForPosition(pos) {
> +        clickSlider(pos);
> +        var value = Math.round((pos - halfThumbWidth - 0.5) / (input.offsetWidth - thumbWidth) * (input.max - input.min) + input.min);
> +        value = Math.max(Math.min(value, input.max), input.min);
> +        return value;
> +    }

This function is not used.
Comment 17 Keishi Hattori 2012-08-02 02:03:22 PDT
Created attachment 156010 [details]
Patch
Comment 18 Keishi Hattori 2012-08-02 02:05:40 PDT
> > Source/WebCore/html/shadow/SliderThumbElement.cpp:280
> > +        double closestFraction = stepRange.proportionFromValue(closest).toDouble();
> 
> Is it better to do in decimal arithmetic?
> Could you tell me why do we use double arithmetic? For speed?

Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double.
Comment 19 yosin 2012-08-02 02:15:57 PDT
(In reply to comment #15)
> (From update of attachment 156004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156004&action=review
> 
> > Source/WebCore/html/InputType.cpp:914
> > +    return Decimal::nan();
> 
> ASSERT_NOT_REACHED().
> 
> > Source/WebCore/html/RangeInputType.cpp:380
> > +        middle = (static_cast<long>(left) + static_cast<long>(right)) / 2;
> 
> Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long).
> 
> middle = left + (right - left) / 2;
> 

It is impossible to have elements in Vector due by available memory.
Comment 20 Kent Tamura 2012-08-02 02:21:40 PDT
(In reply to comment #19)
> > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long).
> > 
> > middle = left + (right - left) / 2;
> > 
> 
> It is impossible to have elements in Vector due by available memory.

Right.  However we should always write correct code because the code might be copied, and the copy might have integer overflow.
Comment 21 Kent Tamura 2012-08-02 02:23:47 PDT
Comment on attachment 156010 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156010&action=review

> Source/WebCore/html/RangeInputType.cpp:356
> +    StepRange stepRange(createStepRange(RejectAny));

This StepRange looks unnecessary because parseToNumber call below never fails.
Comment 22 yosin 2012-08-02 02:26:55 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long).
> > > 
> > > middle = left + (right - left) / 2;
> > > 
> > 
> > It is impossible to have elements in Vector due by available memory.
> 
> Right.  However we should always write correct code because the code might be copied, and the copy might have integer overflow.

Can we use binarySearch in WTF/wtf/StdLibExtras.h?
http://trac.webkit.org/browser/trunk/Source/WTF/wtf/StdLibExtras.h
Comment 23 Keishi Hattori 2012-08-02 02:35:26 PDT
(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > > Still have possibility of integer overflow because sizeof(size_t) is usually same as sizeof(long).
> > > > 
> > > > middle = left + (right - left) / 2;
> > > > 
> > > 
> > > It is impossible to have elements in Vector due by available memory.
> > 
> > Right.  However we should always write correct code because the code might be copied, and the copy might have integer overflow.
> 
> Can we use binarySearch in WTF/wtf/StdLibExtras.h?
> http://trac.webkit.org/browser/trunk/Source/WTF/wtf/StdLibExtras.h

I guess its possible by creating a new class that represents the space between tick marks. And doing a binary search against that.
Comment 24 yosin 2012-08-02 02:42:05 PDT
(In reply to comment #18)
> > > Source/WebCore/html/shadow/SliderThumbElement.cpp:280
> > > +        double closestFraction = stepRange.proportionFromValue(closest).toDouble();
> > 
> > Is it better to do in decimal arithmetic?
> > Could you tell me why do we use double arithmetic? For speed?
> 
> Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double.

Agree, we don't need to use decimal arithmetic.
However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit.
Comment 25 Kent Tamura 2012-08-02 03:07:11 PDT
(In reply to comment #24)
> (In reply to comment #18)
> > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:280
> > > > +        double closestFraction = stepRange.proportionFromValue(closest).toDouble();
> > > 
> > > Is it better to do in decimal arithmetic?
> > > Could you tell me why do we use double arithmetic? For speed?
> > 
> > Should I use decimal arithmetic? My thinking was that Decimal precision is unnecessary and it would be slightly faster to use double.
> 
> Agree, we don't need to use decimal arithmetic.
> However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit.

We shouldn't use LayoutUnit here.  LayoutUnit is an alias of int if !ENABLE(SUBPIXEL_LAYOUT).
Comment 26 Kent Tamura 2012-08-02 03:16:56 PDT
(In reply to comment #25)
> > However, it seems it is better to use LayoutUnit instead of double. Because postion and trackSize are LayoutUnit and closestPostion also can be LayoutUnit.
> 
> We shouldn't use LayoutUnit here.  LayoutUnit is an alias of int if !ENABLE(SUBPIXEL_LAYOUT).

We discussed offline.
We had better make 'closestPosition' and 'snapThreashold' LayoutUnit because they are pixel lengths.
Comment 27 Keishi Hattori 2012-08-02 03:36:49 PDT
Created attachment 156028 [details]
Patch
Comment 28 Kent Tamura 2012-08-02 03:45:38 PDT
Comment on attachment 156028 [details]
Patch

ok
Comment 29 WebKit Review Bot 2012-08-02 19:06:06 PDT
Comment on attachment 156028 [details]
Patch

Clearing flags on attachment: 156028

Committed r124549: <http://trac.webkit.org/changeset/124549>
Comment 30 WebKit Review Bot 2012-08-02 19:06:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 János Badics 2012-08-03 00:40:22 PDT
Unfortunately the newly added test (fast/forms/datalist/range-snap-to-datalist.html) fails on Qt and EFL bots since r124549. I created a bug entry for the issue:
https://bugs.webkit.org/show_bug.cgi?id=93074

Would you give it a look please?