Bug 30802

Summary: Implement automatic switching to slider-vertical
Product: WebKit Reporter: Mike Taylor <runningtowardsnothing>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Enhancement CC: adele, arv, jwieczorek, mike, paulirish, rolandsteiner, syoichi, tkent
Priority: P2    
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-

Description Mike Taylor 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
Comment 1 Kent Tamura 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.
Comment 2 Kent Tamura 2010-05-22 12:46:16 PDT
Created attachment 56785 [details]
Patch
Comment 3 Kent Tamura 2010-05-22 12:47:42 PDT
Created attachment 56786 [details]
Patch (rev.2)
Comment 4 Jakub Wieczorek 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.
Comment 5 Kent Tamura 2010-05-23 00:08:15 PDT
Created attachment 56812 [details]
Patch (rev.3)
Comment 6 Kent Tamura 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.
Comment 7 Ojan Vafai 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().
Comment 8 Kent Tamura 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.