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)

29 Branch
defect
Not set
normal

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)

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).
Attached patch manipulation_ver1.diff (obsolete) — Splinter Review
I started implement support new property "manipulation".
Any comments and suggestion are welcome.
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-
Attached patch manipulation_ver2.diff (obsolete) — Splinter Review
+ 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?
Please flag either myself or Botond for review on any changes to APZCTreeManager or AsyncPanZoomController when you feel the patch is ready for review.
Attached patch manipulation_ver3.diff (obsolete) — Splinter Review
+ 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)
Attachment #8392223 - Flags: feedback?(mbrubeck)
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: nobody → alessarik
Attachment #8392223 - Flags: review?(bugmail.mozilla)
Attachment #8392223 - Flags: review?(botond)
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 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 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+
Attached patch manipulation_ver4.diff (obsolete) — Splinter Review
+ 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 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 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+
Attached patch manipulation_ver5.diff (obsolete) — Splinter Review
+ 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 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 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+
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.
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+
Attached patch manipulation_ver6.diff (obsolete) — Splinter Review
+ 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+
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
Attached patch manipulation_ver7.diff (obsolete) — Splinter Review
+ 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 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+
Attached patch manipulation_ver8.diff (obsolete) — Splinter Review
+ changes according with last version of source code
Attachment #8401229 - Attachment is obsolete: true
Attachment #8401229 - Flags: feedback?(nicklebedev37)
Attached patch manipulation_ver8.diff (obsolete) — Splinter Review
+ repair compilation error
Attachment #8407384 - Attachment is obsolete: true
Attachment #8407513 - Flags: review?(mbrubeck)
Attachment #8407513 - Flags: feedback?(oleg.romashin)
(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?
Attachment #8407513 - Flags: review?(mbrubeck) → review+
Attached patch manipulation_ver9.diff (obsolete) — Splinter Review
+ 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)
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?
Attached patch manipulation_ver9.diff (obsolete) — Splinter Review
+ update commit message
Attachment #8410238 - Attachment is obsolete: true
Attachment #8410238 - Flags: feedback?(oleg.romashin)
Keywords: checkin-needed
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)
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)
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
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.