[webkit-reviews] review granted: [Bug 98666] input[type=range] as a flex item renders thumb at wrong position : [Attachment 168739] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Tony Chang <tony at chromium.org>'s
request for review:
Bug 98666: input[type=range] as a flex item renders thumb at wrong position
https://bugs.webkit.org/show_bug.cgi?id=98666

Attachment 168739: Patch
https://bugs.webkit.org/attachment.cgi?id=168739&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168739&action=review


Big improvement!

> Source/WebCore/ChangeLog:39
> +	   * rendering/RenderSlider.h: Fix indention.

s/indention/indentation

> Source/WebCore/html/shadow/SliderThumbElement.cpp:178
> +    RenderBox* thumb = toRenderBox(input->sliderThumbElement()->renderer());


sliderThumbElement can exist without having a renderer, e.g. if in the
inspector you point to it with a global variable and then delete it. Not sure
if this is possible in practice since it's in the shadow DOM.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:186
> +	   thumbLocation.setY(thumbLocation.y() + track->contentHeight() -
offset);

FWIW, I think for consistency sake, we should support RTL on vertical sliders
as well to have them start at the top. You obviously don't need to fix this,
but maybe add a FIXME? In an ideal world, we'd swap it so that RTL vertical
sliders start at the bottom and LTR ones start at the top.

> LayoutTests/platform/chromium/TestExpectations:4012
> +webkit.org/b/98666 [ Win Mac ]
fast/forms/datalist/input-appearance-range-with-datalist.html [ Failure ]

Won't this be ImageOnlyFailure?