1242690 - Mouse events don't get untransformed by the resolution, if there is one

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

We have a TODO in the code [1] about whether we should run non-touch events through the callback transform. The answer is "yes" for some scenarios. A easily reproducible scenario is if you (on desktop) set apz.allow_zooming and dom.meta-viewport.enabled to true, and restart the browser, you'll see that the mouse hover effect is not in sync with the mouse pointer position (easily visible on about:home, with the icons along the bottom). The reason for this is that a resolution is in effect and the mouse events don't have the resolution unapplied properly. This is impacting real scenarios in bug 1241491 (this bug is really just a generic version of that one, because I don't want to land these patches as part of a confidential bug when they don't need to be secret). This will probably affect things like mouse support on Fennec with APZ as well.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2256ee8ae9a8ee0bf7ab49a8b1924720d846cc7/widget/nsBaseWidget.cpp#1021

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Oh, this might also affect mouse clicking on desktop in cases where there are other things in the callback transform. Usually that's just some fractional scroll delta but there might be cases where it's significant, I'm not sure.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df61697f34cd

Comment 3

[Kartikaya Gupta (email:[e-mail])]

New try push with an extra patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fb7a4ca4f1b

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1242690 - Ensure that mouse events have the callback transform applied. r=

Review commit: https://reviewboard.mozilla.org/r/32613/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32613/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1242690 - Squash together DispatchAPZAwareEvent and DispatchInputEvent. r=

Review commit: https://reviewboard.mozilla.org/r/32615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32615/

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1242690 - Make sure that synthetic mouse events have a reasonable guid so that the callback transform can get unapplied properly. r=

Review commit: https://reviewboard.mozilla.org/r/32617/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32617/

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Hi Kats,

