Bug 94473 - Clicking input type=range with padding or border sets wrong value
Summary: Clicking input type=range with padding or border sets wrong value
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:
Depends on: 94578
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-20 05:48 PDT by Keishi Hattori
Modified: 2012-08-21 06:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.61 KB, patch)
2012-08-20 06:19 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (6.80 KB, patch)
2012-08-20 20:15 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-08-20 05:48:21 PDT
Clicking input type=range with padding or border sets wrong value.
Comment 1 Keishi Hattori 2012-08-20 06:19:04 PDT
Created attachment 159409 [details]
Patch
Comment 2 Kent Tamura 2012-08-20 19:08:21 PDT
Comment on attachment 159409 [details]
Patch

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

> Source/WebCore/html/shadow/SliderThumbElement.cpp:273
>          trackSize = input->renderBox()->contentHeight() - renderBox()->height();

Need to change to trackElement->renderBox()->contentHeight()?

> Source/WebCore/html/shadow/SliderThumbElement.cpp:274
> +        position = offset.y() - renderBox()->height() / 2 - trackBoundingBox.y() + inputBoundingBox.y();

Doesn't  - (input->renderBox()->borderTop() + input->renderBox()->paddingTop()) work? If it works, we can remove trackBoundingBox and inputBoundingBox.
Comment 3 Keishi Hattori 2012-08-20 19:30:16 PDT
Comment on attachment 159409 [details]
Patch

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

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:274
>> +        position = offset.y() - renderBox()->height() / 2 - trackBoundingBox.y() + inputBoundingBox.y();
> 
> Doesn't  - (input->renderBox()->borderTop() + input->renderBox()->paddingTop()) work? If it works, we can remove trackBoundingBox and inputBoundingBox.

I think that will work for all cases where user is styling the input element, but using trackBoundingBox will be more robust to styling pseudo elements. (e.g. adding margin to the track element or adding ::before to slider container element)
Comment 4 Keishi Hattori 2012-08-20 20:15:18 PDT
Created attachment 159602 [details]
Patch
Comment 5 Kent Tamura 2012-08-20 20:16:10 PDT
Comment on attachment 159602 [details]
Patch

ok
Comment 6 WebKit Review Bot 2012-08-20 21:27:43 PDT
Comment on attachment 159602 [details]
Patch

Clearing flags on attachment: 159602

Committed r126132: <http://trac.webkit.org/changeset/126132>
Comment 7 WebKit Review Bot 2012-08-20 21:27:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Zan Dobersek 2012-08-21 00:51:44 PDT
The test introduced in the reviewed patch has been failing on all non-Chromium ports:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fforms%2Frange%2Frange-hit-test-with-padding.html

How is this test expected to behave on ports that have Shadom DOM or subpixel layout or perhaps both disabled?
Comment 9 Keishi Hattori 2012-08-21 05:40:37 PDT
(In reply to comment #8)
> The test introduced in the reviewed patch has been failing on all non-Chromium ports:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fforms%2Frange%2Frange-hit-test-with-padding.html
> 
> How is this test expected to behave on ports that have Shadom DOM or subpixel layout or perhaps both disabled?

Ports without subpixel layout won't match up exactly. I will be changing the test in Bug 94585. Please rebaseline when it lands.

I don't know of any ports that support <input type=range> but don't have Shadow DOM.
Comment 10 Keishi Hattori 2012-08-21 06:12:30 PDT
Bug 94585 is in the commit queue. I was able to change the test in a way that you won't need to rebaseline.
Comment 11 Keishi Hattori 2012-08-21 06:39:57 PDT
> I don't know of any ports that support <input type=range> but don't have Shadow DOM.
I misunderstood. This patch has nothing to do with the Shadow DOM flag.