Closed Bug 1385071 Opened 7 years ago Closed 7 years ago

Allow keyboard APZ with passive listeners, behind a pref

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: milan, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

Bug 1385071 - Add another method to EventTarget for detecting only non-passive key listeners.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1385071 - Allow keyboard APZ with passive listeners, behind a pref.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1385071 - Don't do the default action for key inputs that are handled by APZ.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
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: → 1383900
Whiteboard: [gfx-noted]
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+
(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 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 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 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 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 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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: