Touch adjustment snaps to wrong target at a plugin boundary.
Created attachment 169807 [details] Patch
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 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 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.
Created attachment 169933 [details] Patch
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.
Tonikitoo, can you please take a look at this latest patch.
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 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 on attachment 169933 [details] Patch Clearing flags on attachment: 169933 Committed r132509: <http://trac.webkit.org/changeset/132509>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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.