1329241 - Add support for touch-action:pinch-zoom

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #1285685 +++

Follow-up from bug 1029631. In that bug we turned on touch-action support on Nightly, but the touch-action implementation is from the v1 Pointer Events spec. We should also support the "pinch-zoom" keyword because without it sites that use pan-x/pan-y can end up not being zoomable in places. Chrome/Edge already support this and have been engaged in some evangelism efforts to get sites using it.

Pinch-zoom is specced in https://compat.spec.whatwg.org/#touch-action

Comment 1

[Botond Ballo [:botond]]

Martin reminded me that this should be part of our desktop-zoom-post work. Tracking, tentatively as a P2, but we can revisit this in our next APZ planning meeting.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This should be easy to implement on the APZ side of things, because the machinery already exists and is hooked up. Adding CSS parsing support of the pinch-zoom property will be needed, but then the only change beyond that should be to take this line and wrap it in a if (!(touchAction & StyleTouchAction::PINCH_ZOOM)) condition.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

In this bug we'll update the CSS syntax of the touch-action CSS property from this:
auto | none | [ pan-x || pan-y ] | manipulation
to this:
auto | none | [ pan-x || pan-y || pinch-zoom ] | manipulation

The a | b notation means "either a or b" and the a || b notation means "either a, or b, or both, in any order". The brackets just indicate precedence. What this means in concrete terms is that in addition to these valid values we'll also be allowing these ones:

• pinch-zoom
• pinch-zoom pan-x
• pinch-zoom pan-y
• pan-x pinch-zoom
• pan-y pinch-zoom
• pinch-zoom pan-x pan-y
• pinch-zoom pan-y pan-x
• pan-x pinch-zoom pan-y
• pan-y pinch-zoom pan-x
• pan-x pan-y pinch-zoom
• pan-y pan-x pinch-zoom

I asked emilio about how to make the necessary changes. It's pretty much all contained in this rust code. The main changes needed will be:

• Adding a new value to the TouchAction struct for PINCH_ZOOM and update comments/annotations as appropriate.
• Updating the to_css function to serialize the pinch-zoom value if the bitflags contains it
• Updating the parse function to parse the pinch-zoom value and store it in the bitflags

However, as we can see from the list above, adding pinch-zoom factorially increases the number of possibilities allowed, and the current structure of to_css and parse means that adding pinch-zoom will dramatically increase the amount of code. Instead, it would be better to first reorganize the to_css and parse functions to be more similar to e.g. these ones for the contain CSS property. The contain CSS property similar allows many a || b-type possibilities, and uses loops instead of enumeration to handle the cases efficiently. The allowed syntax is pretty much exactly what we need for touch-action, so the code for parsing/serializing contain can be copied directly and then modified as needed for `touch-action.

Once that's all done, these lists can be augmented with additional valid and invalid values for testing, and the change in comment 2 can be made to support the CSS property in the code.

I think it would be good to split up the changes into three patches:

1. Rewrite the to_css and parse functions using the loop style seen in the contain CSS code while maintaining the existing behaviour (i.e. don't add pinch-zoom support yet).
2. Add the pinch-zoom support to the TouchAction struct, the to_css, and parse functions in box.rs, and also update the lists in property_database.js to exercise the new valid possibilities and some invalid ones.
3. Make the one-line change to nsIFrame.cpp described in comment 2, and add a test to go with it. The test can be as simple as a modification to one of the existing tests in this file.

Comment 4

[Sunita Sen]

Hello,
I would like to work on this.
I don't understand how a || b is implemented in this parse function.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

So the way the parse function works, is it loops over each token in turn here. By "each token" I mean that if the CSS has something like contain: paint size, the loop will run twice with name holding paint the first time and size the second time. It then produces a flag based on the identifier which is combined into the final result here.

Note that the syntax does not allow things like contain: strict layout, because strict is mutually exclusive with the [ size || layout || paint] option. That gets detected here because if we encounter the strict identifier, it (a) checks to make sure result.is_empty(), i.e. that no other values were encountered previously and (b) returns the final result for strict immediately, rather than continuing the loop.

The bit in the middle here makes sure that duplicate flag values (e.g. contain: layout layout) get rejected, because of the result.contains(flag) check, and also makes sure that if some other identifier turned up (e.g. contain: foo) then that gets rejected as well.

Feel free to ask if you have additional questions or need help understanding the Rust syntax.

Comment 6

[Sunita Sen]

Attachment: Bug 1329241 - Add support for touch-action:pinch-zoom - Rewrite to_css and parse for touchAction r=kats

Comment 7

[Sunita Sen]

Attachment: Bug 1329241 - Add support for touch-action:pinch-zoom - Add pinch-zoom support to touchAction r=kats

Depends on D97650

Comment 8

[Kartikaya Gupta (email:[e-mail])]

It appears my Phabricator account got accidentally disabled, so I can't comment on the patch directly. But I'll put my comment here and hopefully the account disabling will get sorted out soon.

The patch itself looks good. I did a try push with both your patches to see if there were any test failures. The try push is here and shows two tests failing.

One is the pointerevent_touch-action-verification.html test in the wpt11 job, where the explicit-pinch-zoom subtest is expected to fail but now passes. So that's good, because it confirms that your fix actually enabled some support for `pinch-zoom. We can fix the test expectation to mark it as expected to pass by removing these lines.

