Closed Bug 1329241 Opened 7 years ago Closed 3 years ago

Add support for touch-action:pinch-zoom

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
85 Branch
Tracking Status
firefox85 --- fixed

People

(Reporter: kats, Assigned: sunitacool41, Mentored)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [gfx-noted])

Attachments

(3 files)

+++ 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
Type: defect → enhancement

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.

Priority: P3 → P2

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.

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.

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

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.

Assignee: nobody → sunitacool41

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?

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!

Attachment #9189609 - Attachment description: Bug 1329241 - Add support for touch-action:pinch-zoom - update hittest_touchaction and nslFrame r=kats → Bug 1329241 - Add support for touch-action:pinch-zoom - update hittest_touchaction and nsIFrame r=kats

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.

Mentor: kats
Keywords: dev-doc-needed
Pushed by kgupta@mozilla.staktrace.com:
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Thank you Sunita!

Thanks, it was fun contributing! :)

This has been added to BCD and the release notes.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: