Bug 84674

Summary: Input type=range issue with events not being raised when value set in js
Product: WebKit Reporter: balders
Component: FormsAssignee: Kevin Ellis <kevers>
Status: RESOLVED FIXED    
Severity: Major CC: ap, jonlee, kevers, mifenton, rjkroege, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.7   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description balders 2012-04-24 00:42:32 PDT
Clicking to the left of the thumb moves the thumb but does not not generate "onclick"  or "onchange" event. Second clicks work. 

<html>                  
        <body onload='document.getElementById("myRange").value=1; alert("Primed: Click to the left fails. Click to the right works.");'>
                <input type="range" id="myRange" min="0" max="3" value="0" onclick="alert('onclick');" onchange="alert('onchange');" />
        </body>                 
</html>
Comment 1 Alexey Proskuryakov 2012-04-24 10:33:03 PDT
Event dispatch certainly appears quite erratic here. And if I change maximum/initial from 3/1 to 30/10, I'm only getting change events, and never click ones.
Comment 2 Kevin Ellis 2012-06-29 11:55:05 PDT
Created attachment 150232 [details]
Patch
Comment 3 Kevin Ellis 2012-07-04 06:06:00 PDT
@tkent:  Can you please review this patch?

There are two issues. First, 'change' events were not reliably fired because of logic to prevent firing duplicate change events for textual input elements. Second, 'click' events were not reliably fired if the slider is repositioned beneath the cursor in the process of clicking.  Both issues are addressed in the patch.
Comment 4 Kent Tamura 2012-07-08 22:00:24 PDT
Comment on attachment 150232 [details]
Patch

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

The C++ change looks ok.  Need some improvement for tests.

> LayoutTests/fast/events/click-range-slider.html:19
> +function log(s)
> +{
> +    document.getElementById('console').appendChild(document.createTextNode(s + "\n"));
> +}

Remove this. This is not used.

> LayoutTests/fast/events/click-range-slider.html:28
> +if (window.testRunner) {
> +    testRunner.dumpAsText();
> +}

Remove this.  js-test-pre.js does it.

> LayoutTests/fast/events/click-range-slider.html:55
> +    shouldBeEqualToString("String(clickCount)", "3");

Should be:
    shouldBe("clickCount", "3");

> LayoutTests/fast/events/onchange-range-slider.html:19
> +function log(s)
> +{
> +    document.getElementById('console').appendChild(document.createTextNode(s + "\n"));
> +}

Remove this.  js-test-pre.js provides debug() function.

> LayoutTests/fast/events/onchange-range-slider.html:23
> +    log ('PASS: change event fired.');

Use testPassed() provided by js-test-pre.js.

> LayoutTests/fast/events/onchange-range-slider.html:30
> +if (window.testRunner) {
> +    testRunner.dumpAsText();
> +}

Remove this.

> LayoutTests/fast/events/onchange-range-slider.html:52
> +        log('Fail: change event not fired.');

Use testFailed() provided by js-test-pre.js.
Comment 5 Kevin Ellis 2012-07-09 12:13:57 PDT
Created attachment 151294 [details]
Patch
Comment 6 Kevin Ellis 2012-07-09 12:26:40 PDT
Comment on attachment 150232 [details]
Patch

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

>> LayoutTests/fast/events/click-range-slider.html:19
>> +}
> 
> Remove this. This is not used.

Done.

>> LayoutTests/fast/events/click-range-slider.html:28
>> +}
> 
> Remove this.  js-test-pre.js does it.

Done.

>> LayoutTests/fast/events/click-range-slider.html:55
>> +    shouldBeEqualToString("String(clickCount)", "3");
> 
> Should be:
>     shouldBe("clickCount", "3");

Done.

>> LayoutTests/fast/events/onchange-range-slider.html:19
>> +}
> 
> Remove this.  js-test-pre.js provides debug() function.

Done.

>> LayoutTests/fast/events/onchange-range-slider.html:30
>> +}
> 
> Remove this.

Done.

>> LayoutTests/fast/events/onchange-range-slider.html:52
>> +        log('Fail: change event not fired.');
> 
> Use testFailed() provided by js-test-pre.js.

Done.
Comment 7 Alexey Proskuryakov 2012-07-09 12:34:41 PDT
Am I right to assume that these tests pass in Firefox?
Comment 8 Kevin Ellis 2012-07-09 12:58:13 PDT
(In reply to comment #7)
> Am I right to assume that these tests pass in Firefox?

Does not appear that Firefox supports rendering <input type="range"> as a slider.

Bug 344618 - Implement <input type="range">

Tested on Firefox 13.0.1.
Comment 9 Kent Tamura 2012-07-09 20:27:23 PDT
Comment on attachment 151294 [details]
Patch

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

> LayoutTests/fast/events/onchange-range-slider.html:24
> +if (window.testRunner) {
> +    testRunner.dumpAsText();

Should remove this.  js-test-pre.js does it.
Comment 10 Kevin Ellis 2012-07-10 06:29:21 PDT
Created attachment 151450 [details]
Patch
Comment 11 Kent Tamura 2012-07-10 06:30:26 PDT
Comment on attachment 151450 [details]
Patch

ok
Comment 12 WebKit Review Bot 2012-07-10 08:51:46 PDT
Comment on attachment 151450 [details]
Patch

Clearing flags on attachment: 151450

Committed r122224: <http://trac.webkit.org/changeset/122224>
Comment 13 WebKit Review Bot 2012-07-10 08:51:51 PDT
All reviewed patches have been landed.  Closing bug.