Remove event forwarding logic from input[type=range], simplify event flow and thumb positioning logic.

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Remove event forwarding logic from input[type=range], simplify event flow and thumb positioning logic.

[Dimitri Glazkov (Google)]

Created attachment 78977 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78977 [details]
Patch

Do css-transformed sliders still work correctly?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> (From update of attachment 78977 [details])
> Do css-transformed sliders still work correctly?

Yup.

[Dimitri Glazkov (Google)]

(In reply to comment #0)
> Remove event forwarding logic from input[type=range], simplify event flow and thumb positioning logic.

I've been thinking if the next step here is just positioning the thumb with CSS and simply changing the corresponding coordinates in SliderThumbElement::setPosition. WDYT?

[Dimitri Glazkov (Google)]

Created attachment 79287 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> Created an attachment (id=79287) [details]
> Patch

I noticed there was an friend decl which is now unnecessary. Patch updated.

[Darin Adler]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

> Source/WebCore/html/shadow/SliderThumbElement.cpp:111
> +    ASSERT(input);
> +    ASSERT(input->renderer());
> +    ASSERT(renderer());
> +
> +    if (!input->renderer() || !renderer())
> +        return;

A couple minor thoughts on this. It’s a little strange to assert three things but only check two at runtime.

Checking for a 0 renderer often indicates that a function is suitable for calling only when layout and style calculation is up to date. That makes such functions dangerous. They can’t be called, for example, from JavaScript, which can’t guarantee layout and style is up to date. I think the event handlers that call this function may need to trigger layout and style calculation explicitly unless there’s some guarantee these have already occurred. Otherwise there can be bugs where a script changes the state of a document and the dispatches an event immediately when the renderer is in a bad state.

Also, this function later assumes both renderers are of class RenderBox, but it does not assert that nor check it at runtime outside an assertion. Is there a guarantee that we can’t get a different renderer class? If we did, renderBox() would return 0.

I think it would be better to put the two renderers into local variables and use those rather than repeatedly refetching them. In fact, each call to renderBox() redoes the runtime type check!

It’s possible none of this applies because the element is hidden from script inside a shadow tree. Not sure.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:122
> +        position = offset.y() - renderBox()->height() / 2;

I think the rounding here rounds in the vertical down direction and we normally round in the vertical up direction.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:126
> +        position = offset.x() - renderBox()->width() / 2;

I think the rounding here rounds to the right and we normally round to the left.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:140
> +    // FIXME: This is no longer being set from renderer. Consider updating the
> +    // method name.

I’d do this comment on a single line.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:151
> +    if (Frame* frame = document()->frame()) {
> +        frame->eventHandler()->setCapturingMouseEventsNode(this);
> +        m_inDragMode = true;
> +    }

What if some code moves the thumb element between documents while it’s being dragged? Will everything turn out OK?

> Source/WebCore/html/shadow/SliderThumbElement.cpp:163
> +    if (renderer())
> +        renderer()->setNeedsLayout(true);

Why? This code needs a why comment. What change causes the need for a layout? The m_inDragMode change? If so, it’s surprising that there is no need to do this in startDragging.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:185
> +        // FIXME: Ideally, we should setDefaultHandled here, but we need to fall
> +        // through to keep MediaInputControlElement bits working. Add it once
> +        // MediaElement is refactored to use shadow DOM.

But only if we are dragging. I don’t think we’d need that if the element got a mouse up that did not trigger dragging. Or maybe there is no such thing.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:189
> +        dragTo(mouseEvent->absoluteLocation());
> +        event->setDefaultHandled();

It seems wrong to set default handled on the mouse move event when we are just getting events about the mouse passing over the element and not during dragging.

Separately, I’m also not sure that we ever want to set default handled for mouse move, since a move doesn’t really need to be “consumed” the way other events are.

[Dimitri Glazkov (Google)]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

Thanks for the review. I think I need to look into questions your raise.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:111
>> +        return;
> 
> A couple minor thoughts on this. It’s a little strange to assert three things but only check two at runtime.
> 
> Checking for a 0 renderer often indicates that a function is suitable for calling only when layout and style calculation is up to date. That makes such functions dangerous. They can’t be called, for example, from JavaScript, which can’t guarantee layout and style is up to date. I think the event handlers that call this function may need to trigger layout and style calculation explicitly unless there’s some guarantee these have already occurred. Otherwise there can be bugs where a script changes the state of a document and the dispatches an event immediately when the renderer is in a bad state.
> 
> Also, this function later assumes both renderers are of class RenderBox, but it does not assert that nor check it at runtime outside an assertion. Is there a guarantee that we can’t get a different renderer class? If we did, renderBox() would return 0.
> 
> I think it would be better to put the two renderers into local variables and use those rather than repeatedly refetching them. In fact, each call to renderBox() redoes the runtime type check!
> 
> It’s possible none of this applies because the element is hidden from script inside a shadow tree. Not sure.

Thanks for looking closely at this. You are right, I need to study this more closely.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:122
>> +        position = offset.y() - renderBox()->height() / 2;
> 
> I think the rounding here rounds in the vertical down direction and we normally round in the vertical up direction.

I moved these as-is from RenderSlider::positionForOffset. I'll look to see if this matters.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:140
>> +    // method name.
> 
> I’d do this comment on a single line.

Will fix.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:151
>> +    }
> 
> What if some code moves the thumb element between documents while it’s being dragged? Will everything turn out OK?

