If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support cross-apzc axis lock with touch-action enabled

RESOLVED FIXED in Firefox 54

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: botond, Assigned: botond)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

8 months ago
In bug 1201101, :Cwiiis implemented support for cross-apzc axis locking (where we take into account the entire scroll handoff chain when we determine whether we are scrollable in a given direction, not just the first APZC).

However, we have since enabled touch-action, and that takes a different codepath which does not have this enhancement.

To continue benefitting from cross-apzc axis locking, we should implement it for the touch-action codepath as well.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

8 months ago
mozreview-review
Comment on attachment 8835195 [details]
Bug 1337990 - Add a gtest for cross-apzc axis lock with touch-action disabled.

https://reviewboard.mozilla.org/r/110890/#review112408

::: gfx/layers/apz/test/gtest/APZTestCommon.h:257
(Diff revision 2)
>  
>    void AssertStateIsFling() const {
>      ReentrantMonitorAutoEnter lock(mMonitor);
>      EXPECT_EQ(FLING, mState);
>    }
> +  

nit: trailing whitespace. there's more in the other file in this patch too
Attachment #8835195 - Flags: review?(bugmail) → review+

Comment 6

8 months ago
mozreview-review
Comment on attachment 8835196 [details]
Bug 1337990 - Support cross-apz axis lock with touch-action enabled, and add a gtest for it.

https://reviewboard.mozilla.org/r/110892/#review112418

Looks pretty straightforward. Thanks for adding tests!
Attachment #8835196 - Flags: review?(bugmail) → review+
Priority: -- → P3
(Assignee)

Comment 7

8 months ago
Try push:

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

Looks like the changes broke APZScrollHandoffTester.PartialFlingHandoff.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

8 months ago
(In reply to Botond Ballo [:botond] from comment #7)
> Looks like the changes broke APZScrollHandoffTester.PartialFlingHandoff.

The issue here was that the Pan() function would add an additional vertical displacement between touch-start and touch-move to overcome the pan-start threshold. Since the displacement was vertical only, it ended up activating the vertical axis lock. Adding a displacement to both dimensions fixes the issue.

The updated patch fixes this and addresses the review comment.
(Assignee)

Comment 11

8 months ago
(In reply to Botond Ballo [:botond] from comment #10)
> Adding a displacement to both dimensions fixes the issue.

That doesn't quite work either. Since axis lock is determined based on the angle of the portion of the pan in the TOUCHING state, adding an equal displacement to both dimensions to get past the TOUCHING state means we'll never get into an axis lock.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

8 months ago
All right, I think it should all be sorted out now. There's a new patch to go at the beginning of the series, and one of the original patches has changed enough to need a re-review.
(Assignee)

Updated

8 months ago
Attachment #8835195 - Flags: review+ → review?(bugmail)

Comment 16

7 months ago
mozreview-review
Comment on attachment 8836242 [details]
Bug 1337990 - Introduce a PanOptions enumeration for passing options to APZCTesterBase::Pan().

https://reviewboard.mozilla.org/r/111710/#review113162

Might be better to use an enum class for better namespacing but I don't care much either way.
Attachment #8836242 - Flags: review?(bugmail) → review+
Comment on attachment 8835195 [details]
Bug 1337990 - Add a gtest for cross-apzc axis lock with touch-action disabled.

LGTM
Attachment #8835195 - Flags: review?(bugmail) → review+
(Assignee)

Comment 18

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> Might be better to use an enum class for better namespacing but I don't care
> much either way.

The reason I didn't do that is that then I'd have to define the bitwise operators for them.
I stand by my original "don't care much" but today I discovered this: http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/mfbt/TypedEnumBits.h#142
(Assignee)

Comment 20

7 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> I stand by my original "don't care much" but today I discovered this:
> http://searchfox.org/mozilla-central/rev/
> d3307f19d5dac31d7d36fc206b00b686de82eee4/mfbt/TypedEnumBits.h#142

Nice find! That works well enough, I'll change it to an enum class.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

7 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e77313e71aad
Introduce a PanOptions enumeration for passing options to APZCTesterBase::Pan(). r=kats
https://hg.mozilla.org/integration/autoland/rev/210c908511e4
Add a gtest for cross-apzc axis lock with touch-action disabled. r=kats
https://hg.mozilla.org/integration/autoland/rev/70026b13140b
Support cross-apz axis lock with touch-action enabled, and add a gtest for it. r=kats

Comment 25

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e77313e71aad
https://hg.mozilla.org/mozilla-central/rev/210c908511e4
https://hg.mozilla.org/mozilla-central/rev/70026b13140b
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.