459604 - nsDOMWindowUtils::SendMouseEvent doesn't accept points outside the viewport

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Stuart Parmenter]

Go to nytimes.com, should be able to click on links on the page but can't.

I think this is probably a event/content/layout bug.  Needs a reduced (non-fennec) testcase.

Comment 1

[Daniel Brooks [:db48x]]

For what it's worth, not calling scrollTo() in _clientToContentCoords() "fixes" the problem, although of course once you've panned any they stop working again, as the events are redispatched to the wrong coordinates. I'm not sure exactly what that means, however.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Looks like the nytimes site now has a "Welcome to TimesPeople" bar at the top of the page which interferes with clicking. We try to scroll the point we're clicking on to 0,0, but the top bar follows the scroll and therefore intercepts the click.

Really what we need is to be able to dispatch events outside of the visible viewport so that we can avoid scrolling entirely.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

A somewhat hacky workaround would be to just scroll things to 50,50 instead, or something.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That should be pretty easy. You just want an option to make nsLayoutUtils::GetFrameForPoint (called via PresShell::HandleEvent) build its display list using SetIgnoreScrollFrame on the root scrollframe, like nsPresShell::RenderDocument does when we call it from DrawWindow.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This addresses fennec's use case, by allowing us to click on and find elements outside the viewport. I'm not sure whether the change to PresShell::HandleEvent is going to cause compatibility problems.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cool, that actually works? :-)

I'm not sure how to expose this in API. We can't just make this change to elementFromPoint, since the spec is clear it should return null for coordinates outside the viewport, but we could add a new method Moz-only method alongside elementFromPoint that handles outside-the-viewport points. We can't just make this change to PresShell::HandleEvent, since it would at least mess up mouse events targeting the viewport scrollbars. We might be able to add a parameter to DOMWindowUtils::SendMouseEvent to trigger this behaviour, but it would be a little ugly since we'd have to add a flag to nsMouseEvent.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: SendMouseEvent patch

This adds a member to nsGUIEvent about whether or not the scroll frame should be ignored during dispatch. I'm not sure whether this is acceptable, since it bloats all events and is not typically useful, but as you say there aren't really any immediately obvious alternatives for doing this. Other nsIDOMWindowUtils methods could also make use of this, but I only changed sendMouseEvent for now since that's all Fennec cares about.

I'm not really sure what to do with elementFromPoint - I suppose adding an optional argument to its signature isn't an option? If we go with the alternate method route, what interface should it be on, and what should the method be called?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why can't you add that flag to nsMouseEvent_base? It won't use any more space there. And make it a PRPackedBool.

If you're using elementFromPoint from chrome for something in Fennec, then you could just add your special elementFromPoint variant to nsIDOMWindowUtils.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Why can't you add that flag to nsMouseEvent_base? It won't use any more space
> there. And make it a PRPackedBool.

Ah, I had originally done that, but nsPresShell::HandleEvent takes an nsGUIEvent*, and I hadn't noticed that it was possible to differentiate using eventStructType. I'll do that.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Hmm, should I use eventStructType == NS_MOUSE_EVENT, or NS_IS_MOUSE_EVENT?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: SendMouseEvent patch

I went with eventStructType since that seemed more likely to be reliable.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 344497 [details] [diff] [review]
SendMouseEvent patch

Are there use cases for other event types (which are also mouse events, or inheriting nsMouseEvent_base). Drag events? Mouse scroll?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Fennec doesn't have an immediate use for non-click mouse events. I'm not sure I understand why you're asking, though.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

maybe drag and drop events?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Sure, we might find the need to make additional changes in the future (either to DOMWindowUtils to add the optional parameter to more methods, or to the event classes to allow this for non-mouse events). I don't think we should worry about that until the need arises, though.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 344497 [details] [diff] [review]
SendMouseEvent patch

+
+  // Optionally ignore scroll frame
+  if (aIgnoreScrollFrame)
+    event.ignoreScrollFrame = PR_TRUE;

Why not just event.ignoreScrollFrame = aIgnoreScrollFrame?

+    if (rootScrollFrame)
+      builder.SetIgnoreScrollFrame(rootScrollFrame);

{} around the statement

Comment 17

[:Gavin Sharp [email: [e-mail]]]

I split off the elementFromPoint part to bug 461803.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Landed with those comments addressed:
http://hg.mozilla.org/mozilla-central/rev/7b456bc0eb30

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I guess this added new warning:
../../../../../dist/include/widget/nsGUIEvent.h:745: warning: ‘nsMouseEvent::clickCount’ will be initialized after
../../../../../../dist/include/widget/nsGUIEvent.h:738: warning:   ‘PRPackedBool nsMouseEvent::ignoreScrollFrame’
../../../../../../dist/include/widget/nsGUIEvent.h:700: warning:   when initialized here