Created attachment 95152 [details] Patch

Comment on attachment 95152 [details] Patch Oops, the patch doesn't respect baseSize in RenderSlider::layout().

Comment on attachment 95152 [details] Patch Attachment 95152 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8741362 New failing tests: fast/multicol/client-rects.html

Created attachment 95154 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 95155 [details] Patch 2

Comment on attachment 95155 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=95155&action=review I think this patch might be a tactical win, but not much more. We should still use CSS machinery here. > Source/WebCore/html/shadow/SliderThumbElement.cpp:-90 > - // FIXME: Move the logic of positioning the thumb here. I don' think this should be removed. It's still the right place to position. > Source/WebCore/html/shadow/SliderThumbElement.cpp:139 > +inline static bool hasVerticalAppearance(HTMLInputElement* input) > +{ > + RenderStyle* sliderStyle = input->renderer()->style(); > + return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart; > +} > + > +// Returns a value between 0 and 1. > +inline static double sliderPosition(HTMLInputElement* element) > +{ > + StepRange range(element); > + return range.proportionFromValue(range.valueFromElement(element)); > +} > + > +void SliderThumbElement::setPositionDuringLayout() > +{ > + HTMLInputElement* input = hostInput(); > + ASSERT(input); > + if (!input || !input->renderBox() || !renderBox()) > + return; > + > + RenderBox* hostBox = input->renderBox(); > + RenderBox* box = renderBox(); > + double fraction = sliderPosition(input); > + IntRect hostRect = hostBox->contentBoxRect(); > + double x, y; > + if (hasVerticalAppearance(input)) { > + x = hostRect.x() + (hostRect.width() - box->width()) / 2; > + y = hostRect.y() + nextafter((hostRect.height() - box->height()) + 1, 0) * (1 - fraction); > + } else { > + x = hostRect.x() + nextafter((hostRect.width() - box->width()) + 1, 0) * fraction; > + y = hostRect.y() + (hostRect.height() - box->height()) / 2; > + } > + box->setX(clampToInteger(x)); > + box->setY(clampToInteger(y)); > +} I don't understand why you moved such RenderObject-heavy logic into an Element? Seems like you're mixing render tree and DOM tree logic here. Can this live in RenderSlider? > Source/WebCore/html/shadow/SliderThumbElement.cpp:164 > + // Ditto. This comment is in danger of losing context. You should explicitly refer to the comment above. > Source/WebCore/rendering/RenderSlider.cpp:117 > + thumbElement->setPositionDuringLayout(); You might still need LayoutStateMaintainer here. You're changing coordinates, but not accounting for any transforms.

I understand CSS works.