Bug 99938 – Touch adjustment snaps to wrong target at a plugin boundary.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kevin Ellis]

Touch adjustment snaps to wrong target at a plugin boundary.

[Kevin Ellis]

Created attachment 169807 [details]
Patch

[Kevin Ellis]

Touch adjustment snaps to an on-screen target when I touch on a control in a flash plugin (e.g. Youtube flash player).

<embed> elements were not considered as candidates for touch adjustment.  It is difficult to tap on a control at the bottom of the flash player without also clipping the toolbar below the player, which triggers touch adjustment to the toolbar buttons.

[Allan Sandfeld Jensen]

Comment on attachment 169807 [details]
Patch

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

> Source/WebCore/html/HTMLEmbedElement.cpp:239
> +bool HTMLEmbedElement::willRespondToMouseClickEvents()
> +{
> +    return !disabled();
> +}

I think we might want to make this slightly more specific. For instance check if it actually embeds a plugin or plugin-document.

[Allan Sandfeld Jensen]

Comment on attachment 169807 [details]
Patch

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

>> Source/WebCore/html/HTMLEmbedElement.cpp:239
>> +}
> 
> I think we might want to make this slightly more specific. For instance check if it actually embeds a plugin or plugin-document.

Specifically. WillRespondToMouseClickEvents is a companion function to defaultEventHandler, so it might fit  better in HTMLPlugInElement where the defaultEventHandler is, and it uses a toRenderWidget()->widget() call to get the widget. We could say it could responds to mouseClickEvents if it has a widget.

[Kevin Ellis]

Created attachment 169933 [details]
Patch

[Kevin Ellis]

Comment on attachment 169807 [details]
Patch

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

>>> Source/WebCore/html/HTMLEmbedElement.cpp:239
>>> +}
>> 
>> I think we might want to make this slightly more specific. For instance check if it actually embeds a plugin or plugin-document.
> 
> Specifically. WillRespondToMouseClickEvents is a companion function to defaultEventHandler, so it might fit  better in HTMLPlugInElement where the defaultEventHandler is, and it uses a toRenderWidget()->widget() call to get the widget. We could say it could responds to mouseClickEvents if it has a widget.

Done, with one small caveat.  The default event handler has some special handling for embedded objects that is performed before testing for a widget, thus basing the rule on being an embedded object or widget.

[Kevin Ellis]

Tonikitoo, can you please take a look at this latest patch.

[Antonio Gomes]

Comment on attachment 169933 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.cpp:88
> +    if (!r->isEmbeddedObject() && !r->isWidget())

AND or OR?

[Kevin Ellis]

Comment on attachment 169933 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInElement.cpp:88
>> +    if (!r->isEmbeddedObject() && !r->isWidget())
> 
> AND or OR?

And is correct here.  Could have used !(A || B) instead.

[WebKit Review Bot]

Comment on attachment 169933 [details]
Patch

Clearing flags on attachment: 169933

Committed r132509: <http://trac.webkit.org/changeset/132509>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Falkenhagen]

Comment on attachment 169933 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.cpp:83
> +    if (disabled())

Hi, I'm working on refactoring Node::disabled. It seems the concept is really just for form control elements. Do you expect HTMLPlugInElement to have disable() return true ever?

My guess is this just copied Node::willRespondToMouseClickEvents(), in which case I should be able to remove this call in the refactoring, but wanted to ask just in case.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 169933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169933&action=review
> 
> > Source/WebCore/html/HTMLPlugInElement.cpp:83
> > +    if (disabled())
> 
> Hi, I'm working on refactoring Node::disabled. It seems the concept is really just for form control elements. Do you expect HTMLPlugInElement to have disable() return true ever?
> 
> My guess is this just copied Node::willRespondToMouseClickEvents(), in which case I should be able to remove this call in the refactoring, but wanted to ask just in case.

While disabled() is mainly for form-elements, it has been named genericly so that any Node objects could have circumstances where they are disabled. I would keep the check, but no, I don't think there are any cases currently where it would return true.

[Matt Falkenhagen]

(In reply to comment #13)
Thanks! (I forgot to CC myself earlier). The context of the Node::disabled refactoring is bug 112085 and bug 110952. I'm still working through the codebase and seeing if it makes sense to move it out of Node.