Closed Bug 1337990 Opened 7 years ago Closed 7 years ago

Support cross-apzc axis lock with touch-action enabled

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(3 files)

Bug 1337990 - Add a gtest for cross-apzc axis lock with touch-action disabled.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1337990 - Support cross-apz axis lock with touch-action enabled, and add a gtest for it.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1337990 - Introduce a PanOptions enumeration for passing options to APZCTesterBase::Pan().
59 bytes, text/x-review-board-request
kats
: review+
Details 
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 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 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+
Try push:

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

Looks like the changes broke APZScrollHandoffTester.PartialFlingHandoff.
(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.
(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.
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.
Attachment #8835195 - Flags: review+ → review?(bugmail)
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+
(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.
(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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: