Bug 99938 - Touch adjustment snaps to wrong target at a plugin boundary.
Summary: Touch adjustment snaps to wrong target at a plugin boundary.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kevin Ellis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-21 10:07 PDT by Kevin Ellis
Modified: 2013-03-19 12:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.71 KB, patch)
2012-10-21 10:23 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2012-10-22 10:08 PDT, Kevin Ellis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ellis 2012-10-21 10:07:12 PDT
Touch adjustment snaps to wrong target at a plugin boundary.
Comment 1 Kevin Ellis 2012-10-21 10:23:08 PDT
Created attachment 169807 [details]
Patch
Comment 2 Kevin Ellis 2012-10-21 10:30:03 PDT
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.
Comment 3 Allan Sandfeld Jensen 2012-10-22 04:57:19 PDT
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.
Comment 4 Allan Sandfeld Jensen 2012-10-22 05:01:25 PDT
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.
Comment 5 Kevin Ellis 2012-10-22 10:08:36 PDT
Created attachment 169933 [details]
Patch
Comment 6 Kevin Ellis 2012-10-23 13:19:05 PDT
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.
Comment 7 Kevin Ellis 2012-10-25 09:11:51 PDT
Tonikitoo, can you please take a look at this latest patch.
Comment 8 Antonio Gomes 2012-10-25 10:40:18 PDT
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?
Comment 9 Kevin Ellis 2012-10-25 11:13:11 PDT
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.
Comment 10 WebKit Review Bot 2012-10-25 11:50:04 PDT
Comment on attachment 169933 [details]
Patch

Clearing flags on attachment: 169933

Committed r132509: <http://trac.webkit.org/changeset/132509>
Comment 11 WebKit Review Bot 2012-10-25 11:50:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Matt Falkenhagen 2013-03-14 00:55:11 PDT
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.
Comment 13 Allan Sandfeld Jensen 2013-03-14 04:32:20 PDT
(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.
Comment 14 Matt Falkenhagen 2013-03-19 12:11:50 PDT
(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.