Bad event dispatch on engadget.com

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla39
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

()

Attachments

(8 attachments, 6 obsolete attachments)

5.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.67 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.16 KB, patch
smaug
: review+
Details | Diff | Splinter Review
642 bytes, patch
kats
: review+
Details | Diff | Splinter Review
32.08 KB, patch
botond
: review+
tnikkel
: feedback+
Details | Diff | Splinter Review
1.26 KB, patch
kats
: review+
Details | Diff | Splinter Review
4.76 KB, patch
Details | Diff | Splinter Review
With APZ enabled, when I visit engadget.com and hover over anything in the menu bar, I can still scroll the content underneath. With APZ disabled that doesn't happen.
Posted patch fix (obsolete) — Splinter Review
The InputQueue code for scroll events was never propagating nsEventStatus_eConsumeNoDefault back.
Attachment #8556363 - Flags: review?(bugmail.mozilla)
Comment on attachment 8556363 [details] [diff] [review]
fix

Review of attachment 8556363 [details] [diff] [review]:
-----------------------------------------------------------------

In general return values from AsyncPanZoomController::HandleInputEvent are not meant to be returned out of APZCTreeManager. Despite the fact that they are both nsEventStatus types, they are really incompatible. Can you explain more about the problem you're seeing here?
Attachment #8556363 - Flags: review?(bugmail.mozilla) → review-
Posted patch fix v2 (obsolete) — Splinter Review
Don't take ACTION_SCROLL if we expect the scroll animation to happen in the compositor.
Attachment #8556363 - Attachment is obsolete: true
Attachment #8556735 - Flags: review?(bugs)
Comment on attachment 8556735 [details] [diff] [review]
fix v2

Review of attachment 8556735 [details] [diff] [review]:
-----------------------------------------------------------------

You probably want to use "action" in the switch below?
Comment on attachment 8556735 [details] [diff] [review]
fix v2

Hrm, you're right. I still had my original patch applied. With just this, things work, but the performance is really bad. So it must not be enough.
Attachment #8556735 - Flags: review?(bugs)
Posted patch fix (obsolete) — Splinter Review
Okay, this works after all. It just exacerbates a timing issue in the APZ smooth scrolling code where if a scroll animation completes very quickly then the velocity doesn't carry over to the next scroll event. That's a separate issue though.
Attachment #8556735 - Attachment is obsolete: true
Attachment #8558195 - Flags: review?(bugs)
Attachment #8558195 - Flags: review?(bugs) → review+
That failing test actually exposed quite a few problems. The first is that, not all wheel events will be handled by APZ, so the exact same check needs to be in EventStateManager as well.
Attachment #8558195 - Attachment is obsolete: true
Flags: needinfo?(dvander)
Attachment #8560052 - Flags: review?(bugs)
These aren't the best names, but whatever. The new method is virtual in nsIWidget so PuppetWidget can forward it to the parent process.
Attachment #8560197 - Flags: review?(bugmail.mozilla)
This makes synthesizeWheelEvent work with e10s+APZ. This event could *probably* be async since scrolls will be async anyway.
Attachment #8560199 - Flags: review?(bugs)
Comment on attachment 8560052 [details] [diff] [review]
part 1, don't double scroll

