Bug 102817

Created attachment 175231 [details] Patch

Comment on attachment 175231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > + // Reset the thumb location before layout. > + thumb->setLocation(LayoutPoint()); > + } When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position?

(In reply to comment #2) > (From update of attachment 175231 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review Thanks for reviewing. > > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > > + // Reset the thumb location before layout. > > + thumb->setLocation(LayoutPoint()); > > + } > > When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position? Yes, I think so, I'll try

Comment on attachment 175231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > Source/WebCore/ChangeLog:13 > + ignoring any previous location set. If the slider thumb is not > + laid out, the previous value is added to the new one by the slider > + container. This sounds like it should be observable testable. Is it not?

It will be observable once https://bugs.webkit.org/show_bug.cgi?id=102352 is fixed. It's already covered by existing tests.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 175231 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=175231&action=review > > Thanks for reviewing. > > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:185 > > > + if (input->sliderThumbElement() && input->sliderThumbElement()->renderer()) { > > > + thumb = toRenderBox(input->sliderThumbElement()->renderer()); > > > + // Reset the thumb location before layout. > > > + thumb->setLocation(LayoutPoint()); > > > + } > > > > When we call thumbLocation.setX, can we set the position to offset rather than adding in the old position? > > Yes, I think so, I'll try The initial position of the thumb slider is set by the parent container in RenderBlock::layoutBlockChild() and it depends on the padding, border and margin. I don't know if the thumb can be affected by those, because after the layoutBlockChild() the thumb position is always 0. In case of being greater than 0 I guess it makes sense to add the offset, but I guess we should also use the current thumb position when the container is not laid out.

Committed r135388: <http://trac.webkit.org/changeset/135388>

I landed the patch without any modification in the end to be extra sure it doesn't break anything.

(In reply to comment #6) > The initial position of the thumb slider is set by the parent container in RenderBlock::layoutBlockChild() and it depends on the padding, border and margin. I don't know if the thumb can be affected by those, because after the layoutBlockChild() the thumb position is always 0. In case of being greater than 0 I guess it makes sense to add the offset, but I guess we should also use the current thumb position when the container is not laid out. If there was a padding/border/margin on the thumb, after bug 102352 lands, we're no longer going to be applying them on a relayout since we will reset the location to 0, 0 then not call layout on the thumb. I would probably call thumbLocation.setX directly on the new position since it's less code, but it doesn't really matter. Conceptually, I don't think it makes sense for web authors to be able to use CSS to control the position of the thumb.