I will look into this.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:163
>> +        renderer()->setNeedsLayout(true);
> 
> Why? This code needs a why comment. What change causes the need for a layout? The m_inDragMode change? If so, it’s surprising that there is no need to do this in startDragging.

Will fix.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:185
>> +        // MediaElement is refactored to use shadow DOM.
> 
> But only if we are dragging. I don’t think we’d need that if the element got a mouse up that did not trigger dragging. Or maybe there is no such thing.

Since we're using EventHandler::setCapturingMouseEventsNode machinery here, things are tricky, but I will look into this.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:189
>> +        event->setDefaultHandled();
> 
> It seems wrong to set default handled on the mouse move event when we are just getting events about the mouse passing over the element and not during dragging.
> 
> Separately, I’m also not sure that we ever want to set default handled for mouse move, since a move doesn’t really need to be “consumed” the way other events are.

Oh, that's right. Will fix.

[Dimitri Glazkov (Google)]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:111

>> 
>> A couple minor thoughts on this. It’s a little strange to assert three things but only check two at runtime.
>> 
>> Checking for a 0 renderer often indicates that a function is suitable for calling only when layout and style calculation is up to date. That makes such functions dangerous. They can’t be called, for example, from JavaScript, which can’t guarantee layout and style is up to date. I think the event handlers that call this function may need to trigger layout and style calculation explicitly unless there’s some guarantee these have already occurred. Otherwise there can be bugs where a script changes the state of a document and the dispatches an event immediately when the renderer is in a bad state.
>> 
>> Also, this function later assumes both renderers are of class RenderBox, but it does not assert that nor check it at runtime outside an assertion. Is there a guarantee that we can’t get a different renderer class? If we did, renderBox() would return 0.
>> 
>> I think it would be better to put the two renderers into local variables and use those rather than repeatedly refetching them. In fact, each call to renderBox() redoes the runtime type check!
>> 
>> It’s possible none of this applies because the element is hidden from script inside a shadow tree. Not sure.
> 
> Thanks for looking closely at this. You are right, I need to study this more closely.

Ok, I clearly over-ASSERT-ed here :)

* input should never be 0. That assert stays.
* input->renderer() can indeed be 0 and it's a valid situation (for example, display:none set during mousedown event).
* renderer() is same as above, which means my early return is good. Just need to get rid of the two asserts for renderers.

[Darin Adler]

