Closed Bug 1376551 Opened 7 years ago Closed 7 years ago

APZ: Don't always lose the focus target when the mouse moves

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1376551 - Track whether there are pointer movement listeners in a window.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1376551 - Don't lose APZ focus state when the mouse moves and there are no listeners.
59 bytes, text/x-review-board-request
kats
: review+
Details 
Currently all input events force APZ to lose it's focus target until content returns with a focus update. What this means, is that if the user is moving the mouse while keyboard scrolling, we will never acquire focus in APZ and will always sync scroll, which is not optimal.

An alternative is to track whether the current page has mousemove listeners, and when it doesn't, we can consider mousemove listeners as non-focus changing.
I used "mouse movement listeners" to refer to event listeners that could be triggered by a mouse move widget input. This is more than just 'mousemove'. It's kind of confusing, so I'm open to a better name.
Comment on attachment 8893301 [details]
Bug 1376551 - Don't lose APZ focus state when the mouse moves and there are no listeners.

https://reviewboard.mozilla.org/r/164370/#review169848

::: gfx/layers/apz/src/FocusState.cpp:105
(Diff revision 1)
> +        // Intentionally only check for mouse movement listeners in the leaf
> +        // focus target because chrome has quite a few mouse movement listeners
> +        mHasMouseMovementEventListeners = target.mHasMouseMovementEventListeners;

Just to make sure I understand, this is an optimization path that would result in incorrect behaviour if the chrome mouse listeners changed focus inside the content, right? (That's probably a pretty unlikely scenario, so I'm ok with this, but we should make the comment more explicit about the failure condiiton).
Attachment #8893301 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 8893301 [details]
> Bug 1376551 - Don't lose APZ focus state when the mouse moves and there are
> no listeners.
> 
> https://reviewboard.mozilla.org/r/164370/#review169848
> 
> ::: gfx/layers/apz/src/FocusState.cpp:105
> (Diff revision 1)
> > +        // Intentionally only check for mouse movement listeners in the leaf
> > +        // focus target because chrome has quite a few mouse movement listeners
> > +        mHasMouseMovementEventListeners = target.mHasMouseMovementEventListeners;
> 
> Just to make sure I understand, this is an optimization path that would
> result in incorrect behaviour if the chrome mouse listeners changed focus
> inside the content, right? (That's probably a pretty unlikely scenario, so
> I'm ok with this, but we should make the comment more explicit about the
> failure condiiton).

Yes, if the chrome mousemove listeners were to change focus we could have a race. This is the same problem we have for key listeners, where we check for non system group listeners for untrusted events. Which I had forgotten about.. Testing it out, the same solution works.

I'll update the patch to make it consistent with key listeners, and also fix another problem I thought of.
Now we ignore chrome mouse movement listeners with the same criteria we use for keyboard listeners (non-system and wants untrusted).

I also updated the FocusState code to check for mHasMouseMovementListeners on all layer tree's, because that corresponds to the whole visible region of the window.
Comment on attachment 8893300 [details]
Bug 1376551 - Track whether there are pointer movement listeners in a window.

https://reviewboard.mozilla.org/r/164368/#review169970

::: dom/events/EventListenerManager.cpp:389
(Diff revision 2)
>            "They are slower than pointerover/out!");
>  #endif
>          window->SetHasPointerEnterLeaveEventListeners();
>        }
>      }
> -  } else if (aTypeAtom == nsGkAtoms::onmouseenter ||
> +  } else if (aTypeAtom == nsGkAtoms::onmousemove ||

So what about pointerevents?
And since those should be checked too, naming should be more like MayHavePointerMovementListeners.
Do we need to care about touch too?
Attachment #8893300 - Flags: review?(bugs) → review-
I don't think we need to care about touch; users are much less likely to be touching the screen randomly while keyboard navigating. But yeah, any pointer events that can be triggered by mouse movement should be added to the list as listeners for those events can also change focus.
(In reply to Olli Pettay [:smaug] from comment #9)
>
> Do we need to care about touch too?

I don't think we need to care about touch listeners. We're only using this for physical mouse inputs.
Summary: APZ: Consider not always losing the focus target when the mouse moves → APZ: Don't always lose the focus target when the mouse moves
I don't quite understand what we try to achieve with this. What if listeners are in some iframe?
oh, we do call parent windows method too, ok.

But any iframe which has movement listener affects to the top level window.
I would assume many ads have some mousemove listeners to detect activity.


Does this patch really help in real world? Could we at least have some telemetry first to tell whether this would help?
Or am I misunderstanding the reasoning for this all.
Comment on attachment 8893300 [details]
Bug 1376551 - Track whether there are pointer movement listeners in a window.

https://reviewboard.mozilla.org/r/164368/#review170232

I think I'd prefer to see some evidence this would actually help. Or explain how I've misunderstood this all :)
Attachment #8893300 - Flags: review?(bugs)
Comment on attachment 8893300 [details]
Bug 1376551 - Track whether there are pointer movement listeners in a window.

https://reviewboard.mozilla.org/r/164368/#review170258

or, I guess we can try this. But some telemetry about whether this helps would be good.

::: dom/base/nsPIDOMWindowInlines.h:134
(Diff revision 3)
> +    return;
> +  }
> +
> +  mMayHavePointerMovementEventListener = true;
> +
> +  if (nsCOMPtr<nsPIDOMWindowOuter> outer = GetOuterWindow()) {

Why all the nsCOMPtr usage here? Try to just use raw pointers here if possible.

::: dom/events/EventListenerManager.cpp:378
(Diff revision 3)
> -             aEventMessage <= ePointerEventLast) {
> +              aEventMessage <= ePointerEventLast) ||
> +             (aEventMessage >= eMouseEventFirst &&
> +              aEventMessage <= eMouseEventLast)) {
>      nsPIDOMWindowInner* window = GetInnerWindowForTarget();
> +
> +    if ((aTypeAtom == nsGkAtoms::onpointermove ||

Please check first that !mMayHavePointerMovementEventListener
Attachment #8893300 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8893300 [details]
> Bug 1376551 - Track whether there are pointer movement listeners in a window.
> 
> https://reviewboard.mozilla.org/r/164368/#review170258
> 
> or, I guess we can try this. But some telemetry about whether this helps
> would be good.
> 

I also wonder about the usefulness about this. My fear is that this won't kick in often, there may be a similar problem to key listeners. We could possibly use the passive flag here, to help out.

I'll do some browsing with this patch and total up how often there aren't any pointer movement listeners.

If it seems like there are some wins, we could possibly land this with telemetry and get some more data.
I visited some sites with logging on, and essentially all of them had mouse/pointer movement listeners.

Sites with listeners:
google.com
bbc.com
amazon.com
bing.com
yahoo.com
youtube.com
github.com
bugzilla.mozilla.org
facebook.com
messenger.com
irccloud.com

Additionally, from a spot check, the event listeners are not marked passive, so we can't filter them out.

So with that, I'm not sure if there is much benefit to this for the overhead it adds.
Closing as WONTFIX based on comment 19. We can reopen it later if we have other ideas on how to make this work better.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: