Closed Bug 1215434 Opened 4 years ago Closed 4 years ago

APZC shouldn't handle eWheel event when the event will be handled by plugin

Categories

(Core :: Panning and Zooming, defect)

x86
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(2 files)

I fixed bug 376679 which supports WM_MOUSEWHEEL and WM_MOUSEHWHEEL with windowless plugins. My approach of that is, EventStateManager::ComputeScrollTarget() may return nsPluginFrame (working with wheel transaction) and send the event from PostHandleEvent() to nsPluginFrame::HandleWheelEventAsDefaultAction().

With following patch, we can support it even if APZC is enabled. However, even with that, APZC also scrolls its ancestor scrollable element. I have no idea why that happens even if the patch consumes the eWheel event.

This bug is very important for Win x64 build since it will support only windowless plugins. Then, our users will see regression.
The plugin frame needs to add itself to the dispatch-to-content region so that APZ always waits for a content response before starting the scroll, and nsPluginFrame::HandleWheelEventAsDefaultAction needs to set event.mFlags.mDefaultPrevented to true so that APZEventState::ProcessWheelEvent can tell APZ that it shouldn't scroll.
Attachment #8674751 - Flags: feedback?(mstange) → feedback+
(In reply to Markus Stange [:mstange] from comment #1)
> The plugin frame needs to add itself to the dispatch-to-content region so
> that APZ always waits for a content response before starting the scroll, and
> nsPluginFrame::HandleWheelEventAsDefaultAction needs to set
> event.mFlags.mDefaultPrevented to true so that
> APZEventState::ProcessWheelEvent can tell APZ that it shouldn't scroll.

Oh, I didn't realize your comment at attaching the patch. What's the dispatch-to-content region?
> Attachment #8674751 [details] [diff] - Flags: feedback?(mstange@themasta.com) → feedback+

Hmm, the patch won't work fine, so, I've asked the reason. But I guess that the answer for comment 3 must be that.
It's a region that can be set on a layer. When APZ does hit testing for an event [1], and the hit test result is "the event is over the dispatch to content region of the targeted layer" [2], then APZ considers the input block (wheel transaction) as "not confirmed" [3][4] until it gets notified by the main thread [5][6] whether (and what) it should scroll. The dispatch-to-content region is one of several types of "event regions". Event regions are collected during display list building in nsDisplayLayerEventRegions display items, which FrameLayerBuilder later converts to the regions it sets on the layer.

I think you can just add a check for plugin frames in nsDisplayLayerEventRegions::AddFrame after the aBuilder->GetAncestorHasApzAwareEventHandler() check.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#725
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/HitTestingTreeNode.cpp#262
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#748
[4] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp#144
[5] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1954
[6] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#1176
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Hmm, the patch won't work fine, so, I've asked the reason. But I guess that
> the answer for comment 3 must be that.

Exactly, the patch implements one part of the solution, and the dispatch-to-content region is the other part that should be needed.
(In reply to Markus Stange [:mstange] from comment #5)
> then APZ considers the input
> block (wheel transaction) as "not confirmed" [3][4] until it gets notified
> by the main thread [5][6] whether (and what) it should scroll.

This was a little too simplified. There are actually two different notifications for "what to scroll" (the confirmed target) and "whether to scroll" (the content response / prevent default status). The former is sent before EventStateManager processes the event, the latter after.

Anyway, it doesn't change what you need to do here, I just wanted to clarify.
Thank you, you're right. This patch stops the double action at sending eWheel event to nsPluginFrame!
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8674777 - Flags: review?(mstange)
Comment on attachment 8674751 [details] [diff] [review]
part.1 If scroll target is a plugin frame, EventStateManager::PostHandleEvent() should send the wheel event to the plug in frame even if APZC already handled it

Let's make the wheel event on a plugin frame APZC-aware. Even when it was handled by APZ, we should send the eWheel event to the plugin frame. Then, plugin frame should consume the wheel event explicitly. After that, APZC will cancel to scroll the ancestor scrollable frame.
Attachment #8674751 - Flags: review?(bugs)
Comment on attachment 8674777 [details] [diff] [review]
part.2 Add plugin frame rect to dispatch-to-content region if it wants to handle wheel event as default action

yay
Attachment #8674777 - Flags: review?(mstange) → review+
Comment on attachment 8674751 [details] [diff] [review]
part.1 If scroll target is a plugin frame, EventStateManager::PostHandleEvent() should send the wheel event to the plug in frame even if APZC already handled it





>+  // Consume the event explicitly.
>+  aWheelEvent->mFlags.mDefaultPrevented = true;
>+  aWheelEvent->mFlags.mDefaultPreventedByChrome = true;

Would be nice to add some helper method to set these flags.
WidgetEvent::PreventDefault()
{
  mFlags.mDefaultPrevented = true;
  mFlags.mDefaultPreventedByChrome = true;
}

We probably don't need to have anything for mDefaultPreventedByContent since it should be set only via DOM Event.
Attachment #8674751 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e99568ffca32090c9a761240d85c9a7ae033e348
Bug 1215434 part.1 If scroll target is a plugin frame, EventStateManager::PostHandleEvent() should send the wheel event to the plugin frame even if APZC already handled it r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/380f6425759c35716dd0382859c8a15b74f186b8
Bug 1215434 part.2 Add plugin frame rect to dispatch-to-content region if it wants to handle wheel event as default action r=mstange
https://hg.mozilla.org/mozilla-central/rev/e99568ffca32
https://hg.mozilla.org/mozilla-central/rev/380f6425759c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.