(In reply to comment #9)
> * input->renderer() can indeed be 0 and it's a valid situation (for example, display:none set during mousedown event).
> * renderer() is same as above, which means my early return is good. Just need to get rid of the two asserts for renderers.

Can those also be non-RenderBox?

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> (In reply to comment #9)
> > * input->renderer() can indeed be 0 and it's a valid situation (for example, display:none set during mousedown event).
> > * renderer() is same as above, which means my early return is good. Just need to get rid of the two asserts for renderers.
> 
> Can those also be non-RenderBox?

Nope. All *Type::createRenderer() return a RenderBox.

[Darin Adler]

(In reply to comment #11)
> All *Type::createRenderer() return a RenderBox.

Not InputType::createRenderer. That one calls through to RenderObject::createObject and respects the style.

[Dimitri Glazkov (Google)]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:163
>>> +    if (renderer())
>>> +        renderer()->setNeedsLayout(true);
>> 
>> Why? This code needs a why comment. What change causes the need for a layout? The m_inDragMode change? If so, it’s surprising that there is no need to do this in startDragging.
> 
> Will fix.

Dirtying layout bit here is not necessary, ContainerNode::stateChanged handles repainting of the thumb state.

[Dimitri Glazkov (Google)]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:151
>>> +    }
>> 
>> What if some code moves the thumb element between documents while it’s being dragged? Will everything turn out OK?
> 
> I will look into this.

You were right. The setCapturingMouseEventsNode should be cleared when node is adopted. I'll file a bug on this and fix separately, since pre-patch code has the same problem.

[Dimitri Glazkov (Google)]

Comment on attachment 79287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

I think I am ready to land with tweaks mentioned.

>>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:111
>>>> +        return;
>>> 
>>> A couple minor thoughts on this. It’s a little strange to assert three things but only check two at runtime.
>>> 
>>> Checking for a 0 renderer often indicates that a function is suitable for calling only when layout and style calculation is up to date. That makes such functions dangerous. They can’t be called, for example, from JavaScript, which can’t guarantee layout and style is up to date. I think the event handlers that call this function may need to trigger layout and style calculation explicitly unless there’s some guarantee these have already occurred. Otherwise there can be bugs where a script changes the state of a document and the dispatches an event immediately when the renderer is in a bad state.
>>> 
>>> Also, this function later assumes both renderers are of class RenderBox, but it does not assert that nor check it at runtime outside an assertion. Is there a guarantee that we can’t get a different renderer class? If we did, renderBox() would return 0.
>>> 
>>> I think it would be better to put the two renderers into local variables and use those rather than repeatedly refetching them. In fact, each call to renderBox() redoes the runtime type check!
>>> 
>>> It’s possible none of this applies because the element is hidden from script inside a shadow tree. Not sure.
>> 
>> Thanks for looking closely at this. You are right, I need to study this more closely.
> 
> 

I went through all the paths and made sure the renderBox() or input->renderBox() can never be 0. Thanks for pointing out potential issues!

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:185
>>> +        // MediaElement is refactored to use shadow DOM.
>> 
>> But only if we are dragging. I don’t think we’d need that if the element got a mouse up that did not trigger dragging. Or maybe there is no such thing.
> 
> Since we're using EventHandler::setCapturingMouseEventsNode machinery here, things are tricky, but I will look into this.

I changed my mind. I don't think we should setDefaultHandled at all in these. I'll change the patch accordingly.

[Dimitri Glazkov (Google)]

Committed r76147: <http://trac.webkit.org/changeset/76147>

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (From update of attachment 79287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review
> 
> >>> Source/WebCore/html/shadow/SliderThumbElement.cpp:163
> >>> +    if (renderer())
> >>> +        renderer()->setNeedsLayout(true);
> >> 
> >> Why? This code needs a why comment. What change causes the need for a layout? The m_inDragMode change? If so, it’s surprising that there is no need to do this in startDragging.
> > 
> > Will fix.
> 
> Dirtying layout bit here is not necessary, ContainerNode::stateChanged handles repainting of the thumb state.

Turns out I was wrong. This only happens when the mouse is on the thumb. If you dragged cursor away from the track, you'll need this set to get the thumb back into the inactive state.