Closed
Bug 979345
Opened 10 years ago
Closed 10 years ago
Implement "touch-action: manipulation" CSS value for Pointer Events
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mbrubeck, Assigned: alessarik)
References
()
Details
(Keywords: compat, dev-doc-needed, mobile, Whiteboard: [parity-ie])
Attachments
(1 file, 11 obsolete files)
21.84 KB,
patch
|
Details | Diff | Splinter Review |
The Pointer Events WG is planning to add "manipulation" as a value for the "touch-action" CSS property. This is already implemented by IE (as an extension) and Blink plans to ship it too after it is specced: http://lists.w3.org/Archives/Public/public-pointer-events/2014JanMar/0125.html "touch-action: manipulation" allows panning and pinch zooming, but not double-tap zooming. This allows sites to use double-tap for their own purposes, or to opt out of the 300ms delay for double-tap detection (bug 941995).
Updated•10 years ago
|
Keywords: dev-doc-needed
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
I started implement support new property "manipulation". Any comments and suggestion are welcome.
Comment 2•10 years ago
|
||
Comment on attachment 8386751 [details] [diff] [review] manipulation_ver1.diff Review of attachment 8386751 [details] [diff] [review]: ----------------------------------------------------------------- In general looks good but you need to add tests for adding value to css style system. So basically minusing only due to lack of tests stuff. Also could you please split your patches into the two parts: css one and apzc one (since I believe they will be reviewed by different persons). ::: gfx/layers/composite/APZCTreeManager.h @@ +32,5 @@ > NONE = 0, > VERTICAL_PAN = 1 << 0, > HORIZONTAL_PAN = 1 << 1, > ZOOM = 1 << 2, > + DOUBLE_TAP_ZOOM = 1 << 3, If ZOOM doesn't include DOUBLE_TAP_ZOOM behavior i think we need to rename ZOOM to PINCH_ZOOM ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +994,5 @@ > APZC_LOG("%p got a double-tap in state %d\n", this, mState); > + > + if (!TouchActionAllowDoubleTapZoom()) { > + return nsEventStatus_eIgnore; > + } Out of curiosity: what sequence of touch/pointer events double tap would produce with and without touch-action: manipulation? Please make sure that correct one is produced. ::: layout/style/nsCSSProps.cpp @@ +1619,5 @@ > > const KTableValue nsCSSProps::kTouchActionKTable[] = { > eCSSKeyword_pan_x, NS_STYLE_TOUCH_ACTION_PAN_X, > eCSSKeyword_pan_y, NS_STYLE_TOUCH_ACTION_PAN_Y, > + eCSSKeyword_manipulation, NS_STYLE_TOUCH_ACTION_MANIPULATION, Please add appropriate tests to the file http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#4363 and make sure that all of them pass. ::: layout/style/nsCSSValue.cpp @@ +1009,5 @@ > > case eCSSProperty_touch_action: > nsStyleUtil::AppendBitmaskCSSValue(aProperty, intValue, > NS_STYLE_TOUCH_ACTION_PAN_X, > + NS_STYLE_TOUCH_ACTION_MANIPULATION, I believe you have a type here. ::: layout/style/nsComputedDOMStyle.cpp @@ +3824,5 @@ > } else { > nsAutoString valueStr; > nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_touch_action, > intValue, NS_STYLE_TOUCH_ACTION_PAN_X, > + NS_STYLE_TOUCH_ACTION_MANIPULATION, valueStr); Likely it is a typo.
Attachment #8386751 -
Flags: feedback-
Assignee | ||
Comment 3•10 years ago
|
||
+ ZOOOM -> PINCH_ZOOM + Some tests for manipulation property + Correct behaviour Any comments and suggestions are welcome.
Attachment #8386751 -
Attachment is obsolete: true
Attachment #8389141 -
Flags: feedback?
Comment 4•10 years ago
|
||
Please flag either myself or Botond for review on any changes to APZCTreeManager or AsyncPanZoomController when you feel the patch is ready for review.
Assignee | ||
Comment 5•10 years ago
|
||
+ some tests for manipulation property + correct behaviour Any comments and suggestions are welcome.
Attachment #8389141 -
Attachment is obsolete: true
Attachment #8389141 -
Flags: feedback?
Attachment #8392223 -
Flags: review?(bugs)
Attachment #8392223 -
Flags: review?(bugmail.mozilla)
Attachment #8392223 -
Flags: feedback?(oleg.romashin)
Attachment #8392223 -
Flags: feedback?(nicklebedev37)
Updated•10 years ago
|
Attachment #8392223 -
Flags: feedback?(mbrubeck)
Comment 6•10 years ago
|
||
Comment on attachment 8392223 [details] [diff] [review] manipulation_ver3.diff Given that I didn't review Bug 795567, I think dbaron and kats should look at this.
Attachment #8392223 -
Flags: review?(bugs) → review?(dbaron)
I can review the CSS side (though not right now), but you should find somebody else to review the rest of the patch.
Assignee | ||
Updated•10 years ago
|
Attachment #8392223 -
Flags: review?(bugmail.mozilla)
Attachment #8392223 -
Flags: review?(botond)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8392223 [details] [diff] [review] manipulation_ver3.diff Review of attachment 8392223 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1873,1 @@ > bool AsyncPanZoomController::TouchActionAllowZoom() { We might want to rename this method TouchActionAllowPinchZoom. ::: layout/style/nsCSSParser.cpp @@ +11662,5 @@ > > + // Manipulation is not allowed in conjunction with others. > + if ((intValue | nextIntValue) & NS_STYLE_TOUCH_ACTION_MANIPULATION) { > + return false; > + } Note that IE currently *does* allow "manipulation" in combination with other values, although the spec disallows it. We're currently discussing (on public-pointer-events) whether to change the spec to match IE, or vice-versa. For now, this patch is correct.
Attachment #8392223 -
Flags: feedback?(mbrubeck) → feedback+
Comment 9•10 years ago
|
||
Comment on attachment 8392223 [details] [diff] [review] manipulation_ver3.diff Review of attachment 8392223 [details] [diff] [review]: ----------------------------------------------------------------- I looked at the APZC bits; there are some minor issues but looks fine otherwise. It would also be nice to also add a test for this feature to TestAsyncPanZoomController.cpp. ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +951,5 @@ > } > > nsEventStatus AsyncPanZoomController::OnSingleTapUp(const TapGestureInput& aEvent) { > APZC_LOG("%p got a single-tap-up in state %d\n", this, mState); > + if(TouchActionAllowDoubleTapZoom()) { Merge this into the controller && !mZoomConstraints.mAllowDoubleTapZoom check below. @@ +997,5 @@ > > nsEventStatus AsyncPanZoomController::OnDoubleTap(const TapGestureInput& aEvent) { > APZC_LOG("%p got a double-tap in state %d\n", this, mState); > + if (!TouchActionAllowDoubleTapZoom()) { > + return nsEventStatus_eIgnore; Merge this into the if (controller) check below @@ +1886,5 @@ > + return true; > +} > + > +bool AsyncPanZoomController::TouchActionAllowDoubleTapZoom() { > + if(!mTouchActionPropertyEnabled) { nit: space before ( ::: widget/xpwidgets/ContentHelper.cpp @@ +35,5 @@ > ContentHelper::UpdateAllowedBehavior(uint32_t aTouchActionValue, bool aConsiderPanning, TouchBehaviorFlags& aOutBehavior) > { > if (aTouchActionValue != NS_STYLE_TOUCH_ACTION_AUTO) { > // Dropping zoom flag since zooming requires touch-action values of all touches > // to be AUTO. Update this comment.
Attachment #8392223 -
Flags: review?(bugmail.mozilla)
Attachment #8392223 -
Flags: review-
Attachment #8392223 -
Flags: feedback?(mbrubeck)
Attachment #8392223 -
Flags: feedback+
Comment 10•10 years ago
|
||
Comment on attachment 8392223 [details] [diff] [review] manipulation_ver3.diff Review of attachment 8392223 [details] [diff] [review]: ----------------------------------------------------------------- Mid-aired with matt, sorry.
Attachment #8392223 -
Flags: feedback?(mbrubeck) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
+ TouchActionAllowZoom -> TouchActionAllowPinchZoom + small changes from comments (In reply to Matt Brubeck (:mbrubeck) from comment #8) > Note that IE currently *does* allow "manipulation" in combination with other > values, although the spec disallows it. We're currently discussing (on > public-pointer-events) whether to change the spec to match IE, or > vice-versa. For now, this patch is correct. IMHO correct version is auto|none|[pan-x|pan-y]|manipulation. In other case we can find property "pan-x manipulation" - if anybody can explain what it is meaning, feel free to explain. For the present I propose this version of patch.
Attachment #8392223 -
Attachment is obsolete: true
Attachment #8392223 -
Flags: review?(dbaron)
Attachment #8392223 -
Flags: review?(botond)
Attachment #8392223 -
Flags: feedback?(oleg.romashin)
Attachment #8392223 -
Flags: feedback?(nicklebedev37)
Attachment #8394662 -
Flags: review?(mbrubeck)
Attachment #8394662 -
Flags: review?(dbaron)
Attachment #8394662 -
Flags: review?(bugmail.mozilla)
Attachment #8394662 -
Flags: review?(botond)
Attachment #8394662 -
Flags: feedback?(oleg.romashin)
Attachment #8394662 -
Flags: feedback?(nicklebedev37)
Comment 12•10 years ago
|
||
Comment on attachment 8394662 [details] [diff] [review] manipulation_ver4.diff Review of attachment 8394662 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the gfx/ and widget/ bits.
Attachment #8394662 -
Flags: review?(bugmail.mozilla) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8394662 [details] [diff] [review] manipulation_ver4.diff Review of attachment 8394662 [details] [diff] [review]: ----------------------------------------------------------------- r+ for APZ and widget bits ::: gfx/layers/ipc/AsyncPanZoomController.h @@ +572,5 @@ > was not set yet. we still need to abort animations. */ > }; > > /* > * Returns whether current touch behavior values allow zooming. s/zooming/pinch-zooming
Attachment #8394662 -
Flags: review?(botond) → review+
Assignee | ||
Comment 14•10 years ago
|
||
+ update only one comment (In reply to Botond Ballo [:botond] from comment #13) > s/zooming/pinch-zooming
Attachment #8394662 -
Attachment is obsolete: true
Attachment #8394662 -
Flags: review?(mbrubeck)
Attachment #8394662 -
Flags: review?(dbaron)
Attachment #8394662 -
Flags: feedback?(oleg.romashin)
Attachment #8394662 -
Flags: feedback?(nicklebedev37)
Attachment #8395584 -
Flags: review?(mbrubeck)
Attachment #8395584 -
Flags: review?(dbaron)
Attachment #8395584 -
Flags: review?(bugmail.mozilla)
Attachment #8395584 -
Flags: review?(botond)
Attachment #8395584 -
Flags: feedback?(oleg.romashin)
Attachment #8395584 -
Flags: feedback?(nicklebedev37)
Comment 15•10 years ago
|
||
Comment on attachment 8395584 [details] [diff] [review] manipulation_ver5.diff Review of attachment 8395584 [details] [diff] [review]: ----------------------------------------------------------------- You don't need to request a new review if that's all you changed.
Attachment #8395584 -
Flags: review?(bugmail.mozilla)
Comment 16•10 years ago
|
||
Comment on attachment 8395584 [details] [diff] [review] manipulation_ver5.diff Review of attachment 8395584 [details] [diff] [review]: ----------------------------------------------------------------- As Kats said, feel free to "carry" the r+ when making a trivial change.
Attachment #8395584 -
Flags: review?(botond) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8395584 -
Flags: review?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #8) > Note that IE currently *does* allow "manipulation" in combination with other > values, although the spec disallows it. We're currently discussing (on > public-pointer-events) whether to change the spec to match IE, or > vice-versa. For now, this patch is correct. It looks like https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property has changed so it now allows 'manipulation' in combination with other properties. It's not clear to me how 'manipulation pan-x' differs from 'manipulation', though.
Comment 18•10 years ago
|
||
I don't think the spec has changed in this regard. I believe the plan is to change it so that 'manipulation pan-x' is illegal, but there was still some open debate. See http://www.w3.org/2014/03/11-pointerevents-minutes.html#item02. IE treats 'manipulation pan-x' and 'manipulation' exactly the same. Blink currents treats it as illegal (we implemented according to the grammar in the MSDN docs before it was in the spec). The compat risk here should be very small - even if the spec stays as-is, it'll be a pretty low priority bug for us to change blink to support this weird case (and it would complicated our code a little since we can't currently distinguish the two cases in the data structures we use internally).
Comment on attachment 8395584 [details] [diff] [review] manipulation_ver5.diff In nsComputedDOMStyle you can change the AppendBitmaskCSSValue call just like you do in nsCSSValue.cpp, instead of adding a separte case for NS_STYLE_TOUCH_ACTION_MANIPULATION. r=dbaron on the layout/style parts with that change
Attachment #8395584 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 20•10 years ago
|
||
+ small update according Comment19
Attachment #8395584 -
Attachment is obsolete: true
Attachment #8395584 -
Flags: feedback?(oleg.romashin)
Attachment #8395584 -
Flags: feedback?(nicklebedev37)
Attachment #8399963 -
Flags: review?(dbaron)
Comment on attachment 8399963 [details] [diff] [review] manipulation_ver6.diff >diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp >+ nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_touch_action, intValue, >+ NS_STYLE_TOUCH_ACTION_PAN_X, NS_STYLE_TOUCH_ACTION_MANIPULATION, valueStr); Please wrap " valueStr);" to the next line to avoid going over 80 chars. r=dbaron on the layout/style parts with that, although you really shouldn't have asked for review again since I gave you review+ before
Attachment #8399963 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Maybe the correct way to change all this cases to one? For example: nsAutoString valueStr; nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_touch_action, intValue, NS_STYLE_TOUCH_ACTION_NONE, NS_STYLE_TOUCH_ACTION_MANIPULATION, valueStr); val->SetString(valueStr); If we have checking correct state in CSSParserImpl::ParseTouchAction we always should have correct value in nsComputedDOMStyle::DoGetTouchAction
Assignee | ||
Comment 23•10 years ago
|
||
+ changes in CSSParserImpl::ParseTouchAction() + changes in nsComputedDOMStyle::DoGetTouchAction()
Attachment #8399963 -
Attachment is obsolete: true
Attachment #8401229 -
Flags: feedback?(oleg.romashin)
Attachment #8401229 -
Flags: feedback?(nicklebedev37)
Comment 24•10 years ago
|
||
Comment on attachment 8401229 [details] [diff] [review] manipulation_ver7.diff I'm not very much familiar with this code, but I definitely would like to have more tests which are covering new functionality specifically.
Attachment #8401229 -
Flags: feedback?(oleg.romashin) → feedback+
Assignee | ||
Comment 25•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=66004a6f6696
Assignee | ||
Comment 26•10 years ago
|
||
+ changes according with last version of source code
Attachment #8401229 -
Attachment is obsolete: true
Attachment #8401229 -
Flags: feedback?(nicklebedev37)
Assignee | ||
Comment 27•10 years ago
|
||
+ repair compilation error
Attachment #8407384 -
Attachment is obsolete: true
Attachment #8407513 -
Flags: review?(mbrubeck)
Attachment #8407513 -
Flags: feedback?(oleg.romashin)
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Rick Byers from comment #18) > I don't think the spec has changed in this regard. I believe the plan is to > change it so that 'manipulation pan-x' is illegal, but there was still some > open debate. See > http://www.w3.org/2014/03/11-pointerevents-minutes.html#item02. The spec was changed already. Correct value is auto | none | [ pan-x || pan-y ] | manipulation https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property (In reply to Oleg Romashin (MS) from comment #24) > I'm not very much familiar with this code, but I definitely would like to > have more tests which are covering new functionality specifically. There are a number of tests in changes in layout/style/test/property_database.js Is it not enought?
Reporter | ||
Updated•10 years ago
|
Attachment #8407513 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 29•10 years ago
|
||
+ update according with latest version of source code
Attachment #8407513 -
Attachment is obsolete: true
Attachment #8407513 -
Flags: feedback?(oleg.romashin)
Attachment #8410238 -
Flags: feedback?(oleg.romashin)
Assignee | ||
Comment 30•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c5ac0ecd1a26 https://tbpl.mozilla.org/?tree=Try&rev=379dceea2cee Builds have several issues. But I think they are not related with my changes. Can I checkin last version of my patch?
Assignee | ||
Comment 31•10 years ago
|
||
+ update commit message
Attachment #8410238 -
Attachment is obsolete: true
Attachment #8410238 -
Flags: feedback?(oleg.romashin)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 32•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/039b15c9b84e
Flags: in-testsuite+
Keywords: checkin-needed
Comment 33•10 years ago
|
||
backout |
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3215c7fc090b for breaking the builds. Pretty much every build broke like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=38275632&tree=Mozilla-Inbound
Flags: in-testsuite+ → needinfo?(alessarik)
Assignee | ||
Comment 34•10 years ago
|
||
Very strange hear for me that previous patch breaks build. On TRY servers all were done successfully. New revision with build: https://tbpl.mozilla.org/?tree=Try&rev=01f41cff9d1d
Attachment #8410279 -
Attachment is obsolete: true
Flags: needinfo?(alessarik)
Comment 35•10 years ago
|
||
landing |
It looks like the patch that was landed earlier contained a compilation error introduced during rebasing. I re-landed the latest version of the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd16de7bf25
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bfd16de7bf25
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•