Closed Bug 1467838 Opened 2 years ago Closed 2 years ago

Add more tests for touch-action hit-testing

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

Right now we are not explicitly testing APZ hit-testing codepaths on touch-action content. We test some of it implicitly via the helper_touch_action* tests in test_group_touchevents, but it would be good to have explicit coverage of a wider range of scenarios in test_group_hittest.html.

I had started writing a test (attachment 8945952 [details] [diff] [review]) that I halted because of bugs in the WR codepath. With bug 1421384 fixed we should able to continue work on that and add more tests, fixing any issues that crop up.
Comment on attachment 8985998 [details]
Bug 1467838 - Add tests for APZ hit-testing on touch-action elements.

https://reviewboard.mozilla.org/r/251476/#review257774

Thanks!

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:543
(Diff revision 1)
> -// Compute the coordinates of the center of the given element.
> +// Compute the coordinates of the center of the given element. The argument
> +// can either be a string (the id of the element desired) or the element
> +// itself.
>  function centerOf(element) {
> +  if (typeof element === "string") {
> +    element = document.getElementById(element);

I've recently learned that JS automagically invents variables whose names correspond to the id's of elements on the page, referring to those elements.

So e.g. instead of doing |centerOf('taNone')|, you can do |centerOf(taNone)| and it Just Works.

I'll leave it up to you whether you want to rely on this.

::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:72
(Diff revision 1)
> + </div>
> +
> + <br/>
> +
> + <!-- More divs for hit-testing. These comprise one big outer div with
> +     a touch-action property, then inside is a scrollable div, possible with

possible -> possibly

::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:78
(Diff revision 1)
> +     a touch-action of its own, and inside that is another div to make the
> +     scrollable div scrollable. Note that the touch-action for an element
> +     includes the restrictions from all ancestor elements up to and including
> +     the element that has the default action. So any pan restrictions on the
> +     outer div should not apply to the inner div, but zooming restrictions
> +     should. Also, the touch-action on the scrollable div itself should apply

I don't understand how "any pan restrictions on the outer div should not apply to the inner div, but zooming restrictions should" follows from the preceding statements.

::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:105
(Diff revision 1)
> +<script type="application/javascript">
> +
> +var config = getHitTestConfig();
> +
> +function* test(testDriver) {
> +  for (var scroller of document.querySelectorAll('.taBigBox div')) {

Why isn't this selector '.taBigBox > div'?

::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:298
(Diff revision 1)
> +      config.utils.getViewId(document.getElementById('taScroller3')),
> +      "touch-action: manipulation outside scroller gets inherited in");
> +}
> +
> +if (!config.isWebRender) {
> +  ok(true, "This test is WebRender-only because we get a bunch of dispatch-to-content regions without it and the test isn't very interesting.");

Why do we not get touch-action related event regions (like mHorizontalPanRegion) without WR? Is that just not hooked up?
Attachment #8985998 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #3)
> ::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:78
> (Diff revision 1)
> > +     a touch-action of its own, and inside that is another div to make the
> > +     scrollable div scrollable. Note that the touch-action for an element
> > +     includes the restrictions from all ancestor elements up to and including
> > +     the element that has the default action. So any pan restrictions on the
> > +     outer div should not apply to the inner div, but zooming restrictions
> > +     should. Also, the touch-action on the scrollable div itself should apply
> 
> I don't understand how "any pan restrictions on the outer div should not
> apply to the inner div, but zooming restrictions should" follows from the
> preceding statements.

(That said, the rule that zooming restrictions are inherited into subframes but panning restrictions are not, does make logical sense to me.)
(In reply to Botond Ballo [:botond] from comment #3)
> I've recently learned that JS automagically invents variables whose names
> correspond to the id's of elements on the page, referring to those elements.
> 
> So e.g. instead of doing |centerOf('taNone')|, you can do |centerOf(taNone)|
> and it Just Works.
> 
> I'll leave it up to you whether you want to rely on this.

I think I prefer keeping it more explicit. But that's good to know!

> ::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:72
> > + <!-- More divs for hit-testing. These comprise one big outer div with
> > +     a touch-action property, then inside is a scrollable div, possible with
> 
> possible -> possibly

Fixed

> ::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:78
> (Diff revision 1)
> > +     the element that has the default action. So any pan restrictions on the
> > +     outer div should not apply to the inner div, but zooming restrictions
> > +     should. Also, the touch-action on the scrollable div itself should apply
> 
> I don't understand how "any pan restrictions on the outer div should not
> apply to the inner div, but zooming restrictions should" follows from the
> preceding statements.

The reason is that scrolling is a default action of a scrollframe, but zooming is the default action of the RCD. Also missing from my description is some variant of "restrictions are independent of each other". That is, the zooming restrictions for an element are computed by inheriting all the zooming restrictions from ancestor elements up to the element whose default action is zooming (i.e. the RCD), while the scrolling restrictions are computed by inheriting all the scrolling restrictions from ancestor elements up to the element whose default action is scrolling (i.e. the nearest enclosing scrollframe).

I've updated the comment to clarify this.

> > +  for (var scroller of document.querySelectorAll('.taBigBox div')) {
> 
> Why isn't this selector '.taBigBox > div'?

Good catch, it should indeed be .taBigBox > div. Fixed.

> ::: gfx/layers/apz/test/mochitest/helper_hittest_touchaction.html:298
> > +if (!config.isWebRender) {
> > +  ok(true, "This test is WebRender-only because we get a bunch of dispatch-to-content regions without it and the test isn't very interesting.");
> 
> Why do we not get touch-action related event regions (like
> mHorizontalPanRegion) without WR? Is that just not hooked up?

We do, sort of. But the code in FrameLayerBuilder that combines regions [1] falls back to d-t-c regions whenever it's combining  regions with different touch-action properties, because it doesn't have all the information to combine them properly. So in most of these divs we just end up with d-t-c regions.

[1] https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/layout/painting/FrameLayerBuilder.cpp#3901
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> The reason is that scrolling is a default action of a scrollframe, but
> zooming is the default action of the RCD. Also missing from my description
> is some variant of "restrictions are independent of each other". That is,
> the zooming restrictions for an element are computed by inheriting all the
> zooming restrictions from ancestor elements up to the element whose default
> action is zooming (i.e. the RCD), while the scrolling restrictions are
> computed by inheriting all the scrolling restrictions from ancestor elements
> up to the element whose default action is scrolling (i.e. the nearest
> enclosing scrollframe).
> 
> I've updated the comment to clarify this.

Thanks, makes sense now!
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fce6b794a239
Add tests for APZ hit-testing on touch-action elements. r=botond
https://hg.mozilla.org/mozilla-central/rev/fce6b794a239
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.