Bug 56059 - REGRESSION(r76147): A slider thumb that is styled cannot be programmatically moved
Summary: REGRESSION(r76147): A slider thumb that is styled cannot be programmatically ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P1 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords: InRadar, Regression
Depends on:
Blocks: 52464
  Show dependency treegraph
 
Reported: 2011-03-09 16:12 PST by piet
Modified: 2011-03-16 11:21 PDT (History)
4 users (show)

See Also:


Attachments
test case (1.01 KB, text/html)
2011-03-10 11:11 PST, Alexey Proskuryakov
no flags Details
Patch (9.78 KB, patch)
2011-03-15 15:37 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description piet 2011-03-09 16:12:27 PST
This is a regression that I noticed after Chrome upgraded from 9 to 10. I'm developing an application that displays videos using the <video> element and custom controls. After upgrading to Chrome 10, the slider that shows the current playback position and allows to do some fast scrubbing is no longer being updated when the movie is playing.

The bug is related to the styling of the thumb. If the thumb is not styled, it is correctly updated when the value is changed by script. If the thumb is styled, it does not move anymore when the value is changed.

To reproduce:
- Go to http://www.w3schools.com/html5/tryit.asp?filename=tryhtml5_form_range
- Enter the code below in the left frame
- Click "Edit and Click Me >>"
==> BUG: The custom-styled thumb does not move every second as it should.

- Then remove the style rule that matches the thumb ("input[type="range"]::-webkit-slider-thumb")
- Click "Edit and Click Me >>"
==> OK: The native-looking thumb moves when the value is updated by script.

- Restore the style rule and re-activate the commented code that toggles between "inline-block" and "inline"
- Click "Edit and Click Me >>"
==> WORKAROUND: The custom-styled thumb now moves correctly (but that's quite an ugly workaround)

CCd dglazkov who recently refactored the slider.

Here is the code:
========

<!DOCTYPE HTML>
<html>
<style>

input[type="range"] {
    -webkit-appearance: none;
    height: 10px;
    border-radius: 4px;
    background: -webkit-gradient(linear, 0% 0%, 0% 100%, from(#444), color-stop(0.4, #888), color-stop(0.6, #888), to(#444));
}

input[type="range"]::-webkit-slider-thumb {
    -webkit-appearance: none;
    width: 10px;
    height: 28px;
    border-radius: 4px;
    background: -webkit-gradient(linear, 0% 0%, 100% 0%, from(#222), color-stop(0.4, #888), color-stop(0.6, #888), to(#222));
}

</style>
<body>

Points: <input id="slider" type="range" min="0" max="10" />
<input type="submit" />

<script>
var slider = document.getElementById('slider');
slider.value = 0;
setTimeout("test()", 1000);

function test() {
var slider = document.getElementById('slider');
slider.value++;
//if (!(slider.value % 2))
//setTimeout("slider.style.display = 'inline-block'", 1);
//else
//setTimeout("slider.style.display = 'inline'", 1);
if (slider.value < 9)
    setTimeout("test()", 1000);
}
</script>
</body>
</html>
Comment 1 Alexey Proskuryakov 2011-03-10 11:09:20 PST
Confirmed as a regression from Safari 5 using a local build of r80599.
Comment 2 Alexey Proskuryakov 2011-03-10 11:09:38 PST
<rdar://problem/9115880>
Comment 3 Alexey Proskuryakov 2011-03-10 11:11:05 PST
Created attachment 85351 [details]
test case

Same test as an attachment.
Comment 4 Dimitri Glazkov (Google) 2011-03-10 12:50:45 PST
Fixing...
Comment 5 Dimitri Glazkov (Google) 2011-03-14 16:48:38 PDT
This is more complex than I anticipated...
Comment 6 Dimitri Glazkov (Google) 2011-03-15 15:37:28 PDT
Created attachment 85865 [details]
Patch
Comment 7 Kent Tamura 2011-03-15 17:55:32 PDT
Comment on attachment 85865 [details]
Patch

ok
Comment 8 WebKit Commit Bot 2011-03-15 21:09:55 PDT
The commit-queue encountered the following flaky tests while processing attachment 85865 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2011-03-15 21:14:06 PDT
Comment on attachment 85865 [details]
Patch

Clearing flags on attachment: 85865

Committed r81216: <http://trac.webkit.org/changeset/81216>
Comment 10 WebKit Commit Bot 2011-03-15 21:14:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 piet 2011-03-16 10:52:48 PDT
If I may make a minor comment... I think that the bulk of SliderThumbElement::setPositionFromPoint() should be moved into a method called ::setValueFromPoint() and at the very end of the function, it should call ::setPositionFromValue() instead of calling directly renderer()->setNeedsLayout(true).

So it should look something like:

void SliderThumbElement::setPositionFromPoint(const IntPoint& point)
{
    HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost());
    ASSERT(input);

    if (!input->renderer() || !renderer())
        return;

    setValueFromPoint(point);
    setPositionFromValue();
    input->dispatchFormControlChangeEvent();
}

It describes better what happens. Reopen if you like.
Comment 12 Dimitri Glazkov (Google) 2011-03-16 11:21:30 PDT
(In reply to comment #11)
> If I may make a minor comment... I think that the bulk of SliderThumbElement::setPositionFromPoint() should be moved into a method called ::setValueFromPoint() and at the very end of the function, it should call ::setPositionFromValue() instead of calling directly renderer()->setNeedsLayout(true).
> 
> So it should look something like:
> 
> void SliderThumbElement::setPositionFromPoint(const IntPoint& point)
> {
>     HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost());
>     ASSERT(input);
> 
>     if (!input->renderer() || !renderer())
>         return;
> 
>     setValueFromPoint(point);
>     setPositionFromValue();
>     input->dispatchFormControlChangeEvent();
> }
> 
> It describes better what happens. Reopen if you like.

Sounds like a great first patch! :)