Allow keyboard APZ with passive listeners, behind a pref

RESOLVED FIXED in Firefox 57

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED FIXED
22 days ago
13 days ago

People

(Reporter: milan, Assigned: rhunt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

We currently block keyboard APZ with all listeners (see bug 1383900), which blocks too many pages.  Just blocking on the passive listeners may fail with focus type problems.

Add a preference to "ignore" the potential problems and enable keyboard APZ as long as all listeners are passive.
Assignee: nobody → rhunt
Blocks: 1376525
See Also: → bug 1383900
Whiteboard: [gfx-noted]
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

22 days ago
mozreview-review
Comment on attachment 8891134 [details]
Bug 1385071 - Allow keyboard APZ with passive listeners, behind a pref.

https://reviewboard.mozilla.org/r/162312/#review167788

Some concerns below. Somebody else should probably review the XBLWindowKeyHandler change.

::: dom/xbl/nsXBLWindowKeyHandler.cpp:497
(Diff revision 2)
> +    // If this event was handled by APZ then don't do the default action
> +    if (widgetKeyboardEvent->mFlags.mHandledByAPZ) {

I don't know the XBL code well enough to know if this is the right place to put this check. It looks like it's inside a IsKeyEventOnPlugin() block which seems a little odd as we don't want to restrict the behaviour to plugins. But I don't know if that function actually means what it sounds like.

But I agree that we do need to check this flag and abort the XBL handler so as to not double-handle the event, I just don't know where that check should go.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1727
(Diff revision 2)
>      // If we're scroll snapping, use a smooth scroll animation to get
>      // the desired physics. Note that SmoothScrollTo() will re-use an
>      // existing smooth scroll animation if there is one.
>      APZC_LOG("%p keyboard scrolling to snap point %s\n", this, Stringify(*snapPoint).c_str());
>      SmoothScrollTo(*snapPoint);
>      return nsEventStatus_eConsumeNoDefault;

Doesn't this return site also need to check APZKeyboardPassiveListeners and return DoDefault if true?

::: gfx/layers/apz/src/FocusTarget.cpp:67
(Diff revision 2)
>    nsTArray<EventTarget*> targets;
>    nsresult rv = EventDispatcher::Dispatch(aContent, nullptr, &event, nullptr,
>        nullptr, nullptr, &targets);
>    NS_ENSURE_SUCCESS(rv, false);
>    for (size_t i = 0; i < targets.Length(); i++) {
> -    if (targets[i]->HasUntrustedOrNonSystemGroupKeyEventListeners()) {
> +    if (targets[i]->HasNonSystemGroupListenersForUntrustedKeyEvents()) {

This rename should be in the previous patch

::: gfx/layers/apz/src/FocusTarget.cpp:144
(Diff revision 2)
> +      HasListenersForNonPassiveKeyEvents(focusedContent ? focusedContent.get()
> +                                                        : document->GetUnfocusedKeyEventTarget());

Would be slightly more readable to extract the "focusedContent ? ..." duplication out of the if condition by storing the result of the ternary operator into a local var
Attachment #8891134 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> ::: dom/xbl/nsXBLWindowKeyHandler.cpp:497
> (Diff revision 2)
> > +    // If this event was handled by APZ then don't do the default action
> > +    if (widgetKeyboardEvent->mFlags.mHandledByAPZ) {
> 
> I don't know the XBL code well enough to know if this is the right place to
> put this check. It looks like it's inside a IsKeyEventOnPlugin() block which
> seems a little odd as we don't want to restrict the behaviour to plugins.
> But I don't know if that function actually means what it sounds like.
> 

Whoops, yeah that was in the wrong spot. Not sure how it was working for me then.

I've split out the XBL change and asked Masayuki to review it.

Comment 9

19 days ago
mozreview-review
Comment on attachment 8891812 [details]
Bug 1385071 - Don't do the default action for key inputs that are handled by APZ.

https://reviewboard.mozilla.org/r/162850/#review168188

::: dom/xbl/nsXBLWindowKeyHandler.cpp:507
(Diff revision 1)
> +  // If this event was handled by APZ then don't do the default action
> +  if (widgetKeyboardEvent->mFlags.mHandledByAPZ) {
> +    return NS_OK;
> +  }

I feel that this is odd. That reason of this is, APZ *already* handled default action, isn't it? Then, APZ should calls PreventDefault() of the event when it handles (takes) the event.
Attachment #8891812 - Flags: review?(masayuki) → review-
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> I feel that this is odd. That reason of this is, APZ *already* handled
> default action, isn't it? Then, APZ should calls PreventDefault() of the
> event when it handles (takes) the event.

Because of the APZ architecture, APZ doesn't operate on WidgetEvent types. It operates on InputData types which don't have that function. We propagate the fact that APZ handled the event by using the mHandledByAPZ flag, and not just for keyboard events. It's used in other places/for other input types as well because in some cases behaviour is more complex than simply skipping the gecko default action (e.g. in some cases we need to send additional information back to APZ). Using this flag here is more consistent with existing handling of input events when APZ is around.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> > I feel that this is odd. That reason of this is, APZ *already* handled
> > default action, isn't it? Then, APZ should calls PreventDefault() of the
> > event when it handles (takes) the event.
> 
> Because of the APZ architecture, APZ doesn't operate on WidgetEvent types.
> It operates on InputData types which don't have that function. We propagate
> the fact that APZ handled the event by using the mHandledByAPZ flag, and not
> just for keyboard events. It's used in other places/for other input types as
> well because in some cases behaviour is more complex than simply skipping
> the gecko default action (e.g. in some cases we need to send additional
> information back to APZ). Using this flag here is more consistent with
> existing handling of input events when APZ is around.

Well, the consistency is important. However, from the point of view of Event Handling, checking mHandledByAPZ in event handlers means that *all* default event listeners need to check both mHandledByAPZ and DefaultPrevented(). In other words, Event Handling depends on APZ design. This is really odd design to me since this means that there could be other similar bugs in other default keyboard event listeners.

And also APZ should be the only module which can judge if the event should cause default action with the event even after APZ takes it. At least in KeyboardEvent case, any keyboard events shouldn't be handled twice nor more. So, when a keyboard event should be consumed by APZ. I believe that in the other cases except some events which may send some information back to APZ should be dispatched with default prevented state if mHandledByAPZ is true.

So, I really recommend that when mHandledByAPZ becomes true, APZ should also call PeventDefault(). And then, in follow up bug, this odd design should be fixed for the other events.

Additionally, I'm confusing at the meaning of mHandledByAPZ.

The document is here:
https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/BasicEvents.h#121-124
>   // The event's action will be handled by APZ. The main thread should not
>   // perform its associated action. This is currently only relevant for
>   // wheel and touch events.
>   bool mHandledByAPZ : 1;

So, the original patch <https://bugzilla.mozilla.org/attachment.cgi?id=8592827&action=diff> said, when this is true, default action shouldn't be executed. Therefore, I feel that your example cases are special cases. Do such cases need to distinguish if preventDefault() is called by content or not? If so, you can use DefaultPreventedByContent(), though <https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/BasicEvents.h#208>.  Otherwise, APZ should keep not calling DefaultPrevent() when setting mHandledByAPZ to true only for the specific cases/events.

Anyway, you need to update this comment for conforming to the latest design.
Yeah I guess that comment is slightly out-of-date, we should update it. I think we cannot simply replace mHandledByAPZ with PreventDefault() across the board since it is used for a number of things (e.g. telemetry [1] and special handling [2]). So at best we can do both - set mHandledByAPZ and also call PreventDefault().

However, if we call PreventDefault() on the event, doesn't that get exposed to web content? These events might in some cases [3] still be dispatched to key listeners in web content, and they might be checking the .defaultPrevented property on the event for whatever reason - if we set that in APZ then their behaviour might change. Or is there a way to set it so that it's not detectable by web content? The semantics of DefaultPreventedByContent() vs DefaultPrevented() are not totally clear to me. If this is not a concern then I guess we can start calling PreventDefault() on key events and see how it goes.

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/base/PresShell.cpp#8076
[2] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/ipc/TabChild.cpp#1604
[3] In general we will avoid doing APZ handling of key events if there are listeners registered, but we cannot handle all scenarios - for example if a listener is registered via setTimeout concurrently with APZ handling the event.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> I think we cannot simply replace mHandledByAPZ with PreventDefault() across
> the board since it is used for a number of things (e.g. telemetry [1] and
> special handling [2]). So at best we can do both - set mHandledByAPZ and
> also call PreventDefault().

Yes, I think that we should do the latter.

> However, if we call PreventDefault() on the event, doesn't that get exposed
> to web content? These events might in some cases [3] still be dispatched to
> key listeners in web content, and they might be checking the
> .defaultPrevented property on the event for whatever reason - if we set that
> in APZ then their behaviour might change. Or is there a way to set it so
> that it's not detectable by web content? The semantics of
> DefaultPreventedByContent() vs DefaultPrevented() are not totally clear to
> me. If this is not a concern then I guess we can start calling
> PreventDefault() on key events and see how it goes.

When default handlers call PreventDefault(), its hidden optional argument is set to true. Then, it's not marked as the event is prevented its default by content, i.e., DefaultPreventedByContent() doesn't return true.

If dom::Event::DefaultPrevented() is called by content script, it uses the result of DefaultPreventedByContent():
https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/events/Event.cpp#1081,1086,1093-1094

So, event.defaultPrevented in content script is still false even after calling PreventDefault() in C++ code. This was implemented by bug 930374.
Ok, thanks. Ryan, can you try calling PreventDefault on aEvent just before [1] if mHandledByAPZ is true and it's a keyboard event, and see if that works?

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/nsBaseWidget.cpp#1050

Comment 15

18 days ago
mozreview-review
Comment on attachment 8891133 [details]
Bug 1385071 - Add another method to EventTarget for detecting only non-passive key listeners.

https://reviewboard.mozilla.org/r/162310/#review168516
Attachment #8891133 - Flags: review?(bugs) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Ok, thanks. Ryan, can you try calling PreventDefault on aEvent just before
> [1] if mHandledByAPZ is true and it's a keyboard event, and see if that
> works?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/nsBaseWidget.cpp#1050

Hmm, so when I did this I noticed that the keypress events weren't being dispatched to content listeners when they were handled by APZ. 

Investigating the problem, it appears that when WidgetEvent->DefaultPrevented() == true, we don't dispatch the events to the content process. [1][2]

We could get around this by calling preventDefault on the event when it is created in the content process [3], but I want to be sure we're using preventDefault correctly and this make me unsure. I'm also unsure if that would cause problems with chrome pages, because then only content would have the event preventDefaulted.

Masayuki, does that solution seem correct?

[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/events/EventListenerManager.cpp#1387
[2] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/events/EventStateManager.cpp#1337
[3] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/ipc/TabChild.cpp#1894
Flags: needinfo?(masayuki)
(In reply to Ryan Hunt [:rhunt] from comment #16)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> > Ok, thanks. Ryan, can you try calling PreventDefault on aEvent just before
> > [1] if mHandledByAPZ is true and it's a keyboard event, and see if that
> > works?
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/nsBaseWidget.cpp#1050
> 
> Hmm, so when I did this I noticed that the keypress events weren't being
> dispatched to content listeners when they were handled by APZ. 
> 
> Investigating the problem, it appears that when
> WidgetEvent->DefaultPrevented() == true, we don't dispatch the events to the
> content process. [1][2]

Yeah, I know this, but I forgot this... I think that this is odd because if the event is already consumed by chrome, it should be fired in the content first, or it should stop cross process forwarding with using StopCrossProcessForwarding(). For example, Alt key events on Windows may be consumed when it's being dispatched from chrome since native key messages have such state on Windows. So, I think that we should not check if the event is consumed here.  How do you think, smaug? I think that if we shouldn't do that, we should fix it in another bug before this bug.

> We could get around this by calling preventDefault on the event when it is
> created in the content process [3], but I want to be sure we're using
> preventDefault correctly and this make me unsure. I'm also unsure if that
> would cause problems with chrome pages, because then only content would have
> the event preventDefaulted.
> 
> Masayuki, does that solution seem correct?

So, I think that parent process should send events whose defaultPrevented is true to remote process except when they are stopped forwarding to remote process explicitly.
Flags: needinfo?(masayuki) → needinfo?(bugs)
Calling preventDefault in chrome should stop calling listeners in web page.
If one doesn't want that behavior, better to let content handle the event first and then resend event to chrome.
Flags: needinfo?(bugs)
So what should we do here Masayuki? I'm not sure what dispatching this event to content first and then resending it to chrome would look like.
Flags: needinfo?(masayuki)
I don't think we can even do that in this case because the "chrome" in this context is APZ which is on a different thread. Sending the event to content before sending it to APZ defeats the purpose of having APZ.
Who is calling preventDefault and where and why?
(In reply to Olli Pettay [:smaug] from comment #21)
> Who is calling preventDefault and where and why?

We'd like to dispatch a key event to the DOM after its scroll has been handled by APZ. We need to prevent the nsXBLWindowKeyHandler from double handling the scroll.

The original patch used mHandledByAPZ to prevent double handling the default action. Masayuki suggested using preventDefault instead to be consistent with other cases.

So we would call preventDefault on the event here [1] if the input is a key input and was handled by APZ.

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/widget/nsBaseWidget.cpp#1050
Consistent with what cases?
IMO mHandledByAPZ approach sounds better, and more consistent with the setup we have now.
(In reply to Olli Pettay [:smaug] from comment #23)
> Consistent with what cases?

I think I slightly misread Masayuki. I'll just quote the relevant part:

> Well, the consistency is important. However, from the point of view of Event
> Handling, checking mHandledByAPZ in event handlers means that *all* default
> event listeners need to check both mHandledByAPZ and DefaultPrevented(). In
> other words, Event Handling depends on APZ design. This is really odd design
> to me since this means that there could be other similar bugs in other
> default keyboard event listeners.

> IMO mHandledByAPZ approach sounds better, and more consistent with the setup
> we have now.

I'm fine with using mHandledByAPZ also, I just want to do whatever is consistent with previous work.
Although, I still don't understand why there are the cases that chrome consumes events but not stop cross process forwarding since in such case, chrome should request reply event from content process. And as I mentioned above, stopping cross process forwarding with defaultPrevented state, we must have inconsistent behavior with non-e10s.

However, they are not worthwhile to block this bug. Okay, use mHandledByAPZ.
Flags: needinfo?(masayuki)

Comment 26

16 days ago
mozreview-review-reply
Comment on attachment 8891812 [details]
Bug 1385071 - Don't do the default action for key inputs that are handled by APZ.

https://reviewboard.mozilla.org/r/162850/#review168188

> I feel that this is odd. That reason of this is, APZ *already* handled default action, isn't it? Then, APZ should calls PreventDefault() of the event when it handles (takes) the event.

Okay, let's use this approach.

However, I'm really afraid that there could be some other event listeners which only check defaultPrevented but not check mHandledByAPZ in chrome.

Comment 27

16 days ago
mozreview-review
Comment on attachment 8891812 [details]
Bug 1385071 - Don't do the default action for key inputs that are handled by APZ.

https://reviewboard.mozilla.org/r/162850/#review169540
Attachment #8891812 - Flags: review- → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #26)
> Comment on attachment 8891812 [details]
> Bug 1385071 - Don't do the default action for key inputs that are handled by
> APZ.
> 
> https://reviewboard.mozilla.org/r/162850/#review168188
> 
> > I feel that this is odd. That reason of this is, APZ *already* handled default action, isn't it? Then, APZ should calls PreventDefault() of the event when it handles (takes) the event.
> 
> Okay, let's use this approach.
> 
> However, I'm really afraid that there could be some other event listeners
> which only check defaultPrevented but not check mHandledByAPZ in chrome.

What other default key event listeners are there?

Keyboard APZ will only consume key events that scroll, and the event target will be the body or root element. We shouldn't have to worry about interactive elements like buttons, input, or textarea.
Typically frames for XUL elements and JS in chrome (and some addons? I'm not sure about WebExtensions, though). They can listen  to keyboard events of the root element.

E.g., nsMenuBarListener has 3 key event listeners and they don't check mHandledByAPZ. I guess that they won't match with key values handled by APZ for now (when the keyboard event will go to remote process), though. However, such design is risky for regressions of future changes.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #29)
> Typically frames for XUL elements and JS in chrome (and some addons? I'm not
> sure about WebExtensions, though). They can listen  to keyboard events of
> the root element.
> 
> E.g., nsMenuBarListener has 3 key event listeners and they don't check
> mHandledByAPZ. I guess that they won't match with key values handled by APZ
> for now (when the keyboard event will go to remote process), though.
> However, such design is risky for regressions of future changes.

Ah, I see what you mean. That actually is a problem even without this patch.

If there was a key input that is handled by APZ, but it would have also been handled by nsMenuBarListener, APZ would consume the event and nsMenuBarListener would never get it. When nsMenuBarListener should get the chance to handle the event before we scroll.

With this patch and pref enabled in the same situation, the input would be handled by APZ, the event would be dispatched to content, and nsMenuBarListener would then also handle the event. Which means both default actions would happen, which isn't much better.

The handling of chrome key event listeners with key APZ isn't perfect right now. We currently just ignore them. This patch is mostly for improving the web content side.

I'll do some thinking on improving this, maybe in another patch..
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8891812 [details]
Bug 1385071 - Don't do the default action for key inputs that are handled by APZ.

To prevent the issues with other default key listeners having to check for mHandledByAPZ, we have nsXBLWindowKeyHandler check for mHandledByAPZ and then preventDefault the event then. This is what normally happens for sync key events when they are handled by nsXBLPrototypeHandler [1].

[1] http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/dom/xbl/nsXBLPrototypeHandler.cpp#526
Attachment #8891812 - Flags: review+ → review?(masayuki)
Attachment #8891812 - Flags: review?(masayuki) → review+

Comment 35

14 days ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8b3b018cf5
Add another method to EventTarget for detecting only non-passive key listeners. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/aafd06fa1990
Don't do the default action for key inputs that are handled by APZ. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/91bd0a08cabe
Allow keyboard APZ with passive listeners, behind a pref. r=kats
Blocks: 1387566
https://hg.mozilla.org/mozilla-central/rev/2d8b3b018cf5
https://hg.mozilla.org/mozilla-central/rev/aafd06fa1990
https://hg.mozilla.org/mozilla-central/rev/91bd0a08cabe
Status: NEW → RESOLVED
Last Resolved: 14 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.