The other failure is in the X2 job, where the test_css-properties-db.js test is failing; the error message here indicates that this is just because two files are out sync, and you can run ./mach devtools-css-db to bring them back in sync. Presumably this command will update the devtools version of the CSS properties db based on your changes to the layout properties db, and then you can incorporate those changes into your commit.

Comment 9

[Sunita Sen]

I have made the changes, but I am not able to run the test.
After I run the command ./mach mochitest --setpref apz.subtest=helper_hittest_touchaction.html --enable-webrender gfx/layers/apz/test/mochitest/test_group_hittest.html these are the errors

 0:17.27 INFO runtests.py | Application pid: 21560
 0:17.27 Started process `GECKO(21560)`
 0:17.58 GECKO(21560) JavaScript error: resource:///modules/BrowserGlue.jsm, line 720: NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED:
 0:20.55 GECKO(21560) 1606151861599     Marionette      TRACE   Marionette enabled
 0:20.88 GECKO(21560) 1606151861931     Marionette      TRACE   Received observer notification toplevel-window-ready
 0:21.04 GECKO(21560) Can't find symbol 'eglSwapBuffersWithDamageEXT'.
 0:21.04 GECKO(21560) Can't find symbol 'eglSetDamageRegionKHR'.
 0:24.75 GECKO(21560) JavaScript error: resource://gre/modules/ExtensionCommon.jsm, line 500: NotFoundError: WindowGlobalChild.getActor: No such JSWindowActor 'Conduits'
 0:24.75 GECKO(21560) JavaScript error: moz-extension://43e9bea2-3daf-4ec1-b7ed-e0252ddf9d16/background.js, line 9: InvalidStateError: An exception was thrown
 0:24.75 GECKO(21560) JavaScript error: resource://gre/modules/ExtensionCommon.jsm, line 500: NotFoundError: WindowGlobalChild.getActor: No such JSWindowActor 'Conduits'
 0:24.75 GECKO(21560) JavaScript error: moz-extension://afc2ddd0-da3b-4c11-bbd0-0d353371834a/background/startBackground.js, line 57: InvalidStateError: An exception was thrown

I face similar errors everytime I try to run.
What should I do?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Is that the full output you get when you try running the test? It looks like there might be some stuff missing. If you trimmed it, please include the full output. Often there are things that look like errors but are safe to ignore, and other things that don't look like errors but are the root cause of the problem. And even if you are not able to run the test please submit the patch; I can review it and push it to our testing server which we'll need to do anyway. Thanks!

Comment 11

[Sunita Sen]

Attachment: Bug 1329241 - Add support for touch-action:pinch-zoom - update hittest_touchaction and nsIFrame r=kats

Depends on D97815

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Latest try push is green: https://treeherder.mozilla.org/jobs?repo=try&revision=5fc9363d819aa59cf2a721e5c1d5bff81befbd47

I sent an intent email to dev-platform, but it's stuck in the moderation queue. I'll update this comment with a link once it gets through. (Update: https://lists.mozilla.org/pipermail/dev-platform/2020-November/025910.html)

And I filed bug 1679275 for the follow-up tweaks that Emilio suggested in phabricator.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24d718383b1b
Add support for touch-action:pinch-zoom - Rewrite to_css and parse for touchAction r=kats,emilio
https://hg.mozilla.org/integration/autoland/rev/25b75dd77432
Add support for touch-action:pinch-zoom - Add pinch-zoom support to touchAction r=kats,emilio
https://hg.mozilla.org/integration/autoland/rev/3cb1e2700349
Add support for touch-action:pinch-zoom - update hittest_touchaction and nsIFrame r=kats

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/24d718383b1b
https://hg.mozilla.org/mozilla-central/rev/25b75dd77432
https://hg.mozilla.org/mozilla-central/rev/3cb1e2700349

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Thank you Sunita!

Comment 16

[Sunita Sen]

Thanks, it was fun contributing! :)

Comment 17

[Rachel Andrew]

This has been added to BCD and the release notes.