Bug 30802

Summary:

Implement automatic switching to slider-vertical

Product:

WebKit

Reporter:

Mike Taylor <runningtowardsnothing>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Enhancement

CC:

adele, arv, jwieczorek, mike, paulirish, rolandsteiner, syoichi, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

URL:

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-input-element-as-a-range-control

Bug Depends on:

Bug Blocks:

19264

Attachments:

Description	Flags
Patch	none
Patch (rev.2)	none
Patch (rev.3)	ojan: review-

Mike Taylor

Reported 2009-10-26 19:41:44 PDT

Per the spec, if the range input's height is greater than its width it should render as a vertical slider. WebKit's implementation does not do this yet. A test page can be found here: http://miketaylr.com/test/vertical-range-input.html

Attachments
Patch (19.14 KB, patch) 2010-05-22 12:46 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Patch (rev.2) (19.14 KB, patch) 2010-05-22 12:47 PDT, Kent Tamura	no flags	Details Formatted Diff Diff
Patch (rev.3) (19.95 KB, patch) 2010-05-23 00:08 PDT, Kent Tamura	ojan: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2010-05-22 12:25:28 PDT

We already have vertical slider internally and you can see it by style="width:20px;height:200px;-webkit-appearance:slider-vertical;". So we need to implement automatic switching from slider-horizontal to slider-vertical.

Kent Tamura

Comment 2 2010-05-22 12:46:16 PDT

Created attachment 56785 [details] Patch

Kent Tamura

Comment 3 2010-05-22 12:47:42 PDT

Created attachment 56786 [details] Patch (rev.2)

Jakub Wieczorek

Comment 4 2010-05-22 15:02:24 PDT

> diff --git a/WebCore/rendering/RenderTheme.cpp b/WebCore/rendering/RenderTheme.cpp > index 445e637bf2d703fee9f5dc27d3b06e71335bff5d..baa8d7cde1c55fc122236f21b044b7a0fa92d42d 100644 > --- a/WebCore/rendering/RenderTheme.cpp > +++ b/WebCore/rendering/RenderTheme.cpp > @@ -962,8 +962,10 @@ void RenderTheme::adjustMenuListButtonStyle(CSSStyleSelector*, RenderStyle*, Ele > { > } > > -void RenderTheme::adjustSliderTrackStyle(CSSStyleSelector*, RenderStyle*, Element*) const > +void RenderTheme::adjustSliderTrackStyle(CSSStyleSelector*, RenderStyle* style, Element*) const > { > + if (style->height().isFixed() && style->width().isFixed() && style->height().value() > style->width().value()) > + style->setAppearance(SliderVerticalPart); > } > > void RenderTheme::adjustSliderThumbStyle(CSSStyleSelector*, RenderStyle*, Element*) const > diff --git a/WebCore/rendering/RenderThemeMac.mm b/WebCore/rendering/RenderThemeMac.mm > index f1de7759e85349b948d2e4d445e22bd88f844de3..ca69babdb25f6d38279f4d362aed964f0e0e4b68 100644 > --- a/WebCore/rendering/RenderThemeMac.mm > +++ b/WebCore/rendering/RenderThemeMac.mm > @@ -1150,8 +1150,9 @@ int RenderThemeMac::minimumMenuListSize(RenderStyle* style) const > const int trackWidth = 5; > const int trackRadius = 2; > > -void RenderThemeMac::adjustSliderTrackStyle(CSSStyleSelector*, RenderStyle* style, Element*) const > +void RenderThemeMac::adjustSliderTrackStyle(CSSStyleSelector* selector, RenderStyle* style, Element* element) const > { > + RenderTheme::adjustSliderTrackStyle(selector, style, element); > style->setBoxShadow(0); > } > The Qt port also reimplements this function (in WebCore/platform/qt/RenderThemeQt.cpp), hence it should call the base one too.

Kent Tamura

Comment 5 2010-05-23 00:08:15 PDT

Created attachment 56812 [details] Patch (rev.3)

Kent Tamura

Comment 6 2010-05-23 00:09:54 PDT

(In reply to comment #4) > The Qt port also reimplements this function (in WebCore/platform/qt/RenderThemeQt.cpp), hence it should call the base one too. Thank you for pointing it out! I fixed it.

Ojan Vafai

Comment 7 2010-07-22 16:15:51 PDT

Comment on attachment 56812 [details] Patch (rev.3) > + if (style->height().isFixed() && style->width().isFixed() && style->height().value() > style->width().value()) This won't work for percentage heights/widths, will it? What about the intrinsic width? Can you add that case to your test case? I'm not sure about this, but I think you might want to look at style->paddingBox().

Kent Tamura

Comment 8 2010-07-22 18:44:37 PDT

(In reply to comment #7) > (From update of attachment 56812 [details]) > > + if (style->height().isFixed() && style->width().isFixed() && style->height().value() > style->width().value()) > > This won't work for percentage heights/widths, will it? What about the intrinsic width? Can you add that case to your test case? I'm not sure about this, but I think you might want to look at style->paddingBox(). Right, this won't work for percentage length. The patch was a hack and we shouldn't do this here. We need more complex change like <meter> does.

Note You need to log in before you can comment on or make changes to this bug.