Add support for touch-action:pinch-zoom
Categories
(Core :: Panning and Zooming, enhancement, P2)
Tracking
()
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)
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1329241 - Add support for touch-action:pinch-zoom - Add pinch-zoom support to touchAction r=kats
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
Reporter | ||
Updated•8 years ago
|
Updated•6 years ago
|
Comment 1•4 years ago
|
||
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.
Reporter | ||
Comment 2•4 years ago
|
||
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.
Reporter | ||
Comment 3•4 years ago
|
||
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 forPINCH_ZOOM
and update comments/annotations as appropriate. - Updating the
to_css
function to serialize thepinch-zoom
value if the bitflags contains it - Updating the
parse
function to parse thepinch-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:
- Rewrite the
to_css
andparse
functions using the loop style seen in thecontain
CSS code while maintaining the existing behaviour (i.e. don't addpinch-zoom
support yet). - Add the
pinch-zoom
support to theTouchAction
struct, theto_css
, andparse
functions in box.rs, and also update the lists in property_database.js to exercise the new valid possibilities and some invalid ones. - 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.
Assignee | ||
Comment 4•4 years ago
|
||
Hello,
I would like to work on this.
I don't understand how a || b
is implemented in this parse function.
Reporter | ||
Comment 5•4 years ago
|
||
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 | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
Depends on D97650
Comment hidden (obsolete) |
Assignee | ||
Comment 9•4 years ago
|
||
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?
Reporter | ||
Comment 10•4 years ago
|
||
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!
Assignee | ||
Comment 11•4 years ago
|
||
Depends on D97815
Updated•4 years ago
|
Reporter | ||
Comment 12•4 years ago
|
||
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•4 years ago
|
||
Comment 14•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/24d718383b1b
https://hg.mozilla.org/mozilla-central/rev/25b75dd77432
https://hg.mozilla.org/mozilla-central/rev/3cb1e2700349
Reporter | ||
Comment 15•4 years ago
|
||
Thank you Sunita!
Assignee | ||
Comment 16•4 years ago
|
||
Thanks, it was fun contributing! :)
Comment 17•4 years ago
|
||
This has been added to BCD and the release notes.
Description
•