I tested these patch on Linux.
Great, the mismatched hover-effect seems fixed.
But there are still 2 weird behaviors, please see video (https://goo.gl/x3YrQn)

The text-dragging-effect is not stick with cursor when the web page does NOT include meta-viewport, i.e nightly landing page.  It behaves well on http://www.debian.org/ which meta-viewport tag.
Also, the hover position seems not well calculated on tiles page.

Any thought ?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Thanks. I'll leave these patches up for review and continue investigating. There's likely more events that need to get routed through the APZCCallbackHelper code and it's just a matter of tracking them down.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8712683 [details]
MozReview Request: Bug 1242690 - Squash together DispatchAPZAwareEvent and DispatchInputEvent. r=

https://reviewboard.mozilla.org/r/32615/#review29591

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8712682 [details]
MozReview Request: Bug 1242690 - Ensure that mouse events have the callback transform applied. r=

https://reviewboard.mozilla.org/r/32613/#review29683

As discussed in person, calling DispatchAPZAwareEvent for mouse events means that, in addition to having the callback transform applied, APZEventState::ProcessMouseEvent will be called. As APZ doesn't currently create input blocks for mouse events, this will result in a (harmless but useless) ContentReceivedInputBlock message with input block id 0, which we can avoid by adding a guard in ProcessUntransformedAPZEvent.

::: dom/ipc/TabChild.cpp:1814
(Diff revision 1)
>  TabChild::RecvRealMouseButtonEvent(const WidgetMouseEvent& event,

s/event/aEvent while you're touching this function

::: gfx/layers/apz/util/APZCCallbackHelper.h:104
(Diff revision 1)
> -    static void ApplyCallbackTransform(WidgetTouchEvent& aEvent,
> +    static void ApplyCallbackTransform(WidgetEvent* aEvent,

Why is this changing from a reference to a pointer? I'd prefer keeping it a reference.

::: widget/nsBaseWidget.cpp:1070
(Diff revision 1)
> -  if (mAPZC) {
> +  return DispatchAPZAwareEvent(aEvent);

There is a comment at the beginning of APZCTreeManager::ReceiveInputEvent that says:

  // This function will be removed once metro code is modified to use the
  // InputData version of ReceiveInputEvent.
  // In general it is preferable to use the version of ReceiveInputEvent
  // that takes an InputData, as that is usable from off-main-thread.

I'm getting the impression this is no longer the case. Could you update the comment?

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8712684 [details]
MozReview Request: Bug 1242690 - Make sure that synthetic mouse events have a reasonable guid so that the callback transform can get unapplied properly. r=

https://reviewboard.mozilla.org/r/32617/#review29685

::: layout/base/nsPresShell.h:782
(Diff revision 1)
> +  // This is an APZ state variable that goes with mMouseLocation and is needed

Please clarify that by "goes with mMouseLocation" you mean that this is the target guid of the last native mouse event that was processed.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #10)
> As discussed in person, calling DispatchAPZAwareEvent for mouse events means
> that, in addition to having the callback transform applied,
> APZEventState::ProcessMouseEvent will be called. As APZ doesn't currently
> create input blocks for mouse events, this will result in a (harmless but
> useless) ContentReceivedInputBlock message with input block id 0, which we
> can avoid by adding a guard in ProcessUntransformedAPZEvent.

Done

> ::: dom/ipc/TabChild.cpp:1814
> s/event/aEvent while you're touching this function

Done. Also did it for a couple of nearby functions that just delegate to this one. I also renamed a "event" to "localEvent" for consistency, it was bugging me.

> Why is this changing from a reference to a pointer? I'd prefer keeping it a
> reference.

Changed this back. There wasn't a good reason for that change.

> ::: widget/nsBaseWidget.cpp:1070
> There is a comment at the beginning of APZCTreeManager::ReceiveInputEvent
> that says:
> 
>   // This function will be removed once metro code is modified to use the
>   // InputData version of ReceiveInputEvent.
>   // In general it is preferable to use the version of ReceiveInputEvent
>   // that takes an InputData, as that is usable from off-main-thread.
> 
> I'm getting the impression this is no longer the case. Could you update the
> comment?

Updated the comment. Half of it is still true.

(In reply to Botond Ballo [:botond] from comment #11)
> Please clarify that by "goes with mMouseLocation" you mean that this is the
> target guid of the last native mouse event that was processed.

Done

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I'll land these patches once the tree is open; I filed bug 1244284 for follow-up work for the dragging.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfc2b6267d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/581cc23fcaac
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ccda47f18b3

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5cfc2b6267d2
https://hg.mozilla.org/mozilla-central/rev/581cc23fcaac
https://hg.mozilla.org/mozilla-central/rev/6ccda47f18b3

Comment 16

[Kartikaya Gupta (email:[e-mail])]

I backed out these patches for causing bug 1244549 which seems like a pretty bad regression.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a49bae84297c

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Backout cset on m-c: https://hg.mozilla.org/mozilla-central/rev/a49bae84297c

Comment 20

[Randall Barker [:rbarker]]

https://reviewboard.mozilla.org/r/32613/#review32337

::: dom/ipc/TabChild.cpp:1824
(Diff revision 1)
> +      mPuppetWidget->GetDefaultScale());

This line of code appears to be the cause of Bug 1244549. When I comment this line out, the scroll thumb behaves as expected.

Comment 21

[Randall Barker [:rbarker]]

After further investigation here is my hypothesis. While scrolling with the thumb with APZ enabled, Gecko and APZ scroll positions get out of sync as I would expect. This causes APZCCallbackHelper::ApplyCallbackTransform to transform the mouse position for Gecko. However, when scrolling with the scroll thumb we do not want to apply the callback transform as we actually need to know where the mouse actually is on the screen. The jittering is caused because as we scroll, the mouse position is adjusted and for the thumb, it looks like the mouse is jumping back and forth between where it actually is on the screen when Gecko and APZ are in sync and then where it is transformed to when they are not in sync.

Comment 22

[Randall Barker [:rbarker]]

My hypothesis was mostly wrong. From IRC:

<kats> i can explain what's happening and what i think the root cause is. i'm not sure if i'm right
<kats> so let's say the page is scrolled to the top, y=0
<kats> the user does a mousedown on the scrollthumb and starts dragging
<kats> say they drag it so that the page scrolls by 10 pixels
<kats> so the way this works, is the nsSliderFrame or something will adjust the main-thread scroll position to y=10
<kats> and that will send a repaint to APZ saying "yo, the new scroll offset is y=10"
<kats> so far so good. now the APZ sends a repaint request back to the main thread, with y=10 as the scroll offset
<kats> but before that repaint request gets processed, the user moves the mouse again, so that the scrollthumb triggers a main-thread scroll change to y=20
<kats> so now when the code at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp#129 runs to handle the repaint request, it will fail to change the main-thread scroll position (and rightly so, since that takes precedence over APZ)
<kats> so after that call to ScrollFrameTo you'd see something like apzScrollOffset = 10, actualScrollOffset = 20, scrollUpdated = false
<kats> and that will set a callbackTransform of -10 at the bottom of that function
<kats> once the callbacktransform is in place, all hell breaks loose, because subsequent mouse events will get that applied
<kats> so the next mousemove the user does will have an extra -10 pixels applied to the position, which will cause the scrollthumb to jump and change the scroll position even more
<kats> and then this oscillates and goes haywire
<kats> so now what i'm wondering is if we can separate the notion of the scroll position of the *content* with the notion of the position of the *thumb*
<kats> that is, when the user is moving their mouse over the thumb, it doesn't matter what the callbackTransform is on the *content*
<kats> and we shouldn't be using that callbackTransform if the mouse is over the thumb, or otherwise targeting the thumb

Comment 23

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1242690-Hack-to-fix-scrollbar-dragging-with-APZ-16022215-efc3dd5.patch

This patch fixes the scroll bar dragging issue. Probably need to find a better solution. At least it verifies the issue.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Yeah I was thinking of something along the same lines but I agree it's not very elegant, and might not be the right layer to fix the issue.

If we take another step back - the guid that is being passed in to RecvRealMouseButtonEvent is that of the root content, when perhaps it shouldn't be. After all the user is dragging on the scrollbar thumb and not the content, so maybe we need to change the hit-test code in APZC so that events over the scrollbar area don't end up using the content APZC as the target?

Comment 25

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> Yeah I was thinking of something along the same lines but I agree it's not
> very elegant, and might not be the right layer to fix the issue.
> 
> If we take another step back - the guid that is being passed in to
> RecvRealMouseButtonEvent is that of the root content, when perhaps it
> shouldn't be. After all the user is dragging on the scrollbar thumb and not
> the content, so maybe we need to change the hit-test code in APZC so that
> events over the scrollbar area don't end up using the content APZC as the
> target?

As you pointed out, after the drag has started, the mouse doesn't need to stay over the scrollbar thumb. So I'm not sure how hit testing could ever be made to work?

Comment 26

[Kartikaya Gupta (email:[e-mail])]

In the APZ all the mouse events corresponding to a drag should be part of the same input block, which means they should share the same target. So the existing machinery in APZC should take care of that (a similar thing happens with touch-dragging, if you're dragging a subframe and then move your finger outside the subframe into the parent frame).

Comment 27

[Josh Cheng [:josh]]

[Blocking Requested - why for this release]:

Hi Randall,
Thanks for helping this issue and it has been an blocker for Smart TV content developer and also our partner. I just want to make sure we are not blocked here for better solution from your comment 23?

Thanks

Comment 28

[Kartikaya Gupta (email:[e-mail])]

I don't think we can land the fix in comment 23 for all platforms. If you guys can create a #define for the TV platform we could land something behind that but since you don't seem to we have to find a solution that works for all platforms which is much harder. I'm going to spend some time today thinking about what to do here.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

I've been working on this today, and came up with a set of patches that is closer to what I want. They fix the problem here, but I think I can improve them a bit more. I've pushed them to [1] (top 3 patches). The main thing I want to change is the condition at [2] so that it's more specific to scrollbar. Still thinking through it.

[1] https://github.com/staktrace/gecko-dev/commits/transform
[2] https://github.com/staktrace/gecko-dev/commit/dae9111abb883d16f1aaa56fee24305b46dc63ad#diff-bc1d379114e9aa4991537e776c8f8271R696

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Spawn a new drag block if a drag continues after it's block is interrupted

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Enable creation of drag blocks even if apz.drag.enabled is false by moving the pref

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 6 - Further refine based on mouse event targeting scrollbar or not

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 7 - Prevent main-thread callback transform on scrollbar-targeted events

So with these patches all the scenarios I can think of work properly for the non-TV case. For the TV case (i.e. when there is a resolution applied) things work properly in cases where width=device-width but there are scrollbar clipping and dragging issues otherwise, which I can look into as a followup if needed.

I tested with apz.drag.enabled both true and false so as to minimize regressions with APZ scrollbar dragging.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

I'm going to wait until after the merge to land these, because if something happens i don't want to have to back it out of 47 on aurora. In the meantime I'll work on the follow up bugs to get other events untransformed properly.

Comment 35

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@kilik will help test these patches on both TV simulator and real TV.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

I also did a try push with these patches at [1]. (The patches aren't visible in the TH pushlog because I did a previous try push with them, but they're in there).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=57c88ea72665&group_state=expanded

Comment 37

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #35)
> @kilik will help test these patches on both TV simulator and real TV.

Sorry, I'll be out of office on Monday too, and will post the test result on Tue.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

How easy is it to have tests for this?

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Umm not horrible but will take some time. Easiest is probably to run some of our existing tests with resolution applied.

Comment 40

[Kilik Kuo [:kikuo] (inactive)]

Hi Kats,

I had a test on browser on linux with these patches introduced.
There are 3 issues I noticed.
1. Text-dragging effect is mismatched with the cursor. (Should be the same issue which Jeff is looking into).
2. On nightly tiled page, hit-test is still incorrect on each tile. The hit-test result also varies with different browser window size.
3. Thumb of scrollbar is missing (misplaced or not rendered ?)

Regarding No.1 & No.2, it seems not to be serious on TV.
I'll check No.3 on real TV because classic-scrollbar is currently enabled on it.

Comment 41

[Kartikaya Gupta (email:[e-mail])]

I noticed (3) as well when I was testing locally, but I didn't think it would have been caused by my patches. I think that's a clipping issue from somewhere else. (2) surprises me as I'm pretty sure I fixed it. I'll test again though.

Comment 42

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #41)
> I noticed (3) as well when I was testing locally, but I didn't think it
> would have been caused by my patches. I think that's a clipping issue from
> somewhere else. (2) surprises me as I'm pretty sure I fixed it. I'll test
> again though.

Should add, if i save the tile-page and open it on browser locally, the hit-test works perfect.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Thanks. That likely means the e10s/multiprocess path works fine but the parent process is still broken.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Ah I can reproduce the issues on the about:newtab page. Previously I was testing on about:home which works fine with these patches. I'll see if I can figure out what's going on there.

Comment 45

[Kartikaya Gupta (email:[e-mail])]

After spending half the day on this I'm still no closer to figuring out what's going on here. I'm going to try some more things but I'm once the trees reopen I'm going to land these patches for now since randall needs them for some Fennec work, and they don't appear to regress any existing behaviour.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bc4a98a12c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb01b42228e
https://hg.mozilla.org/integration/mozilla-inbound/rev/415cd146145d
https://hg.mozilla.org/integration/mozilla-inbound/rev/26bc425fa2a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c37fd20f04d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd968c710a39
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d931aacb061

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5bc4a98a12c
https://hg.mozilla.org/mozilla-central/rev/9cb01b42228e
https://hg.mozilla.org/mozilla-central/rev/415cd146145d
https://hg.mozilla.org/mozilla-central/rev/26bc425fa2a9
https://hg.mozilla.org/mozilla-central/rev/6c37fd20f04d
https://hg.mozilla.org/mozilla-central/rev/fd968c710a39
https://hg.mozilla.org/mozilla-central/rev/1d931aacb061

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Since leave-open bugs always come back to haunt me later, I'm going to close this one and file a follow-up for the remaining issue.

Comment 49

[Kilik Kuo [:kikuo] (inactive)]

Hi Kats,

I've tested these on browser(gecko44), no regressions & this issue is fixed, so I'd like to ask for uplift to gecko44.
Do you mind that I attach patches directly from https://github.com/staktrace/gecko-dev/commits/transform-v25 and ask for a? without getting r+ (I don't think it's necessary because you already rebased and compiled it, and I tested).  Or you would like to do it by yourself ?

And I'm doing the same request on Bug 1211612, not sure whether this is ok or not.

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Attachment: Rollup (includes all 7 patches rebased to b2g44)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Required to fix bug 1241491
User impact if declined: hit-testing is wrong if meta viewport is enabled on B2G TV platforms
Testing completed: by Kilik, see comment above
Risk to taking this patch (and alternatives if risky): it's a large set of changes and rebasing back that many versions is always kind of risky.
String or UUID changes made by this patch: none

Comment 51

[Josh Cheng [:josh]]

Comment on attachment 8733864 [details] [diff] [review]
Rollup (includes all 7 patches rebased to b2g44)

Approve for TV 2.5

Comment 52

[Gary Chen [:xeonchen]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/373f76c006f2

Comment 53

[Josh Cheng [:josh]]

Hi Gary,
Please also help to mark "status-b2g-v2.5:fixed" in tracking b2g flag. Thanks!