Review of attachment 8560052 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/EventStateManager.cpp
@@ +3016,5 @@
> +      // When APZ is enabled, the actual scroll animation might be handled by
> +      // the compositor.
> +      WheelPrefs::Action action = WheelPrefs::ACTION_NONE;
> +      if (!layers::APZCTreeManager::WillHandleWheelEvent(wheelEvent)) {
> +       action = WheelPrefs::GetInstance()->ComputeActionFor(wheelEvent);

indent

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +859,5 @@
>  
> +bool
> +APZCTreeManager::WillHandleWheelEvent(WidgetWheelEvent* aEvent)
> +{
> +  return EventStateManager::WheelEventIsScrollAction(aEvent) &&

add a MOZ_ASSERT(aEvent)
Comment on attachment 8560197 [details] [diff] [review]
part 2, clean up widget APZ dispatch

Review of attachment 8560197 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseWidget.cpp
@@ +953,5 @@
>    }
>  }
>  
>  nsEventStatus
> +nsBaseWidget::ProcessAPZEvent(WidgetInputEvent* aEvent,

Rename this to ProcessUntransformedAPZEvent

@@ +979,5 @@
> +{
> +  if (mAPZC) {
> +      uint64_t inputBlockId = 0;
> +      ScrollableLayerGuid guid;
> +

This file appears to have 2-space indentation but this code does not
Attachment #8560197 - Flags: review?(bugmail.mozilla) → review+
David, do you mind deferring landing this until the b2g stuff we need for 2.2 has landed? Specifically bug 930939 and bug 1127066, since they both touch some of this code and it'll be easier to uplift those to b2g37 if we don't have to rebase around this work.
Comment on attachment 8560052 [details] [diff] [review]
part 1, don't double scroll

>+APZCTreeManager::WillHandleWheelEvent(WidgetWheelEvent* aEvent)
>+{
>+  return EventStateManager::WheelEventIsScrollAction(aEvent) &&
>+         aEvent->deltaMode == nsIDOMWheelEvent::DOM_DELTA_LINE;

Not about this bug, but it is really not clear to me why only line scrolling is handled.
I'd actually expect apz to deal especially with pixel scrolling, and then possibly leave
line scrolling for ESM, since line scrolling depends on the target of the event.
(though, perhaps we know the line information somewhere in layer level too?)
Attachment #8560052 - Flags: review?(bugs) → review+
Comment on attachment 8560199 [details] [diff] [review]
part 3, e10s fix

I have no idea why prio(high).
(In other words, I don't understand what prio(high) means especially with sync)
Attachment #8560199 - Flags: review?(bugs) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> David, do you mind deferring landing this until the b2g stuff we need for
> 2.2 has landed? Specifically bug 930939 and bug 1127066, since they both
> touch some of this code and it'll be easier to uplift those to b2g37 if we
> don't have to rebase around this work.

Missed this comment - but yeah, totally.
Posted patch part 4, fix coordinates (obsolete) — Splinter Review
Make sure we do the inverse of MapEventCoordinatesToChildProcess() when events originate in the other direction.
Attachment #8562337 - Flags: review?(bugmail.mozilla)
Comment on attachment 8562337 [details] [diff] [review]
part 4, fix coordinates

Review of attachment 8562337 [details] [diff] [review]:
-----------------------------------------------------------------

Conceptually I agree this needs to be done, but I'm wondering if we can reuse more code. For one thing, RecvSynthesizedMouseWheelEvent already sets GetWidget() on the event as event.widget, so I feel like without too much trouble you should be able to use EventStateManager::GetChildProcessOffset directly (and then just negate the offset).

I might be misreading the code though, it's confusing and needs to be simplified. Does GetChildProcessOffset actually return an offset or an absolute value? It's odd because the current TabParent::MapEventCoordinatesForChildProcess assigns the offset directly to refPoint for non-touch events but then adds it to the refPoint for touch events. That's definitely bad code even if it happens to work out right, because it's relying on undocumented assumptions somewhere.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> Comment on attachment 8562337 [details] [diff] [review]
> part 4, fix coordinates
> 
> Review of attachment 8562337 [details] [diff] [review]:
> -----------------------------------------------------------------
> I might be misreading the code though, it's confusing and needs to be
> simplified. Does GetChildProcessOffset actually return an offset or an
> absolute value? It's odd because the current
> TabParent::MapEventCoordinatesForChildProcess assigns the offset directly to
> refPoint for non-touch events but then adds it to the refPoint for touch
> events. That's definitely bad code even if it happens to work out right,
> because it's relying on undocumented assumptions somewhere.

GetChildProcessOffset returns an absolute value. Yes, it's confusing, I don't really know what's going on there. What is it supposed to do?
It's supposed to convert the event's coordinates from being in the parent process's widget space to the child process's widget space. On B2G for example there's a 30 pixel difference between the two in most apps, because the status bar at the top of the screen is 30 pixels tall and lives in the parent process. On desktop with e10s there is a similar delta from the chrome part which lives in the parent process.

Conceptually based on the name of the method I would expect that function to return the offset of child process's widget from the parent process's widget, and then the caller code would just add that to whatever event coordinates needed to be adjusted. However that's not what it does - it takes an event, and returns the value of the event's refPoint plus the offset into the child process. I think the reason that it works for touch events is because the touch event's refPoint is always zero, and the individual touch points are what matter. Therefore the call to ESM::GetChildProcessOffset for a *touch* event will return just the offset, which then gets added to the individual touch points mRefPoint.

Looking around, there is also TabParent::GetChildProcessOffset which does exactly what I would expect (although it returns an nsIntPoint instead of a LayoutDeviceIntPoint, but whatever). So we can use that instead - in fact, to fix the problem you're seeing, all you should need to do is make a one-line change to RecvSynthesizedMouseWheelEvent:

localEvent.refPoint -= LayoutDeviceIntPoint::FromUntyped(GetChildProcessOffset());

Nothing else should be needed. I can file another bug to get rid of EventStateManager::GetChildProcessOffset which doesn't really return an offset, and to make all code using switch over to using TabParent::GetChildProcessOffset.
Attachment #8562337 - Flags: review?(bugmail.mozilla) → review-
The failing test creates a scrollframe as part of its setup. The initial state of a scrollframe does not allow APZ scrolling until APZ acknowledges the initial scroll offset (which in the test is 0,0). I'm not sure I really like that, but I can't think of anything else in the meantime.

This patch just changes the test to wait for all paints to be flushed.
Attachment #8563198 - Flags: review?(bugs)
Attachment #8562337 - Attachment is obsolete: true
Attachment #8563209 - Flags: review?(bugmail.mozilla)
Attachment #8563209 - Flags: review?(bugmail.mozilla) → review+
Attachment #8563198 - Flags: review?(bugs) → review+
Two more problems. The first is that on Desktop, text controls don't need focus to be scrolled. The second is that, single-line text controls can't be vertically scrolled with the mouse wheel even if their scrollport overflows the frame.

This patch sets a bit in FrameMetrics if the layer should not be vertically scrolled with the mouse wheel. APZ then treats any wheel-originated scroll in such layers as an overscroll.

This patch depends on bug 1102427, since the outer scrollable frames must be layerized for overscroll handoff to work.
Attachment #8566735 - Flags: review?(botond)
Comment on attachment 8566735 [details] [diff] [review]
part 6, fix single-line text boxes

Review of attachment 8566735 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if there's some way to roll this into the event-regions code. The behaviour here seems quite similar to doing touch-action:pan-x on the scrollable frame, except that it's more like wheel-action:pan-x since it's restricted to wheel events. Still we might be able to reuse some of that machinery to make this work. That being said that machinery isn't quite in place yet so maybe it's not worth waiting for that... I'm not really sure. Something to think about.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> I wonder if there's some way to roll this into the event-regions code. The
> behaviour here seems quite similar to doing touch-action:pan-x on the
> scrollable frame, except that it's more like wheel-action:pan-x since it's
> restricted to wheel events. Still we might be able to reuse some of that
> machinery to make this work. That being said that machinery isn't quite in
> place yet so maybe it's not worth waiting for that... I'm not really sure.
> Something to think about.

What machinery is there already? Would we be able to handoff the scroll at the APZCTM level instead of at the overscroll decision? If so, adding another region type would be a lot cleaner.

Related, I'm not sure whether having both an x and y delta should turn into a y overscroll, or whether the y portion should just be dropped. I'm guessing we should drop the y portion. My trackpad doesn't seem to generate scrolls in both directions at once though.
(In reply to David Anderson [:dvander] from comment #27)
> What machinery is there already? Would we be able to handoff the scroll at
> the APZCTM level instead of at the overscroll decision? If so, adding
> another region type would be a lot cleaner.

The machinery for touch-action:pan-x would (once it's fully functional) cause TouchBlockState::TouchActionAllowsPanningXY() to return false while the innermost APZC was processing the input event (e.g. at [1]) and effectively prevent any vertical scrolling. I guess in your case you still want to propagate that scroll to the next APZC in the handoff chain which is not the case with touch-action. So maybe it's not that similar. :(

> Related, I'm not sure whether having both an x and y delta should turn into
> a y overscroll, or whether the y portion should just be dropped. I'm
> guessing we should drop the y portion. My trackpad doesn't seem to generate
> scrolls in both directions at once though.

I would say we should do a y overscroll. Consider the case where you have some magic trackpad that allows diagonal scrolls - it might be very hard for the user to actually scroll strictly vertically. Any amount of diagonal scroll would fall into this scenario. If we drop the y portion if there's an x component the user might stuck always panning horizontally inside the input box. Not sure if I misunderstood your question though...

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=d805db38cd5f#1787
Comment on attachment 8566735 [details] [diff] [review]
part 6, fix single-line text boxes

Review of attachment 8566735 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/FrameMetrics.h
@@ +683,5 @@
>    // The value of GetLineScrollAmount(), for scroll frames.
>    LayoutDeviceIntSize mLineScrollAmount;
> +
> +  // Whether or not the frame can be vertically scrolled with a mouse wheel.
> +  bool mCannotVerticallyScrollWithWheel;

I prefer names that don't have negatives in them, e.g. mAllowVerticalWheelScroll.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +43,5 @@
> +enum ScrollSource {
> +  SCROLL_SOURCE_TOUCH,
> +  SCROLL_SOURCE_WHEEL,
> +  SCROLL_SOURCE_DOM    // scrollTo(), etc.
> +};

I'd prefer using an enum class (we can do that now!), and dropping the 'SCROLL_SOURCE_' prefix from the enumerator names.

@@ +361,5 @@
>     * Note: this should be used for panning only. For handing off overscroll for
>     *       a fling, use DispatchFling().
>     */
>    bool DispatchScroll(AsyncPanZoomController* aApzc,
> +                      ScrollSource aSource,

Make this a field of OverscrollHandoffState.

(You may have to move ScrollSource into APZUtils.h for header dependency reasons.)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +780,5 @@
> +      // allowed here.
> +      overscroll.y = displacement.y;
> +    } else {
> +      mApzc.mY.AdjustDisplacement(displacement.y, adjustedOffset.y, overscroll.y);
> +    }

Maybe extract a helper function AsyncPanZoomController::AdjustYDisplacement that can be shared between here and AttemptScroll?

@@ +1039,5 @@
>      switch (panGestureInput.mType) {
>        case PanGestureInput::PANGESTURE_MAYSTART: rv = OnPanMayBegin(panGestureInput); break;
>        case PanGestureInput::PANGESTURE_CANCELLED: rv = OnPanCancelled(panGestureInput); break;
>        case PanGestureInput::PANGESTURE_START: rv = OnPanBegin(panGestureInput); break;
> +      case PanGestureInput::PANGESTURE_PAN: rv = OnPan(panGestureInput, SCROLL_SOURCE_TOUCH, true); break;

Are we considering trackpad to be "TOUCH" here?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1097,1 @@
>  #if defined(MOZ_B2G) || defined(MOZ_WIDGET_ANDROID)

Are you perhaps missing a changeset here? This #define is not on trunk, and I don't see the hunk that introduces it in any of the previous patches on this bug.
(In reply to Botond Ballo [:botond] from comment #29)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +780,5 @@
> > +      // allowed here.
> > +      overscroll.y = displacement.y;
> > +    } else {
> > +      mApzc.mY.AdjustDisplacement(displacement.y, adjustedOffset.y, overscroll.y);
> > +    }
> 
> Maybe extract a helper function AsyncPanZoomController::AdjustYDisplacement
> that can be shared between here and AttemptScroll?
> 
> @@ +1039,5 @@
> >      switch (panGestureInput.mType) {
> >        case PanGestureInput::PANGESTURE_MAYSTART: rv = OnPanMayBegin(panGestureInput); break;
> >        case PanGestureInput::PANGESTURE_CANCELLED: rv = OnPanCancelled(panGestureInput); break;
> >        case PanGestureInput::PANGESTURE_START: rv = OnPanBegin(panGestureInput); break;
> > +      case PanGestureInput::PANGESTURE_PAN: rv = OnPan(panGestureInput, SCROLL_SOURCE_TOUCH, true); break;
> 
> Are we considering trackpad to be "TOUCH" here?

Yeah. The Mac touchpad sends these natively so we'll probably have to propagate the ScrollSource through the event itself, later on.
w/ comments addressed
Attachment #8566735 - Attachment is obsolete: true
Attachment #8566735 - Flags: review?(botond)
Attachment #8567327 - Flags: review?(botond)
Comment on attachment 8567327 [details] [diff] [review]
part 6, fix single-line text boxes v2

Review of attachment 8567327 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'd like to have Timothy's feedback about the change to nsGfxScrollFrame.cpp.

::: gfx/layers/apz/src/APZUtils.h
@@ +26,5 @@
> +enum class ScrollSource {
> +  // scrollTo() or something similar.
> +  DOM,
> +
> +  // Touch-screen.

Please add "including trackpad".

::: gfx/layers/apz/src/Axis.cpp
@@ +128,5 @@
>      aDisplacementOut = 0;
>      return false;
>    }
> +  if (forceOverscroll) {
> +    aOverscrollAmountOut = aDisplacement;

Set aDisplacementOut to zero. It so happens that all existing callers pass in a zero-valued float, but we shouldn't rely on that.

::: layout/base/nsDisplayList.cpp
@@ +761,5 @@
>      LayoutDeviceIntSize lineScrollAmountInDevPixels =
>        LayoutDeviceIntSize::FromAppUnitsRounded(lineScrollAmount, presContext->AppUnitsPerDevPixel());
>      metrics.SetLineScrollAmount(lineScrollAmountInDevPixels);
> +
> +    if (EventStateManager::CanVerticallyScrollFrameWithWheel(aScrollFrame->GetParent())) {

Can aScrollFrame->GetParent() be null? EventStateManager::CanVerticallyScrollFrameWithWheel() doesn't check.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1095,5 @@
>                      && (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN);
> +
> +#if defined(MOZ_B2G) || defined(MOZ_WIDGET_ANDROID)
> +  // Mobile platforms need focus to scroll.
> +  bool canScroll = IsFocused(mOuter->GetContent());

This variable name and comment are a bit misleading. Given how we use them, I'd make it:

  // Mobile platforms need focus to scroll without scrollbars.
  bool canScrollWithoutScrollbars = ...;

@@ +1097,5 @@
> +#if defined(MOZ_B2G) || defined(MOZ_WIDGET_ANDROID)
> +  // Mobile platforms need focus to scroll.
> +  bool canScroll = IsFocused(mOuter->GetContent());
> +#else
> +  bool canScroll = true;

I'm not sure whether input fields are the only type of scrollable element that doesn't have scrollbars. If not, we may be making too many things scrollable here. I'd like to have Timothy's opinion on this.
Attachment #8567327 - Flags: review?(botond)
Attachment #8567327 - Flags: review+
Attachment #8567327 - Flags: feedback?(tnikkel)
(In reply to Botond Ballo [:botond] from comment #32)
> I'm not sure whether input fields are the only type of scrollable element
> that doesn't have scrollbars. If not, we may be making too many things
> scrollable here. I'd like to have Timothy's opinion on this.

ScrollFrameHelper::CreateAnonymousContent is what decides whether to create scrollbar frames or not. Places where it doesn't create scrollbar frames: text controls, print/print preview, svg documents being used as images and overflow hidden. We already handle overflow hidden in WantAsyncScroll. Probably doesn't matter for print/print preview. But inside svg image documents probably we should not consider things scrollable.
Comment on attachment 8567327 [details] [diff] [review]
part 6, fix single-line text boxes v2

Assuming the svg images change is okay.
Attachment #8567327 - Flags: feedback?(tnikkel) → feedback+
Another fix since some tests are still failing. If the scroll range of a frame is less than one device pixel, we're not supposed to scroll it.
Attachment #8570374 - Flags: review?(bugmail.mozilla)
Comment on attachment 8570374 [details] [diff] [review]
bug1126090-part7.patch

Review of attachment 8570374 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane to me

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1087,5 @@
>  
>  bool
>  ScrollFrameHelper::WantAsyncScroll() const
>  {
>    nsRect scrollRange = GetScrollRange();

don't need this local variable any more
Attachment #8570374 - Flags: review?(bugmail.mozilla) → review+
Posted patch follow-up fix (obsolete) — Splinter Review
Fix failure on B2G. I don't like this very much. The local ref of mWidget->APZCTM() might be racy, if the APZCTM can be set to null on the widget.
Attachment #8571848 - Flags: review?(bugmail.mozilla)
Comment on attachment 8571848 [details] [diff] [review]
follow-up fix

Review of attachment 8571848 [details] [diff] [review]:
-----------------------------------------------------------------

I also don't like this, but mostly because you haven't even described the problem this is trying to fix.
Attachment #8571848 - Flags: review?(bugmail.mozilla) → review-
Posted patch follow-up fixSplinter Review
B2G can't dispatch APZ events from the main thread.
Attachment #8571848 - Attachment is obsolete: true
Attachment #8572096 - Flags: review?(bugmail.mozilla)
Where are the wheel events coming from? B2G doesn't generate wheel events.
Ah I see, it's a test scenario and the previous patches on the bug pump the wheel event into the B2G widget code. This is getting pretty messy, and it would be nice to fork some of the older patches on this bug into new bugs and land them. Lemme think about this wheel event codepath though.
Comment on attachment 8572096 [details] [diff] [review]
follow-up fix

Review of attachment 8572096 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC I'd rather disable the test on B2G, as wheel input isn't a supported input method on B2G anyway.
Attachment #8572096 - Flags: review?(bugmail.mozilla)
I think this bug broke scrolling completely on the 08/03 nightly. Scrolling is extremely slow with APZ enabled. going back to 07/03 fixes it
No longer depends on: 1140903
Depends on: 1193781
No longer depends on: 1193781
You need to log in before you can comment on or make changes to this bug.