Bug 102817 – Reset the slider thumb location before every layout of the slider container

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Carlos Garcia Campos]

The location of the slider thumb is set by the slider container assuming the render slider has been laid out. When this happens, the location of the slider thumb is reset during the layout 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.

[Carlos Garcia Campos]

Created attachment 175231 [details]
Patch

[Tony Chang]

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?

[Carlos Garcia Campos]

(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

[Alexey Proskuryakov]

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?

[Ojan Vafai]

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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Tony Chang]

(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.