Bug 61132 - fix render overflow computation for input type=range
Summary: fix render overflow computation for input type=range
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 59644
  Show dependency treegraph
 
Reported: 2011-05-19 10:48 PDT by Tony Chang
Modified: 2011-05-24 14:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.93 KB, patch)
2011-05-19 10:51 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2011-05-19 11:11 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (4.09 KB, patch)
2011-05-19 14:07 PDT, Tony Chang
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-05-19 10:48:07 PDT
fix render overflow computation for input type=range
Comment 1 Tony Chang 2011-05-19 10:51:56 PDT
Created attachment 94089 [details]
Patch
Comment 2 Tony Chang 2011-05-19 11:11:20 PDT
Created attachment 94092 [details]
Patch
Comment 3 Tony Chang 2011-05-19 11:11:55 PDT
(In reply to comment #2)
> Created an attachment (id=94092) [details]
> Patch

I moved the call to m_overflow.clear() to be outside the thumb check.  It's possible for the thumb to be remove and the overflow not cleared.
Comment 4 James Robinson 2011-05-19 13:44:20 PDT
Comment on attachment 94092 [details]
Patch

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

Test is (theoretically at least) flaky

> LayoutTests/fast/forms/slider-hit-testing.html:1
> +<head>

<!DOCTYPE html> plz unless this test needs quirks mode

> LayoutTests/fast/forms/slider-hit-testing.html:11
> +window.setTimeout(function() {
> +  document.getElementById("slider").style.width = "100%";
> +  changeAndCheckRangeValue();
> +}, 0);

the setTimeout is a bit odd - does the test not work if you run this code in an onload handler? technically speaking, this setTimeout _could_ execute immediately after parsing the end of the <script> and before the <input> is parsed, although in practice we'll nearly always parse through the end of this document before running timeouts.

> LayoutTests/fast/forms/slider-hit-testing.html:20
> +    var x = slider.offsetLeft + (slider.clientWidth * .75);
> +    var y = slider.offsetTop + (slider.clientHeight / 2);

could you add some comments indicating what you are trying to click on here and what should be happening?
Comment 5 Tony Chang 2011-05-19 14:07:17 PDT
Created attachment 94112 [details]
Patch
Comment 6 Tony Chang 2011-05-19 14:08:23 PDT
Comment on attachment 94092 [details]
Patch

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

>> LayoutTests/fast/forms/slider-hit-testing.html:1
>> +<head>
> 
> <!DOCTYPE html> plz unless this test needs quirks mode

Done.

>> LayoutTests/fast/forms/slider-hit-testing.html:11
>> +}, 0);
> 
> the setTimeout is a bit odd - does the test not work if you run this code in an onload handler? technically speaking, this setTimeout _could_ execute immediately after parsing the end of the <script> and before the <input> is parsed, although in practice we'll nearly always parse through the end of this document before running timeouts.

I didn't work in the onload handler, but I added a call to document.body.offsetLeft to force a layout.  This allowed me to remove the waitUntilDone/notifyDone calls.

>> LayoutTests/fast/forms/slider-hit-testing.html:20
>> +    var y = slider.offsetTop + (slider.clientHeight / 2);
> 
> could you add some comments indicating what you are trying to click on here and what should be happening?

Done.
Comment 7 James Robinson 2011-05-23 19:24:58 PDT
Comment on attachment 94112 [details]
Patch

Looks good!
Comment 8 Tony Chang 2011-05-24 10:44:04 PDT
Committed r87168: <http://trac.webkit.org/changeset/87168>
Comment 9 Ademar Reis 2011-05-24 14:01:49 PDT
Revision r87168 cherry-picked into qtwebkit-2.2 with commit 49b8a1e <http://gitorious.org/webkit/qtwebkit/commit/49b8a1e>