Ensure we trigger a repaint when doing two-finger touch scrolling with zooming disabled

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kats, Assigned: gabrielle.singhcadieux, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted][good first bug][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

From bug 1345355 comment 77 and comment 79:

If the user is doing two-finger scrolling, that works by calling AsyncPanZoomController::OnScale and shifting the focus, which triggers calls to ScrollBy. If zooming is disabled, though, doScale will be false at [1]. In this scenario we never trigger a repaint request while executing this function, even though we are scrolling. We should fix that, probably by adding an else clause and triggering a repaint there. We don't want to do it every time if we *are* scaling, per [2].

[1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/gfx/layers/apz/src/AsyncPanZoomController.cpp#1415
[2] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/gfx/layers/apz/src/AsyncPanZoomController.cpp#1431
Priority: -- → P3
Whiteboard: [gfx-noted][good first bug]
Mentor: botond
Whiteboard: [gfx-noted][good first bug] → [gfx-noted][good first bug][lang=c++]
Assignee

Comment 1

2 years ago
I would like to work on this bug as part of a university assignment!
(In reply to gabrielle.singhcadieux from comment #1)
> I would like to work on this bug as part of a university assignment!

Sounds good, thanks for your interest!

The first step is to build the Firefox codebase as described here [1]. Once you've done that, let me know, and I'll write some more details about the actual code change we'll be making.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Assignee

Comment 3

2 years ago
The build is done.
Great! I've assigned the bug to you.

To provide a bit of context for the bug being fixed here: it involves AsyncPanZoomController ("APZ" for short), which is the component of Firefox that handles input events related to scrolling and zooming. APZ runs on a separate thread (and in some configurations, separate process) from the browser's main loop ("main thread") that handles interaction with the page's DOM and layout of the page. Whenever APZ performs scrolling or zooming, it needs to eventually notify the main thread so that it can send "scroll" events to the page and run any listeners that are registered for them (in case of scrolling), or re-rasterize the page at a new resolution appropriate to the new zoom level (in case of zooming). These notifications are called "repaint requests".

AsyncPanZoomController::OnScale() is the method that's called while a pinch gesture is in progress. Pinch gestures are touch gestures where there are two fingers on the screen. They usually cause zooming (by the fingers moving together or apart), but they can also cause scrolling, by the midpoint of the two fingers (called the gesture's "focus point") moving.

On some pages and in some configurations, zooming is not allowed, but pinch gestures are still processed, they're just limited to causing scrolling. The bug is that, in these configurations, APZ does not send repaint requests to notify the main thread about scrolling.

Kats already described the code change required to fix this bug in comment 0. The only thing I have to add is that the function to trigger the required repaint request is AsyncPanZoomController::RequestContentRepaint() [1].

I think that's enough to get you started. Please feel free to ask questions if you have any! Otherwise, once you make the code change, test that it builds, and if it does upload it here for review. Thanks!

[1] http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/gfx/layers/apz/src/AsyncPanZoomController.h#600
Assignee: nobody → gabrielle.singhcadieux
Assignee

Comment 5

2 years ago
Thank you for the detailed explanation!

I have two questions before I start writing code:
(1) comment 0 suggests adding an else clause and triggering a repaint there. However, it looks like a lot of code outside the if-block is zoom-specific. Would it be alright for me to separate the code in `AsyncPanZoomController::OnScale` into zoom-specific and scroll-specific code (i.e. compute `doScale` earlier, and move some code into and out of that if-block)?

(2) Is there a more specific way to check whether the code works, other than seeing if the build is successful? Are there any existing tests of "scroll" event listeners?
Flags: needinfo?(botond)
(In reply to gabrielle.singhcadieux from comment #5)
> I have two questions before I start writing code:
> (1) comment 0 suggests adding an else clause and triggering a repaint there.
> However, it looks like a lot of code outside the if-block is zoom-specific.
> Would it be alright for me to separate the code in
> `AsyncPanZoomController::OnScale` into zoom-specific and scroll-specific
> code (i.e. compute `doScale` earlier, and move some code into and out of
> that if-block)?

It's not clear to me what code could move out of or into that if block. If you have specific code to move in mind, feel free to run it by me, either by describing it, or just preparing a patch that moves it and posting it here.

> (2) Is there a more specific way to check whether the code works, other than
> seeing if the build is successful? Are there any existing tests of "scroll"
> event listeners?

We have a suite of GTests for testing AsyncPanZoomController. The components it interacts with (like the main thread) are mocked out, but you can test things like "check that RequestContentRepaint was called X number of times" (using GTest's "EXPECT_CALL" facility).

If you're up for it, you're welcome to write a new GTest for this change! The existing tests related to pinching are located in TestPinching.cpp [1]. You can have a look at some of the existing tests to give you an idea of how to write one for this change.

[1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/gfx/layers/apz/test/gtest/TestPinching.cpp
Flags: needinfo?(botond)
Assignee

Comment 7

2 years ago
Posted patch 1354185.patch (obsolete) — Splinter Review
I've added both the bugfix and a test thereof. However, I'm not sure how to filter the GTests to run just the APZ-related tests?
Attachment #8865081 - Flags: review?(botond)
(In reply to gabrielle.singhcadieux from comment #7)
> I've added both the bugfix and a test thereof. However, I'm not sure how to
> filter the GTests to run just the APZ-related tests?

You can run 

  ./mach gtest <FixtureName>.<TestName>

to run a specific test (for example, APZCPinchTester.Panning_TwoFinger_ZoomDisabled), and

  ./mach gtest 'APZ*'

to run all APZ-related tests.
Comment on attachment 8865081 [details] [diff] [review]
1354185.patch

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

Have you tried building this patch with |mach build|? I get some compiler errors when I try to build it. The first one is about the test having the same name as a previous test in the file.
Attachment #8865081 - Flags: review?(botond)
Assignee

Comment 10

2 years ago
`mach build` takes about 2 hours on my system, so I've been using (In reply to Botond Ballo [:botond] from comment #9)
> Comment on attachment 8865081 [details] [diff] [review]
> 1354185.patch
> 
> Review of attachment 8865081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have you tried building this patch with |mach build|? I get some compiler
> errors when I try to build it. The first one is about the test having the
> same name as a previous test in the file.

`mach build` takes about 2 hours on my system, so I've been using `mach build faster`, which does succeed. Would it be better to use `mach build`?

I fixed the naming issue, and the test now succeeds when I run it. However, I'm not sure whether I've named the test correctly because I don't fully understand the naming system used by the other pinching tests?
Assignee

Comment 11

2 years ago
Posted patch 1354185_v2.patch (obsolete) — Splinter Review
Attachment #8865081 - Attachment is obsolete: true
Attachment #8865158 - Flags: review?(botond)
(In reply to gabrielle.singhcadieux from comment #10)
> > Have you tried building this patch with |mach build|? I get some compiler
> > errors when I try to build it. The first one is about the test having the
> > same name as a previous test in the file.
> 
> `mach build` takes about 2 hours on my system, 

That's only the case the first time you build. After that, subsequent builds will be incremental, meaning only the files th at you have changed will be rebuilt, which will be much faster. For this patch, a rebuild shouldn't take more than a few minutes.

> so I've been using `mach
> build faster`, which does succeed. Would it be better to use `mach build`?

This is the first time I hear of |mach build faster| :)

I ran |mach help build| to see what the documentation says about it:

  """
  There are a few special targets that can be used to perform a partial
  build faster than what `mach build` would perform:

  * binaries - compiles and links all C/C++ sources and produces shared
    libraries and executables (binaries).

  * faster - builds JavaScript, XUL, CSS, etc files.
  """

So |mach build faster| does not build C++ files, which is problematic for this patch because it modifies C++ code.

However, |mach build binaries| would be valid (and slightly faster than a plain |mach build|) for this patch, so feel free to use that.

> I fixed the naming issue, and the test now succeeds when I run it. However,
> I'm not sure whether I've named the test correctly because I don't fully
> understand the naming system used by the other pinching tests?

There are no strict rules when it comes to naming the tests - just use a name that describes the scenario being tested. 

When fixing a bug, I sometimes find it useful to add the bug number to the test's name as well (example: [1]).

[1] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp#239
Comment on attachment 8865158 [details] [diff] [review]
1354185_v2.patch

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

Thanks!

The fix looks good.

The test also generally looks good. It builds, and passes with the fix.

However, the test also passes without the fix, indicating that it's not really testing what we want it to test.

The reason the test is passing without the fix is that AsyncPanZoomController::OnScaleEnd() also calls RequestContentRepaint [1], so regardless of whether or not we call RequestContentRepaint() in OnScale(), it will be called at least once when processing a complete pinch gesture.

(The reason it's not called *twice* with your fix in place, is that we have de-duplication logic [2] that ensures that if two consecutive repaint requests would send the exact same information, the second one is not sent.)

One way to fix this would be to not send a complete pinch gesture in the test. That is, instead of calling PinchWithPinchInput(), copy part of its implementation [3] so that it only sends the PINCHGESTURE_START and PINCHGESTURE_SCALE events, but not the PINCHGESTURE_END event. That way, the test will not trigger a call to OnScaleEnd(), and we will get the desired behaviour of getting 0 calls to RequestContentRepaint() without your fix, and 1 call with it.

[1] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/src/AsyncPanZoomController.cpp#1458
[2] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/src/AsyncPanZoomController.cpp#3027
[3] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/test/gtest/InputUtils.h#118
Attachment #8865158 - Flags: review?(botond) → feedback+
Assignee

Comment 14

2 years ago
(In reply to Botond Ballo [:botond] from comment #13)
> Comment on attachment 8865158 [details] [diff] [review]
> 1354185_v2.patch
> 
> Review of attachment 8865158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> The fix looks good.
> 
> The test also generally looks good. It builds, and passes with the fix.
> 
> However, the test also passes without the fix, indicating that it's not
> really testing what we want it to test.
> 
> The reason the test is passing without the fix is that
> AsyncPanZoomController::OnScaleEnd() also calls RequestContentRepaint [1],
> so regardless of whether or not we call RequestContentRepaint() in
> OnScale(), it will be called at least once when processing a complete pinch
> gesture.
> 
> (The reason it's not called *twice* with your fix in place, is that we have
> de-duplication logic [2] that ensures that if two consecutive repaint
> requests would send the exact same information, the second one is not sent.)
> 
> One way to fix this would be to not send a complete pinch gesture in the
> test. That is, instead of calling PinchWithPinchInput(), copy part of its
> implementation [3] so that it only sends the PINCHGESTURE_START and
> PINCHGESTURE_SCALE events, but not the PINCHGESTURE_END event. That way, the
> test will not trigger a call to OnScaleEnd(), and we will get the desired
> behaviour of getting 0 calls to RequestContentRepaint() without your fix,
> and 1 call with it.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#1458
> [2]
> http://searchfox.org/mozilla-central/rev/
> 6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#3027
> [3]
> http://searchfox.org/mozilla-central/rev/
> 6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/layers/apz/test/gtest/
> InputUtils.h#118

Okay, I've attempted that! It builds successfully with `mach build binaries`; and the test succeeds with the fix, and fails without it.

However, out of curiosity, why is this fix necessary if we are sending a repaint request in AsyncPanZoomController::OnScaleEnd(), even when scrolling?
Assignee

Comment 15

2 years ago
Posted patch 1354185_v3.patch (obsolete) — Splinter Review
Attachment #8865158 - Attachment is obsolete: true
Attachment #8865174 - Flags: review?(botond)
(In reply to gabrielle.singhcadieux from comment #14)
> However, out of curiosity, why is this fix necessary if we are sending a
> repaint request in AsyncPanZoomController::OnScaleEnd(), even when scrolling?

Sometimes, pages have visual effects linked to scrolling that are implemented in JavaScript by registering "scroll" event listeners. Firing "scroll" events (and thus triggering execution of these listeners) is one of the things repaint requests do.

In cases like this, the more time that elapses between scrolling visually (which is done by APZ) and firing the "scroll" event, the more out-of-sync the scroll-linked effect will be with scrolling. Sending a repaint request in OnScale() minimizes this lag.
Comment on attachment 8865174 [details] [diff] [review]
1354185_v3.patch

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

Thanks! This looks good, just one nit below:

I noticed something weird when running it, though. If I run the test without your fix, I see this output:

Actual function call count doesn't match EXPECT_CALL(*mcc, RequestContentRepaint(_))...
         Expected: to be called once
         Actual: never called - unsatisfied and active

which is expected, it indicates the test is working - but the test still passes. Are you seeing this as well?

::: gfx/layers/apz/test/gtest/TestPinching.cpp
@@ +250,5 @@
> +  // Send only the PINCHGESTURE_START and PINCHGESTURE_SCALE events,
> +  // in order to trigger a call to AsyncPanZoomController::OnScale
> +  // but not to AsyncPanZoomController::OnScaleEnd.
> +  const ScreenIntPoint& aFocus = ScreenIntPoint(250, 350);
> +  const ScreenIntPoint& aSecondFocus = ScreenIntPoint(200, 300);

nit: These can more simply be written as:

ScreenIntPoint aFocus(250, 350);
ScreenIntPoint aSecondFocus(200, 300);
Attachment #8865174 - Flags: review?(botond) → feedback+
(In reply to Botond Ballo [:botond] from comment #17)
> I noticed something weird when running it, though. If I run the test without
> your fix, I see this output:
> 
> Actual function call count doesn't match EXPECT_CALL(*mcc,
> RequestContentRepaint(_))...
>          Expected: to be called once
>          Actual: never called - unsatisfied and active
> 
> which is expected, it indicates the test is working - but the test still
> passes. Are you seeing this as well?

This is not specific to this test. It looks like, if you take any EXPECT_CALL line in an APZ GTest, alter the expected count to be something different, and run the test, you get output about the actual number of calls being different than expected, but it doesn't make the test fail.

Kats, have you come across this behaviour? Is this perhaps a recent regression in the GTest harness?
Flags: needinfo?(bugmail)
I haven't seen this behaviour, but I haven't run gtests locally in a few months at least so it's quite possible that it's a regression that I haven't encountered.
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> I haven't seen this behaviour, but I haven't run gtests locally in a few
> months at least so it's quite possible that it's a regression that I haven't
> encountered.

I filed bug 1362889 about it.

Meanwhile, there is no need to hold up this patch over that bug, we should go ahead with it.
Assignee

Comment 21

2 years ago
(In reply to Botond Ballo [:botond] from comment #18)
> (In reply to Botond Ballo [:botond] from comment #17)
> > I noticed something weird when running it, though. If I run the test without
> > your fix, I see this output:
> > 
> > Actual function call count doesn't match EXPECT_CALL(*mcc,
> > RequestContentRepaint(_))...
> >          Expected: to be called once
> >          Actual: never called - unsatisfied and active
> > 
> > which is expected, it indicates the test is working - but the test still
> > passes. Are you seeing this as well?
> 
> This is not specific to this test. It looks like, if you take any
> EXPECT_CALL line in an APZ GTest, alter the expected count to be something
> different, and run the test, you get output about the actual number of calls
> being different than expected, but it doesn't make the test fail.
> 
> Kats, have you come across this behaviour? Is this perhaps a recent
> regression in the GTest harness?

I specifically see the following output:

[----------] Global test environment tear-down
[==========] 86 tests from 12 test cases ran. (1048 ms total)
[  PASSED  ] 86 tests.
/home/guest/src/mozilla-central/gfx/layers/apz/test/gtest/TestPinching.cpp:248: Failure
Actual function call count doesn't match EXPECT_CALL(*mcc, RequestContentRepaint(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active

So the test is a failure, but not counted as such?
Assignee

Comment 22

2 years ago
I have fixed the code according to your suggestion, thank you! I am still new to C++.

Should I remove the test from this patch, given the bug w.r.t. EXPECT_CALL?
Attachment #8865174 - Attachment is obsolete: true
Attachment #8865283 - Flags: review?(botond)
(In reply to gabrielle.singhcadieux from comment #21)
> I specifically see the following output:
> 
> [----------] Global test environment tear-down
> [==========] 86 tests from 12 test cases ran. (1048 ms total)
> [  PASSED  ] 86 tests.
> /home/guest/src/mozilla-central/gfx/layers/apz/test/gtest/TestPinching.cpp:
> 248: Failure
> Actual function call count doesn't match EXPECT_CALL(*mcc,
> RequestContentRepaint(_))...
>          Expected: to be called once
>            Actual: never called - unsatisfied and active
> 
> So the test is a failure, but not counted as such?

Yes. By contrast, a real failure (that you might get by e.g. an EXPECT_EQ failing) looks like this:

[----------] Global test environment tear-down
[==========] 86 tests from 12 test cases ran. (59 ms total)
[  PASSED  ] 85 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] APZCTreeManagerTester.WheelInterruptedByMouseDrag
Comment on attachment 8865283 [details] [diff] [review]
1354185_v4.patch

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

Looks good, thanks!
Attachment #8865283 - Flags: review?(botond) → review+
(In reply to gabrielle.singhcadieux from comment #22)
> Should I remove the test from this patch, given the bug w.r.t. EXPECT_CALL?

No, let's keep the test. As mentioned in comment 18, the bug in EXPECT_CALL affects all of our GTests. Once bug 1362889 is fixed, EXPECT_CALL will start behaving properly (meaning, causing tests to fail if the number of calls doesn't match) for all tests, including this one.
Pushed the patch to the Try server (this is a server that will run automated tests, so we can be confident they're passing before committing the patch):

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

Comment 27

2 years ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/142661d6f68a
Ensure we trigger a repaint when doing two-finger touch scrolling with zooming disabled. r=botond
(In reply to Pulsebot from comment #27)
> Pushed by bballo@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/142661d6f68a
> Ensure we trigger a repaint when doing two-finger touch scrolling with
> zooming disabled. r=botond

^ The Try push looks good so I committed the patch to mozilla-inbound. (That's an integration repository that automatically gets merged to mozilla-central periodically.)
Assignee

Comment 29

2 years ago
Thank you for being such a great mentor!

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/142661d6f68a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to gabrielle.singhcadieux from comment #29)
> Thank you for being such a great mentor!

Thanks for your contribution in this bug, Gabrielle!

If you're interested in working on another bug, and would like some suggestions, let me know!
Assignee

Comment 32

2 years ago
(In reply to Botond Ballo [:botond] from comment #31)
> (In reply to gabrielle.singhcadieux from comment #29)
> > Thank you for being such a great mentor!
> 
> Thanks for your contribution in this bug, Gabrielle!
> 
> If you're interested in working on another bug, and would like some
> suggestions, let me know!

I would definitely be interested in your suggestions for another bug! Looking at the list of GFBs, it's a bit difficult for me to tell which are already being worked on.
(In reply to gabrielle.singhcadieux from comment #32)
> > If you're interested in working on another bug, and would like some
> > suggestions, let me know!
> 
> I would definitely be interested in your suggestions for another bug!

From among the bugs I'm mentoring, bug 1355656 might be a good fit for you, as it involves writing APZ GTests, which you already have some experience with from this bug.

Otherwise, I would recommend this tool for finding bugs to work on: https://www.joshmatthews.net/bugsahoy/

> Looking at the list of GFBs, it's a bit difficult for me to tell which are
> already being worked on.

Any bug which has a "Mentor" but no "Assignee" should be up for grabs.

There are also bugs which have an "Assignee" but the contributor who was working on them has abandoned them. For these, if the bug hasn't had any activity in the past 1-2 months, feel free to ask the mentor if you can take over the bug.
You need to log in before you can comment on or make changes to this bug.