Closed Bug 1242690 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
blocking-b2g 2.5+
Tracking Status
firefox48 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 3 open bugs)

Details

(Keywords: correctness, Whiteboard: [ft:conndevices][gfx-noted])

Attachments

(8 files, 1 obsolete file)

58 bytes, text/x-review-board-request
botond
: review+
Details
58 bytes, text/x-review-board-request
dvander
: review+
Details
58 bytes, text/x-review-board-request
botond
: review+
Details
6.78 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
7.89 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
14.62 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
1.51 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
58.69 KB, patch
kats
: review+
Details | Diff | Splinter Review
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
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.
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 ?
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Attachment #8712683 - Flags: review?(dvander) → review+
Comment on attachment 8712683 [details]
MozReview Request: Bug 1242690 - Squash together DispatchAPZAwareEvent and DispatchInputEvent. r=

https://reviewboard.mozilla.org/r/32615/#review29591
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?
Attachment #8712682 - Flags: review?(botond) → review+
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.
Attachment #8712684 - Flags: review?(botond) → review+
(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
I'll land these patches once the tree is open; I filed bug 1244284 for follow-up work for the dragging.
Depends on: 1244549
I backed out these patches for causing bug 1244549 which seems like a pretty bad regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 1241491
blocking-b2g: --- → 2.5+
Whiteboard: [ft:conndevices]
See Also: → 1241491
blocking-b2g: 2.5+ → ---
Keywords: correctness
Whiteboard: [ft:conndevices] → [ft:conndevices][gfx-noted]
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.
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.
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
This patch fixes the scroll bar dragging issue. Probably need to find a better solution. At least it verifies the issue.
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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).
[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
blocking-b2g: --- → 2.5?
Flags: needinfo?(rbarker)
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.
Flags: needinfo?(rbarker)
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
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.
Attachment #8726440 - Flags: review?(rbarker)
Attachment #8722220 - Attachment is obsolete: true
Attachment #8726435 - Flags: review?(rbarker) → review+
Attachment #8726436 - Flags: review?(rbarker) → review+
Attachment #8726438 - Flags: review?(rbarker) → review+
Attachment #8726440 - Flags: review?(rbarker) → review+
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.
@kilik will help test these patches on both TV simulator and real TV.
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
(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.
How easy is it to have tests for this?
Umm not horrible but will take some time. Easiest is probably to run some of our existing tests with resolution applied.
blocking-b2g: 2.5? → 2.5+
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Thanks. That likely means the e10s/multiprocess path works fine but the parent process is still broken.
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.
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.
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.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla47 → mozilla48
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.
Flags: needinfo?(bugmail.mozilla)
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
Flags: needinfo?(bugmail.mozilla)
Attachment #8733864 - Flags: review+
Attachment #8733864 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8733864 [details] [diff] [review]
Rollup (includes all 7 patches rebased to b2g44)

Approve for TV 2.5
Attachment #8733864 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Hi Gary,
Please also help to mark "status-b2g-v2.5:fixed" in tracking b2g flag. Thanks!
Depends on: 1259781
Depends on: 1265983
Depends on: 1269067
Depends on: 1276122
Depends on: 1368866
No longer blocks: desktop-zoom
You need to log in before you can comment on or make changes to this bug.