Closed Bug 795567 Opened 12 years ago Closed 10 years ago

Implement touch-action CSS property for Pointer Events

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mardeg, Assigned: nl)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(7 files, 42 obsolete files)

19.86 KB, patch
Details | Diff | Splinter Review
7.69 KB, patch
Details | Diff | Splinter Review
26.91 KB, patch
Details | Diff | Splinter Review
18.59 KB, patch
Details | Diff | Splinter Review
3.03 KB, patch
Details | Diff | Splinter Review
27.22 KB, patch
Details | Diff | Splinter Review
12.26 KB, patch
Details | Diff | Splinter Review
This bug is for the touch-action CSS property implementation part of the spec submission which "defines events and related interfaces for handling hardware agnostic pointer input from devices like a mouse, pen, or touchscreen. For compatibility with existing mouse-based content, this specification also describes a mapping to fire [DOM-LEVEL-3-EVENTS] Mouse Events for pointer device types other than mouse"
if it makes it through the drafts in its current form.
Depends on: 745071
Updated the URL field now that it's in the Candidate Recommendation period, which ends 09 August 2013.
Blocks: 822898
The biggest design question here I think is how touch-action interacts with touch events.  I'm working on a particular proposal for Blink that involves adding an additional CSS property (tentatively called touch-action-delay) to allow sites to opt-into touch events getting the full benefits of touch-action.  My design doc is here:  https://docs.google.com/a/chromium.org/document/d/1CV2AXyrdPdGSRypAQcfGrgQVuWYi50EzTmVsMLWgRPM/edit# - please feel free to add comments.  I'm really only interested in designs which both Chromium and Firefox can agree on :-)

I've got some initial code for this in chromium, but the full implementation is going to be quite difficult and time consuming due to our existing touch/gesture design.  You can follow along at crbug.com/241964.
David, do you know if there are example of existing CSS property which would be close from implementation point of view and functionality to touch-action CSS property? And can be used as template example for touch-action property implementation.
Would be nice to know whether the comments raised in this thread have been addressed:
http://lists.w3.org/Archives/Public/public-pointer-events/2013JanMar/thread.html#msg18

The closest analogs are probably font-synthesis and text-decoration-line.
(I'm assuming you're talking about in terms of CSS parsing and computation -- the stuff prior to the computed value.  I'm not sure what an analog for the later stages of the implementation would mean.)
Yes, I want to know which example to look in order to define property, parse it, and apply to current touch-action functionality defined in pointer events spec.
Last one I guess we can figure out. Thanks for hint
Work-in-progress property implementation.
Would this prop apply to individual elements within a page? I'm curious because down at the widget layer, we deal with windows only. So for example in Windows widget we can send events representing default behavior to a window (pixel scroll and mouse) or we can send touch (or both all the time!). 

Right now the w3c touch implementation pays attention to the RegisterTouchWindow/UnregisterTouchWindow calls, and from that, decided whether the window should receive touch or default.

Wondering how we plan to address this for pointer events. Our touch code in widget needs some work, so I'm curious what the requirements might be.
hit-testing would happen at least partially in the layer level, I believe.
Separate layer per different touch-action area or some such.
No, implementing this should be done the same way that implementing preventDefault on regular touch events is done. That is, the touch event is dispatched to content, content does the hit-testing, and if it lands on an element which has disabled the touch-action, then it notifies the APZC/gfx code to not perform the default action. I discussed this with Nick and mbrubeck, and this approach makes the most sense. Generating separate layers for each element with a touch-action is not going to work very well.
Why not? We need to make the hittesting off-main-thread to get good performance.
Also, the whole point of touch-action is that it can be handled off-main-thread.
Layers are expensive, and having a separate layer for each touch-action area will very quickly run into our limit on layer count. My first thought was also that this would need to be implemented with off-main-thread hit-testing in the layers code but I think that route will be much more complicated. As it is we will still have to support the preventDefault route for backwards compatibility so there will still be a delay while we wait on the main thread. Putting touch-action alone on the input or compositor thread doesn't buy us very much.
Well, we don't have to use separate layer, but just map coordinates in a layer to certain touch-action values.
(Note, other browsers, AFAIK, implement this part off-main-thread)
For what it's worth, in chrome we see compositor-thread hit testing as the main advantage of touch-action and one of the main reasons we care about pointer events at all.  It's going to take us substantial work, but I'm extending our compositor-thread hit testing to support touch action.  If we only processed touch-action on the main thread, then it would really be no better than today's preventDefault model.

Our compositor hit testing overview: http://www.chromium.org/developers/design-documents/compositor-hit-testing (although I need to update this - we now track a region per layer).

Chrome touch-action design doc: https://docs.google.com/a/chromium.org/document/d/1CV2AXyrdPdGSRypAQcfGrgQVuWYi50EzTmVsMLWgRPM/edit?usp=drive_web
And yes, Oli, in Blink we keep a list of rects per layer.  touch-action is easier than the touch event case because touch-action is limited to block-level elements (can ignore all the complexities of spans, overhangs, continuations, etc.).

I expect there will still be edge cases where we don't know for sure on the compositor thread what is being hit, so I'm currently thinking of tracking a separate conservative region for each effective touch-action value and hit-test against them all on the compositor thread.  If the hit test matches more than one region then it must be sent to the main thread.  I'll record how often this happens and drive it down so it's rare - as long >99% of the hit tests in practice can happen on the compositor thread then I'll be happy.  There are some tricky perf tradeoffs here.
Sounds like we aren't going to put effort into this until apz is running on desktop, is this correct?
What has that to do with this bug?
Also, even if we couldn't handle touch-action off-main-thread on desktop yet, we should certainly
implement it correctly for mobile. On desktop fallback to main-thread shouldn't be too bad.
(In reply to Olli Pettay [:smaug] from comment #18)
> What has that to do with this bug?

Nick Lebedev, who posted the initial patch here was volunteering some time trying to get this working on desktop with our current implementation.
Drive-by comment after nicklebedev asked about this in #layout: should we be checking for this property in platform code, or perhaps instead in the JS code that handles touches?

I suspect maybe the latter, e.g. in BrowserElementPanning.js's "_findPannable" method, which is what's used (at least in B2G) to find the node that should be scrolled/panned, when the user touches and drags on the screen. MXR link:  http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#338
BrowserElementPanning.js's panning code dealing with subframes will soon be obsolete, now that we have an AsyncPanZoomController per scrollable frame.

Based on the use cases and desired performance characteristics the approach outlined in comment 14 (combined with modifying the AsyncPanZoomController to check those rects and disable scrolling as necessary) makes the most sense, at least for the async scrolling platforms.
I was discussing this a bit further with Nick on IRC, and he wanted to know how to propagate the regions through the layers to the APZC. I have a general idea of how that will work, and I wanted to put it here so that everybody can comment on it if they have any objections.

I think the best place to store the touch-action regions is on the FrameMetrics object, which is populated in the RecordFrameMetrics function nsDisplayList.cpp. Each container layer (including all scrollable layers) has its own FrameMetrics, but not each HTML element has its own FrameMetrics. So for a given element with a touch-action property, we would have to find the closest containing scrollable layer, and make a note of the element's bounds on that layer's FrameMetrics.

The FrameMetrics is propagated over to the compositor thread and goes into the AsyncPanZoomController that controls the scrolling of the scrollable layer. When the APZC gets the input events that drive scrolling, it will need to do a hit test against the regions that were stored in the FrameMetrics to see if scrolling is allowed at that input position (and whether x- or y-scroll is allowed only, etc.) and then respect that behaviour.
Blocks: 758146
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> I think the best place to store the touch-action regions is on the
> FrameMetrics object, which is populated in the RecordFrameMetrics function
> nsDisplayList.cpp.

What about the case where there's an element with a transform animated by the compositor? These regions would be computed on the main thread so wouldn't take account of the transforms on such elements. Isn't that going to be a problem with this approach?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> What about the case where there's an element with a transform animated by
> the compositor? These regions would be computed on the main thread so
> wouldn't take account of the transforms on such elements. Isn't that going
> to be a problem with this approach?

Maybe. The compositor thread does know about the transform too though so it can unapply the transform prior to doing the hit test against the value stored in the FrameMetrics. This is basically what we do in the APZC code as well, because the FrameMetrics data is computed on the main thread at layout time and there are additional transforms in play when the element is rendered on the screen. The code for this is not pretty but it seems to work.
You said:
> So for a given element with a touch-action property, we would have to find the closest containing scrollable layer,
> and make a note of the element's bounds on that layer's FrameMetrics.
If that element has an animated transform, how would the compositor be able to tell which parts of the scrollable layer's touch-action region belong to the transformed element and which parts are due to other elements?
I talked to kats this morning. We need APZC to be able to take animated transforms into account when it does hit-detection, so we need to do hit detecting using the layer tree rather than just using regions set by the layout main thread. See bug 918288.
Depends on: 918288
As mbrubeck pointed - there are a new discussion started about how pointer and touch events should co-exist with each other. The working group on this topic is available by this link - http://www.w3.org/community/touchevents/.

I believe given the discussion we need to come to conclusion about how the touch and pointer events will live together in Firefox.

Maybe for current/temporary approach we could try to implement interaction in the following way: watch for the pointer events handlers and if they exist - dispatch pointer events, if there are no of them - dispatch usual touch events.
WIP, not stable!, based on revison 35b73bb96ca0

The purpose of the this patch is to demonstrate how the flow and state machine is going to change after introducing pointer events (currently i’m following the approach of disabling touch events in case there are pointer events handler on the page)

There are 3 important points:
    * value for touch action is hardcoded (currently it’s pan-y), Roc is currently helping to improve hit testing and bringing touch-action css info to the layers).
   * flag the specifies whether we should dispatch pointer events instead of touch ones is hardcoded (currently it’s always true), to avoid hardcode i think it can be taken from the frameMetrics after we add mayHavePointerListeners property to it.
   * conversion of touch events to pointer events isn’t done at this patch (so events that are going to be pointers are currently dispatched as touch).

one more note: the metroinput code will likley change i believe after landing fix for ticket https://bugzilla.mozilla.org/show_bug.cgi?id=931763.
Attachment #824755 - Flags: feedback+
Attachment #824755 - Attachment is obsolete: true
Apzc part of touch-action implementation

* brings stub for touch-action value hit testing to the apzc
* adds default behavior (panning) restriction according to the touch-action value * adds some logic for differing pointer events from the touch ones

still WIP
Attachment #789734 - Attachment is obsolete: true
Attachment #825379 - Flags: feedback+
Attachment #789734 - Attachment description: Hg patch that stores first step of adding touch-action property (not stable). → Css part of touch-action implementation
Attachment #789734 - Attachment is obsolete: false
* Adds state and flow handling for pointer events to the metro input (since pointer events differs from touch ones on the preventing default behavior)

Note: touch events aren't converted to pointer ones in this patch and dispatched as is.

WIP
Attachment #825384 - Flags: feedback+
Attachment #825384 - Attachment description: Wodget part of touch-action implementation → Widget part of touch-action implementation
Attachment #825384 - Flags: feedback+
Comment on attachment 825384 [details] [diff] [review]
Widget part of touch-action implementation

Review of attachment 825384 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +1280,5 @@
> +    // default behaviour (like scrolling) already started and we have dispatched pointer cancel
> +    // and so we simply forwarding events to apzc.
> +    mWidget->ApzReceiveInputEvent(event);
> +  }
> +  else {

nit - I'd prefer: } else {
Attachment #825384 - Flags: feedback+
Assignee: nobody → nicklebedev37
Comment on attachment 825379 [details] [diff] [review]
Apzc part of touch-action implementation

I'll take a look at this tomorrow and provide some feedback.
Attachment #825379 - Flags: feedback+ → feedback?(bugmail.mozilla)
(In reply to Jim Mathies [:jimm] from comment #32)
> > +  }
> > +  else {
> 
> nit - I'd prefer: } else {

And } else { is also following Gecko coding style.
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Comment on attachment 825379 [details] [diff] [review]
Apzc part of touch-action implementation

Review of attachment 825379 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/APZCTreeManager.h
@@ +253,5 @@
> +   * dispatching only pointer events (happens when there are at least one 
> +   * pointer event handler attached to the page) or only touch events (happens
> +   * otherwise).
> +   */
> +  bool GetPointerEventsMode();

I don't know if doing it this way makes sense. I would rather have widget code dispatch pointer events only, and then put some code in nsEventStateManager to create touch event listeners if needed. Pointer events seems like the best long-term solution and touch events can be generated from them, so there's no need to make special-cased code here support both.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +509,5 @@
> +   * on the main thread and attached to the current scrollable layer. Each of such regions
> +   * contains info about touch action css property.
> +   * IMPORTANT NOTE: for now it's only a stub and returns hardcoded magic value.
> +   */
> +  TouchActionValue GetTouchActionValue(ScreenIntPoint &point);

This might be better on the APZCTreeManager. But it's fine here for now, the basic concept is reasonable.
Attachment #825379 - Flags: feedback?(bugmail.mozilla) → feedback+
Thanks for your comment, kats.

Let me describe why i made two separate modes (touch events and pointer events):
touch and pointer events behave similarly but in some cases they differ a lot, e.g. when treating touch-action css property and dispatching cancel event, even more - it is not yet standardized how touch and pointer events would behave together (there is a w3c group that working on it). So if we make one kind of events generated from another one we may get the behavior that’s not according to the spec. In case you described - we will have incorrect behavior of touch events.

Therefore i made two separate modes, given this approach we have both pointer and touch events working fine (separately) and which kind of events is “turned on” is determined based on some condition (currently this condition is absence of pointer events handlers in the javascript).

Does this make sense?
The "absence of pointer event handlers in the javascript" heuristic makes me very nervous.  It's tempting to think of a web page as a monolithic thing with a single owner, but in practice this is seldom the case.

Before enabling point event support by default we (I work on blink, but same goes for Gecko I think) should be very careful to validate that there is a safe migration path from touch to pointer that won't break the web.  Eg. with your approach it sounds like code that can be embedded in other pages (frameworks like jQuery, facebook like buttons, google maps, etc.) cannot opt into using pointer events without consent from the page owner (since it could break other code on the page), and worse the page can't opt into using pointer events without breaking the touch-event based code in such libraries.  This could cause a deadlock preventing the majority of the web from transitioning to pointer events.

Some more discussion on this topic in my notes here: https://docs.google.com/a/chromium.org/document/d/1Sasl1qYJV6agrDvGplEYlZznzc38U-TFN_3a67-nlSc/edit.  Feel free to add comments there, or discuss further on the touch events community list (www.w3.org/community/touchevents/).
(In reply to Nick Lebedev (MS) from comment #36)
> So if we
> make one kind of events generated from another one we may get the behavior
> that’s not according to the spec. In case you described - we will have
> incorrect behavior of touch events.

While that's true, I think there must be some sort of "super-pointer-events" that we can dispatch from the widget code internally, which allows both W3C-compatible touch events and pointer events to be generated from the event state manager. This should also avoid having the mode switch. I would favour this approach over the one you have because of the concerns Rick brought up in the above comment - I feel like it will be impractical to just "turn off" touch events unless we are explicitly asked to do that by the page content.
So I think we need to support both touch and pointer events simultaneously.
The main issue is perhaps whether touch-action affects to touch events, and for backwards compatibility
it shouldn't, at least by default.
(we may want to add touch-action-delay or similar later, but IMO the plan should be to get
web devs to use pointer events + touch-action, and gradually deprecate touch events)

Rick, what are the plans in Blink.
That sounds like a good plan Olli.

For Blink we're being a bit more conservative - there is concern about the risk of committing to pointer events when not all major browsers will support it (Safari being the key exception).  We're starting by making touch-action work for touch events (with touch-action-delay), which should enable great pointer event polyfills (touch-action being the major stumbling block for PE polyfills today).  Then we'll look for clear demand from web developers (eg. via adoption of polyfills) and consider adding native support for the events then.
I'm just worried that if we have touch-action-delay, people continue using touch events +
touch-action-delay (which feels like a hack) and touch-action, and not move to use
pointer events.
Yes, I'm worried about that as well (although I don't consider touch-action support for touch events to be a hack - other than having to opt-in to the fast mode, it's reasonably elegant).  But as long as Safari doesn't support pointer events then developers are going to have to do something to continue to use touch events.  So focusing on ensuring the best polyfills possible makes some sense.  See https://groups.google.com/a/chromium.org/d/msg/blink-dev/K1qk6qZWgIc/P4JkKQDVez0J for more discussion.

There are other reasons to prefer pointer events over touch events though (and in fact when I talk to average web developers about touch events vs. pointer events, it's these other reasons that they focus on, not touch-action - in particular they like being able to handle touch and mouse with the same event types).  So I feel pretty confident about the path forward...
touch-action is not a hack but touch-action-delay is. And there are reasons for that hack, 
like the default value of touch-action. But it is still a hack :)

Also, until mobile Safari has touch-action and touch-action-delay, script libraries need to handle
non touch-action/delay cases rather well.
Comment on attachment 825379 [details] [diff] [review]
Apzc part of touch-action implementation

Review of attachment 825379 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +792,5 @@
>      SetState(PANNING);
> +  } else if (IsCloseToHorizontal(angle, AXIS_LOCK_ANGLE)) {
> +    mY.SetScrollingDisabled(true);
> +    if (mX.Scrollable()) {
> +      if (mPointerEventsMode && touchAction == TouchActionValue::PAN_X) {

I think you should check the touchAction before checking the angle.  If touch-action is "pan-x" then we should enter PANNING_LOCKED_X regardless of the angle.  (Similar for pan-y.)  If touch-action is "none" then we should disable scrolling completely regardless of angle.

The angle check should matter only if touch-action is "auto".
Attachment #825379 - Attachment is obsolete: true
Attachment #829349 - Flags: feedback+
Attachment #789734 - Attachment is obsolete: true
Attachment #789734 - Flags: feedback?(roc)
Attachment #829352 - Flags: feedback+
Attachment #829349 - Attachment description: apzc_part → Part 1. Added panning restriction and retrieving of touch-action value to apzc.
Attachment #829350 - Attachment description: apzc_zoom_part → Part 2. Added zoom prevention based on the touch-action value.
Attachment #829351 - Attachment description: widget_part → Part 3. Changed order of dispatching touch events according to the started/not started default behavior in the apzc.
Attachment #829352 - Attachment description: css_part → Part 4. Added touch-action css property declaration.
Just updated the patches, the main things that are done since the previous submit:
1) Added zoom prevention based on touch-action value
2) Added getting touch-action value for each touch of the touch-event
3) Made touch-action property non-inheritable
4) Added hit testing fallback
  (The logic that would responsible for retrieving touch-action value at apzc is the logic tracked by   ticket https://bugzilla.mozilla.org/show_bug.cgi?id=928833. In some rare cases that logic might not work and for it we’re going to use fallback that will ask content for hit testing. More info in the ticket mentioned above and also here: https://bugzilla.mozilla.org/show_bug.cgi?id=928839
(Btw, fallback code is far from production quality bar, just a proof of concept that it works)

Things i need to fix/resolve:
1) Restrict touch-action property to only block-level and svg elements
2) Add a kind of “AbsractWidgetTouch/PointerEvent” that will be a parent for the WidgetTouchEvent and WidgetPointerEvent. Based on the instantiated subclass we will convert event to the appropriate one at the DOM/Content level.
3) Merge/Intersect logic of touch and pointer events.
4) Refactoring of the panning logic in the AsyncPanZoomController and of the hit testing fallback.
5) Fixing small issues with zoom prevention.
6) Implement tests (I believe mochitests will be required here).
Please set the feedback flag to "?" and specify the person you want feedback from, in case you are looking for feedback. Most people get so much bugmail that they're probably not going to look at this unless you explicitly request it.
Guys, does someone have a concern if I proceed with implementing of the approach suggested by Rick (https://docs.google.com/document/d/1CV2AXyrdPdGSRypAQcfGrgQVuWYi50EzTmVsMLWgRPM/edit#)?

That looks reasonable + we could have provide similar experience on both Firefox and Blink (at least until another spec/standard adopted).

Btw, Maybe slight "optimization" can also fit there: detect touch listeners and if there are no of them - make elements have touch-action-delay: none by default to make panning/zooming more smooth.
Flags: needinfo?(rbyers)
Flags: needinfo?(oleg.romashin)
Flags: needinfo?(mbrubeck)
Flags: needinfo?(jmathies)
Flags: needinfo?(dholbert)
Flags: needinfo?(bugs)
Flags: needinfo?(bugmail.mozilla)
Working together on a common design would be great!  This would also help validate that my design works well in a browser that natively supports both types of events (since blink will be shipping touch-action support before having any native support for pointer events proper).

Nick, I've given you edit access to that doc - feel free to re-organize it to be less bink-specific or otherwise improve it if you like (perhaps we should split design and impl details into separate docs, but many of the issues raised in the impl details section probably apply to both engines).

I may not have said it explicitly, but yes I expect we'll effectively treat everything as touch-action-delay: none when there is no touch handlers in the document.  This is a generalization of an existing optimization we have, and the goal was the two touch-action-delay settings should behave identically in the cases where there are no touch handlers (eg. for compat with IE).  Note that we'll probably go even further and do location-specific hit-testing - i.e. if you touch on a region of the page that has no touch handlers, then we implicitly treat it as touch-action-delay: none (since even if we're wrong, behavior should be indistinguishable except for the perf improvement).
Flags: needinfo?(rbyers)
Note that we should make sure not to actually enable touch-action-delay support by default in a shipping browser until we've got some on the design consensus at the W3C.  It's sufficiently complicated that I felt I couldn't support an official standard until I had a working prototype, but for blink we won't ship support for the API (outside of our --enable-experimental-web-platform-features flag) until there's some agreement within the W3C in some context (ideally a spec in CR phase, perhaps earlier).  Eg. I haven't worried too much at this stage about trivial details which may change during standardization - like the precise name of the property.
I skimmed through the design doc and I agree with smaug that the touch-action-delay seems like a bit of a hack and I would rather not have to put that in unless it's absolutely needed.
Flags: needinfo?(bugmail.mozilla)
If you don't add something like touch-action-delay, then it'll mean that sites that use touch-action and _mostly_ use pointer events but still have some touch event handlers (eg. maybe an embedded ad) can be stuck blocking scrolling on javascript (waiting for the disposition of touchstart events).  Are you OK with the perf implications of that?

In blink we want some global way that a site can opt-into a fast-scroll mode (and eventually we'll probably start to punish sites that don't - eg. by having a smaller and smaller touch event timeout).

I don't think this necessarily needs to be decided now (should be possible to add later once there's more data without breaking compat).  My main concern here is the path for shipping touch-action in blink.  We're really only willing to implement touch-action if it can be used to speed up sites using touch events, but we also generally don't add new web platform features without some support from other major browsers.

Would you still be willing to work together at the W3C to standardize something like touch-action-delay (happy to change the details), even if you choose to postpone the decision whether to implement it Gecko until there's more data?
touch-action-delay isn't exactly backwards compatible. If some script library expects to
be able to use touch events, it doesn't expect touch events to not be there in certain areas of the
page. That is my main concern.
Flags: needinfo?(bugs)
touch-action-delay doesn't change what touch events get dispatched.  It just changes whether scrolling/zooming is blocked until the touch events have been handled (in order to check their disposition).  Eg. if a library is listening for touch events to detect a tap (eg. with FastClick), then touch-action-delay: none will have no impact on it.

But I agree that disabling the touch-action delay isn't backwards compatible - some code may expect to be able to disable scrolling by calling preventDefault on touchstart, and this breaks in a touch-action-delay: none region (touch-action must be used for that purpose instead).  That's why we need to get sites to opt into it (otherwise we could just do it automatically without a new CSS property).

There's the potential for different components in a page to disagree over it, but it's no worse than the fact that multiple handlers from different libraries can capture events (and disagree on whether preventDefault should get called, or even hide events from eachother with stopPropagation).
(In reply to Rick Byers from comment #57)
> touch-action-delay doesn't change what touch events get dispatched.  It just
> changes whether scrolling/zooming is blocked until the touch events have
> been handled (in order to check their disposition).
That is a huge difference to the current behavior. not "just" ;)


Since we need to get web sites to opt-in anyway, I think we should try to convince them to
use pointer events (+ touch-action), not touch events + touch-action-delay + touch-action.
(and if that doesn't work out, then introduce touch-action-delay).
Understood.  Unfortunately we feel it's prudent for blink to take a more conservative approach unless/until all major browsers are bought into this plan :-(
I think not having touch-action-delay is the more conservative approach.
(Note, I'm not saying that we definitely should not have touch-action-delay.
 But it is a hack, and if we could not introduce that to the web platform, the better.)
Attachment #829349 - Attachment is obsolete: true
Attachment #8338631 - Flags: review?(mbrubeck)
Attachment #8338631 - Flags: review?(bugmail.mozilla)
Attachment #829350 - Attachment is obsolete: true
Attachment #8338632 - Flags: review?(mbrubeck)
Attachment #8338632 - Flags: review?(bugmail.mozilla)
Attachment #829352 - Flags: feedback+ → feedback?(dbaron)
Flags: needinfo?(jmathies)
Flags: needinfo?(oleg.romashin)
Flags: needinfo?(mbrubeck)
Flags: needinfo?(dholbert)
Attachment #8338631 - Flags: review?(botond)
Attachment #8338632 - Flags: review?(botond)
Attachment #8338631 - Attachment description: Part 1. Added panning restriction and retrieving of touch-action value to apzc. → Part 1. Added panning restriction and retrieving of touch-action value to apzc. v2.
Attachment #8338632 - Attachment description: Part 2. Added zoom prevention based on the touch-action value. → Part 2. Added zoom prevention based on the touch-action value. v2.
Attachment #8338633 - Attachment description: Part 3. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. → Part 3. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. v2.
Guys, sorry for spamming, did a really stupid thing by editing patch as comment :). Is there a way to remove/edit comments in bugzilla?

Btw, the main thing i've added in these patches update is preff'ing touch-action logic. By default preference is false.
(In reply to Nick Lebedev (MS) from comment #67)
> Guys, sorry for spamming, did a really stupid thing by editing patch as
> comment :). Is there a way to remove/edit comments in bugzilla?

There is a way, but I think it's reserved for extreme cases. (And don't feel bad; I've made the same "edit as comment" mistake, as have many others :))

I've marked the comment as Private (security-group-visible only), though, as a partial solution, to prevent it from making this bug a zillion pixels tall.
Comment on attachment 8338633 [details] [diff] [review]
Part 3. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. v2.

Review of attachment 8338633 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +484,5 @@
> +  return false;
> +}
> +
> +TouchAction
> +MetroInput::HitTestContent(nsIntPoint& aPoint)

Why are we doing this work from widget, can we store this info in the frame metrics structure that nsDisplayList populates when the display list is built?

Due to the logic here related to mContentConsumingTouch, I can understand the need to query the apzc tree manager for touch action info, but I don't understand why widget needs to be involved in retrieving this style info.
Thanks for comment Jim,
Yes, we can store it in frameMetrics and we will do (i hope) but it won't solve all the cases.

To clarify the situations let me describe the whole algo shortly. We would have two ways to retrieve touch-action value: first one is to use touch-sensitive regions that would be constructed in nsDisplayList, would keep touch-action values and would be attached to frame metrics. They aren't implemented yet (because of it i'm returning unknown value in the apzc currently). But this approach may not able to retrieve touch-action value, for example when frames have complex shapes and can't be represented by  regions. And in this case we should switch to second approach (a fallback approach) that would ask content from widget to perform hit testing and retrieve touch-action css info.

More info can be found here: https://bugzilla.mozilla.org/show_bug.cgi?id=928833#c0. The first comment shortly explains the situation.
Comment on attachment 8338633 [details] [diff] [review]
Part 3. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. v2.

Review of attachment 8338633 [details] [diff] [review]:
-----------------------------------------------------------------

minusing for now until we solidify the methodology for informing the apzc and widget about touch action data.

::: widget/windows/winrt/MetroInput.cpp
@@ +1202,5 @@
>  //#define DUMP_TOUCH_IDS(aTarget, aEvent) DumpTouchIds(aTarget, aEvent)
>  #define DUMP_TOUCH_IDS(...)
>  
>  void
> +MetroInput::handleFirstTouchStartEvent(WidgetTouchEvent* event)

I like breaking these out. Unfortunately various patches have landed in this area so you'll need to update your patch here, I can't get them to apply cleanly.

nit - *H*andleFirstTouchStartEvent

@@ +1216,5 @@
> +      transformedEvent.touches[i]->mRefPoint.x,
> +      transformedEvent.touches[i]->mRefPoint.y));
> +  }
> +
> +  nsTArray<TouchAction>& touchActionValues = GetTouchActionValues(touchPoints);

Can this take the transformed WidgetTouchEvent and work directly with the data it contains rather than making a copy of the touch points? If that isn't optimal, lets try to add a helper to WidgetTouchEvent for retrieving the array.

@@ +1218,5 @@
> +  }
> +
> +  nsTArray<TouchAction>& touchActionValues = GetTouchActionValues(touchPoints);
> +  
> +  // Setting the touch action values to the apzc that will be responsible

nit - white space above this

@@ +1222,5 @@
> +  // Setting the touch action values to the apzc that will be responsible
> +  // for touch behavior. It may be not the same apzc we retrieved touch
> +  // action values from. E.g. for zooming we're taking parent apzc of a few ones
> +  // that were touched but touch-action values would be taken from childs.
> +  mWidget->ApzcSetTouchActionValue(mTargetAPZCGuid, touchActionValues);  

nit - ws

@@ +1250,5 @@
> +  mRecognizerWantsEvents = !(nsEventStatus_eConsumeNoDefault == contentStatus);
> +}
> +
> +void
> +MetroInput::handleFirstTouchMoveEvent(WidgetTouchEvent* event)

nit - *H*andleFirstTouchMoveEvent

@@ +1258,5 @@
> +  nsEventStatus contentStatus;
> +  nsEventStatus apzcStatus;
> +
> +  WidgetTouchEvent transformedEvent(*event);
> +  apzcStatus = mWidget->ApzReceiveInputEvent(event, &mTargetAPZCGuid, &transformedEvent);

nit - you're missing some helpful logging here, please add appropriate DUMP_TOUCH_IDS statements when sending events to the dom or apzc.

@@ +1259,5 @@
> +  nsEventStatus apzcStatus;
> +
> +  WidgetTouchEvent transformedEvent(*event);
> +  apzcStatus = mWidget->ApzReceiveInputEvent(event, &mTargetAPZCGuid, &transformedEvent);
> +  // We need to dispatch here only touch event, not pointer one.

If I'm reading this right, this is a TODO: comment since we're not supporting WidgetPointerEvent yet in this work?

@@ +1269,5 @@
> +  // 1) Create two separate instances of the WidgetTouchEvent and WidgetPointerEvent and
> +  // dispatch them separately.
> +  // 2) Add a boolean flag to the WidgetTouchEvent that states whether this event should produce
> +  // both touch and pointer event or only touch one.
> +  mWidget->DispatchEvent(mChromeHitTestCacheForTouch ? event : &transformedEvent, contentStatus); 

nit - ws

@@ +1341,4 @@
>    // Test for chrome vs. content target. To do this we only use the first touch
>    // point since that will be the input batch target. Cache this for touch events
>    // since HitTestChrome has to send a dom event.
> +  if (mCancelable && event->message == NS_TOUCH_START) {

why did you remove the |mTouches.Count() == 1| check here?

::: widget/windows/winrt/MetroInput.h
@@ +298,5 @@
> +  void handleFirstTouchStartEvent(WidgetTouchEvent* event);
> +  void handleFirstTouchMoveEvent(WidgetTouchEvent* event);
> +
> +  // void DeliverTouchEvent(WidgetTouchEvent* event);
> +  // void DeliverPointerEvent(WidgetTouchEvent* event);

nit - please don't add these types of comments. We'll get to adding this when we implement the rest of pointer events.

::: widget/windows/winrt/MetroWidget.cpp
@@ +1019,5 @@
> +bool
> +MetroWidget::IsTouchListenersPresented()
> +{
> +  LogFunction();
> +  return (APZController::sAPZC && 

nit - ws
Attachment #8338633 - Flags: review?(jmathies) → review-
(In reply to Nick Lebedev (MS) from comment #70)
> Thanks for comment Jim,
> Yes, we can store it in frameMetrics and we will do (i hope) but it won't
> solve all the cases.
> 
> To clarify the situations let me describe the whole algo shortly. We would
> have two ways to retrieve touch-action value: first one is to use
> touch-sensitive regions that would be constructed in nsDisplayList, would
> keep touch-action values and would be attached to frame metrics. They aren't
> implemented yet (because of it i'm returning unknown value in the apzc
> currently). But this approach may not able to retrieve touch-action value,
> for example when frames have complex shapes and can't be represented by 
> regions. And in this case we should switch to second approach (a fallback
> approach) that would ask content from widget to perform hit testing and
> retrieve touch-action css info.
> 
> More info can be found here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=928833#c0. The first comment
> shortly explains the situation.

Ok so this is blocked by that bug. Anyone know what the plan is here, are we planning on landing this work first (in which case the fallback approach would become the default) or is this work truly blocked by bug 928833.

I'd be ok with landing this as initial work provided we can tie the support logic here to a pref that defaults to off initially.
Yes I think we should land this as initial work.
I've updated patches after addressing all codereview comments related to widget part. The things that i've changed are:

* Merged with today's upstream. Logic in HandleFirstTouchStart/Move methods changed much and so might required closer look.

* Refactored getting touch-action values via direct passing WidgetTouchEvent object to the APZCTreeManager (and without creating additional array).

* Sorry for nits, fixed all whitespaces and similar stuff.

* I didn't remove touches.Count() check, it was already removed, you can check it at mxr: http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#1153.

btw, thanks for review.
Attachment #8338633 - Attachment is obsolete: true
Attachment #8342471 - Flags: review?(jmathies)
Comment on attachment 8338631 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v2.

Review of attachment 8338631 [details] [diff] [review]:
-----------------------------------------------------------------

Setting f+ because I'm not an official reviewer of this code.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +46,5 @@
> +bool
> +APZCTreeManager::GetMayHaveTouchListeners()
> +{
> +  return mMayHaveTouchListeners;
> +}

I don't think this will be useful as a member of APZCTreeManager, which might manage APZCs from several different browser tabs.  I think we need to track this per-APZC or at least per "root" APZC, or something like that.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +533,5 @@
> +        : APZCTreeManager::TouchActionValue::AUTO;
> +
> +      return (APZCTreeManager::TouchActionValue::AUTO == touchActionValue
> +         ? nsEventStatus_eConsumeNoDefault
> +         : status);

For clarity, I would fine this slightly more readable:

  if (!mTouchActionValues.length || mTouchActionValues[0] == AUTO) {
    return nsEventStatus_eConsumeNoDefault;
  }
  return status;

(This would replace the 7 lines above.)

@@ -807,2 @@
>      } else {
> -      SetState(CROSS_SLIDING_X);

Since this removes the hack we currently use for "cross-sliding" on the Metro Firefox start page, we'll need to fix that page to use touch-action instead when this lands.  (This is just a reminder to myself to get that ready in a separate patch.)

You can also delete the CROSS_SLIDING_* constants and any references to them.

@@ +886,1 @@
>    // Don't consume an event that starts a cross-slide.

This comment needs to be updated.

@@ +1568,5 @@
> +  }
> +}
> +
> +void AsyncPanZoomController::CleanTouchSession() {
> +  mTouchActionValues.Clear();

Nit: I would prefer "Clear" in the method name instead of "Clean", for consistency.
Attachment #8338631 - Flags: review?(mbrubeck) → feedback+
Attachment #8338632 - Flags: review?(mbrubeck) → feedback+
Comment on attachment 8342471 [details] [diff] [review]
Part 3. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. v3.

Review of attachment 8342471 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +489,5 @@
> +  return false;
> +}
> +
> +TouchAction
> +MetroInput::HitTestContent(const nsIntPoint& aPoint)

Can we move this logic into the apzc tree manager so that we only make the ApzcGetTouchActionValue call from widget?
I think we can't (i could be missing smth) since APZCTM is not aware of the content. APZCTM knows only about layers and frame metrics info.

We possibly could move this logic to another direction - deeper to the content, to Layout or Dom utils.

I agree that currently it doesn't look well since we operate with frames/styles in the widget code. Could you let me know who is the right person to ask about it and possibly request needinfo.
(In reply to Nick Lebedev (MS) from comment #77)
> I think we can't (i could be missing smth) since APZCTM is not aware of the
> content. APZCTM knows only about layers and frame metrics info.
> 
> We possibly could move this logic to another direction - deeper to the
> content, to Layout or Dom utils.
> 
> I agree that currently it doesn't look well since we operate with
> frames/styles in the widget code. Could you let me know who is the right
> person to ask about it and possibly request needinfo.

Kats mentioned that eventually he'd like all hit testing handled by the apzc code. From my understanding this code is temporary until the real hit testing gets worked on in bug 928833. If that's the case, seems reasonable this should go in apzc.

kats or roc might have some ideas here..
Comment on attachment 8338631 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v2.

Review of attachment 8338631 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +54,5 @@
> +{
> +  touchActionValues.Clear();
> +
> +  for (size_t i = 0; i < hittestPoints.Length(); i++) {
> +    nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(hittestPoints[i]);  

trailing whitespace

@@ +64,5 @@
> +  return touchActionValues;
> +}
> +
> +void
> +APZCTreeManager::SetTouchActionValue(const ScrollableLayerGuid& aGuid, 

Ditto

@@ +119,5 @@
> +    ContainerLayer* container = aRoot->AsContainerLayer();
> +    if (container) {
> +      // Not sure that taking touch listeners from the root is enough.
> +      // Very likely we need to iterate through the tree and perform OR
> +      // of all layers' mMayHaveTouchListeners values.

It's not enough, but as I say in a later comment I don't think doing this even makes sense.

::: gfx/layers/composite/APZCTreeManager.h
@@ +209,5 @@
>    static void SetDPI(float aDpiValue) { sDPI = aDpiValue; }
>  
> +   /**
> +   * Returns a flag that specifies whether current document has touch event listeners
> +   * or not. Intented to be called by widget to make specific decisions about using 

s/Intented/Intended/. Also trailing whitespace

@@ +212,5 @@
> +   * Returns a flag that specifies whether current document has touch event listeners
> +   * or not. Intented to be called by widget to make specific decisions about using 
> +   * touch-action css property and dispatching events to the content.
> +   */
> +  bool GetMayHaveTouchListeners();

There is only one APZCTreeManager, and it doesn't know what the "current document" is. I don't think this function belongs on this class. I haven't looked at the rest of the patches so I don't know what you're planning on using this for so I can't suggest any alternative yet.

@@ +214,5 @@
> +   * touch-action css property and dispatching events to the content.
> +   */
> +  bool GetMayHaveTouchListeners();
> +
> +  // We might need another enum that will set of flags (like zoom availabe,

"that will set of flags"?
s/availabe/available/

@@ +216,5 @@
> +  bool GetMayHaveTouchListeners();
> +
> +  // We might need another enum that will set of flags (like zoom availabe,
> +  // horizontal/vertical pan available) that would be degrees of 2 so that
> +  // we could apply & operation to them.

not sure what "apply & operation" means

@@ +217,5 @@
> +
> +  // We might need another enum that will set of flags (like zoom availabe,
> +  // horizontal/vertical pan available) that would be degrees of 2 so that
> +  // we could apply & operation to them.
> +  // Such thing would be more convinient and expandable for future css

typo: convenient

@@ +232,5 @@
> +   * Returns vaue of touch action css property for the apzc.
> +   * Internally performs asks appropriate AsyncPanZoomController to perform
> +   * hit testing on its own.
> +   */
> +  nsTArray<TouchActionValue>& GetTouchActionValue(nsTArray<ScreenIntPoint> &hittestPoints);

Not sure why this function deals in arrays rather than just a single point at a time.

@@ +289,5 @@
>                         nsTArray< nsRefPtr<AsyncPanZoomController> >* aOutRootApzcs);
>    void GetInputTransforms(AsyncPanZoomController *aApzc, gfx3DMatrix& aTransformToApzcOut,
>                            gfx3DMatrix& aTransformToGeckoOut);
>  private:
> +  nsTArray<TouchActionValue> touchActionValues;

This should be local to the method that uses it. No need to have it as an instance variable.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +96,5 @@
>   */
>  static const double AXIS_BREAKOUT_ANGLE = M_PI / 8.0; // 22.5 degrees
>  
>  /**
> + * Angle from axis the the line drawn by pan move.

"the the"

@@ +529,5 @@
>        }
>  
> +      APZCTreeManager::TouchActionValue touchActionValue = mTouchActionValues.Length()
> +        ? mTouchActionValues[0]
> +        : APZCTreeManager::TouchActionValue::AUTO;

If mTouchActionValues is only populated after gecko-based hit testing, it seems unlikely that we can rely on that value being ready here. Gecko might be busy running a script and may not respond right away. I'm thinking we should use an architecture similar to what exists for touch event listeners and ContentReceivedTouch. That is, enter a state WAITING_HIT_DETECTION or something like that where we can explicitly know whether or not gecko has responded.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +13,5 @@
>  #include "mozilla/Monitor.h"
>  #include "mozilla/ReentrantMonitor.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/Atomics.h"
> +#include "mozilla/layers/APZCTreeManager.h"  // for ScrollableLayerGuid

ScrollableLayerGuid has been moved into FrameMetrics.h; when you rebase you should take out this include as it is not needed any more.

@@ +295,5 @@
> +   * This method is invoked by the APZCTreeManager which in its turn invoked by
> +   * the widget after performing hit testing fallback.
> +   * By fallback the hit testing by content is meant (which has high latency).
> +   */
> +  void SetTouchActionValue(const nsTArray<APZCTreeManager::TouchActionValue> &values);

I don't understand why this takes an array instead of just a single value. It looks like the code you wrote only ever reads the first item from this array anyway.

That being said, I would prefer to merge this into the ContentReceivedTouch method. Basically replace both this and ContentReceivedTouch with a new method that dictates the behaviour of the current "touch session" as you call it.

@@ +608,5 @@
>    AxisX mX;
>    AxisY mY;
>  
> +  nsTArray<APZCTreeManager::TouchActionValue> mTouchActionValues;
> +  

trailing whitespace
Attachment #8338631 - Flags: review?(bugmail.mozilla) → review-
Sorry, the above review may have been premature. I think I misunderstood the way you implemented this, I will look at it again.
In the meantime though...

(In reply to Nick Lebedev (MS) from comment #77)
> I think we can't (i could be missing smth) since APZCTM is not aware of the
> content. APZCTM knows only about layers and frame metrics info.

That is correct. I think this code does make sense to be in the widget part of the codebase, but as it will be very similar across different platforms I would like to pull it out into a helper class similar to APZCCallbackHelper. In the case B2G we'll need to invoke it from TabChild.cpp so it makes sense to keep the code somewhere reusable; widget/xpwidgets would be my preferred location.
Attachment #8342471 - Flags: review?(jmathies) → feedback+
Attachment #8338631 - Attachment is obsolete: true
Attachment #8338631 - Flags: review?(botond)
Attachment #8338632 - Attachment is obsolete: true
Attachment #8338632 - Flags: review?(bugmail.mozilla)
Attachment #8338632 - Flags: review?(botond)
Attachment #829352 - Attachment is obsolete: true
Attachment #829352 - Flags: feedback?(dbaron)
Hi Kats, thanks for review. I've updated my patches, the things that i've changed are:

* Fixed all the nits: whitespaces, typos.
* Removed retrieving of mayHaveTouchListeners value. This value is required for performance optimization for case when there are no touch listeners at the content. Can we skip this optimization for now since it may introduce complexities for the b2g and other platforms (as i understand)(1) and touch-action logic is hidden by the preference(2).
* Moved content hit testing to the separate patch.

A few replies:
> If mTouchActionValues is only populated after gecko-based hit testing, it seems unlikely that we can > rely on that value being ready here. Gecko might be busy running a script and may not respond right > away. I'm thinking we should use an architecture similar to what exists for touch event listeners > > and ContentReceivedTouch. That is, enter a state WAITING_HIT_DETECTION or something like that where > we can explicitly know whether or not gecko has responded.

Actually we can rely on mTouchActionValues since the method SetTouchActionValues will always be called before ContentReceivedTouch and therefore mTouchActionValues won't be null/empty when we access it. 
This safety is also provided by the apzc's state machine. We will be already in the touching state and already passed waiting_listener state when handle touch moves and access to touch-action values.
The only thing is that we could rename WAITING_LISTENERS state to smth more general, like WAITING_CONTENT_RESPONSE or smth. Please let me know you consideration about it.

> I don't understand why this takes an array instead of just a single value. It looks like the code
> you wrote only ever reads the first item from this array anyway.

Yes, for panning i'm using only touch-action value only of the first touch. But for zooming spec implies all the touches to have touch-action value to be AUTO. It is the reason why i keep array to touch-action values, not the single value. The zoom logic is kept in the part 2 patch and it uses the whole array.

>That being said, I would prefer to merge this into the ContentReceivedTouch method. Basically  >replace both this and ContentReceivedTouch with a new method that dictates the behaviour of the >current "touch session" as you call it.

Yes, merging them make sense. The only thing is that i will have to introduce a member variable for it to the metroinput that will keep touch-action value since i'm retrieving touch-action value in the handleFirstTouchStart method, but i may need to call "our new method" in handleFirstTouchStart or handleFirstTouchMove.

One more question i would like to ask - what would be a good place for touch-action enum? Currently it lives in the APZCTreeManager and so i have to reference apzctm from metroinput, metrowidget, apzc, contenthelper. Not sure that it is good approach.

Btw, thanks for comments, they were helpful.
Attachment #8344603 - Flags: review?(bugmail.mozilla)
Attachment #8344604 - Flags: review?(bugmail.mozilla)
Attachment #8344605 - Flags: review?(bugmail.mozilla)
Comment on attachment 8344603 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v3.

Review of attachment 8344603 [details] [diff] [review]:
-----------------------------------------------------------------

I looked through the first four patches in the set, and it's looking pretty good. I have one question from the architectural point of view which I'll discuss in my comments on part 4, but otherwise the rest of my comments are just stylistic/documentation nits and a few suggestions for code simplification here and there.

::: browser/metro/profile/metro.js
@@ +42,5 @@
>  pref("layers.async-pan-zoom.enabled", true);
>  pref("layers.componentalpha.enabled", false);
>  
>  // Prefs to control the async pan/zoom behaviour
> +pref("apz.touch_start_tolerance", "0.001"); // dpi * tolerance = pixel threshold

Is this change intentional?

::: gfx/layers/composite/APZCTreeManager.h
@@ +235,5 @@
> +                            nsTArray<APZCTreeManager::TouchActionValue>& aOutValues);
> +
> +  /**
> +   * Sets touch-action value for current touch-session for specific apzc (determined by guid).
> +   * (should be invoked by the widget).

Mention that the TouchActionValues in the list correspond to the different touch points that are currently active.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +618,5 @@
>          return nsEventStatus_eIgnore;
>        }
>  
> +      if (mTouchActionPropertyEnabled &&
> +          APZCTreeManager::TouchActionValue::AUTO == mTouchActionValues[0]) {

If SetTouchActionValues is guaranteed to be called before this runs, then I think it makes sense to get rid of ClearTouchSession() as a separate function, and instead just do mTouchActionValues.Clear() at the top of SetTouchActionValues. I'm a little uncomfortable with just blindly appending stuff in SetTouchActionValues without knowing what's in mTouchActionValues already, and making this change will also solve that.

@@ +964,5 @@
> +      SetState(PANNING_LOCKED_X);
> +      mPanDirRestricted = true;
> +    } else {
> +      // Don't treat these touches as pan/zoom movements since 'touch-acion' value
> +      // require it.

s/acion/action/
s/require/requires/

@@ +1742,5 @@
> +
> +void AsyncPanZoomController::SetTouchActionValues(const nsTArray<APZCTreeManager::TouchActionValue>& aValues)
> +{
> +  for (size_t i = 0; i < aValues.Length(); i++) {
> +    mTouchActionValues.AppendElement(aValues[i]);

See my comment above. I'd like to merge ClearTouchSession into the start of this function.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +298,5 @@
> +   * Internally performs a kind of hit testing based on the regions constructed
> +   * on the main thread and attached to the current scrollable layer. Each of such regions
> +   * contains info about touch action css property. If regions info isn't enough it returns
> +   * UNKNOWN value and we should switch to the fallback.
> +   * IMPORTANT NOTE: for now it's only a stub and returns hardcoded magic value.

I would replace this "IMPORTANT NOTE" line with a "TODO" comment inside the function implementation.

@@ +622,5 @@
>    ReentrantMonitor mMonitor;
>  
> +  // Specifies whether we should use touch-action css property. Initialized from
> +  // the preferences.
> +  bool mTouchActionPropertyEnabled;

Is there a reason you use a local mTouchActionPropertyEnabled instead of just using gTouchActionPropertyEnabled? If so you should document it here.

@@ +625,5 @@
> +  // the preferences.
> +  bool mTouchActionPropertyEnabled;
> +
> +  // Values of touch-action css properties specific for current touch session.
> +  nsTArray<APZCTreeManager::TouchActionValue> mTouchActionValues;

Mention that there is one TouchActionValue for each active touch point.

@@ +649,5 @@
>    AxisY mY;
>  
> +  // When we have sticky axis lock mode panning may breakthrough the locking
> +  // but touch-action css property can disable it and to track it we're using
> +  // this flag.

I would change this comment to just say:

"This flag is set to true when we are in a axis-locked pan as a result of the touch-action CSS property."

No need to talk about the sticky axis lock mode here since that's more of an implementation detail.
Attachment #8344603 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8344604 [details] [diff] [review]
Part 2. Added zoom prevention based on the touch-action value. v3.

Review of attachment 8344604 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +683,5 @@
>    APZC_LOG("%p got a scale-begin in state %d\n", this, mState);
> +
> +  if (mTouchActionPropertyEnabled) {
> +    // To allow zooming pointer events specification implies all touch points
> +    // to have touch-action value equal to AUTO.

all touch points? Or just the first two? (I don't really care about this in practice, but just curious if there's any specific definition of what a pinch is)

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +231,5 @@
>  
> +        rv = mAsyncPanZoomController->HandleInputEvent(pinchEvent);
> +        mState = (nsEventStatus_eConsumeNoDefault == rv
> +          ? GESTURE_PINCH
> +          : GESTURE_NONE);

This isn't actually necessary, right? The code you're adding to AsyncPanZoomController.cpp::OnScaleBegin will make sure that AsyncPanZoomController::mState never goes into STATE_PINCHING, which means all the PINCHGESTURE_SCALE events will be ignored. This is similar to how the !mAllowZoom codepath works.

I think I would prefer leaving the GestureEventListener in GESTURE_PINCH even if the touch-action property determines that you can't zoom, because conceptually the "gesture" the user is doing is still a pinch, and that's what the GestureEventListener is tracking. It shouldn't be concerned about whether or not the APZC actually processes the gesture or not.
Attachment #8344604 - Flags: review?(bugmail.mozilla)
Comment on attachment 8344605 [details] [diff] [review]
Part 3. Introduced ContentHelper class that will be responsible for retrieving touch-action info from content and setting it to widget. v1.

Review of attachment 8344605 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/ContentHelper.cpp
@@ +21,5 @@
> +
> +  nsPoint relativePoint = nsLayoutUtils::GetEventCoordinatesRelativeTo(
> +    aWidget,
> +    aPoint,
> +    viewFrame);

No need to have a separate line for each argument here, you can put them all on one line.

@@ +23,5 @@
> +    aWidget,
> +    aPoint,
> +    viewFrame);
> +
> +  nsIFrame *target = nsLayoutUtils::GetFrameForPoint(viewFrame, relativePoint, 2);

Use IGNORE_ROOT_SCROLL_FRAME instead of "2" please

@@ +31,5 @@
> +
> +  if (target == nearestScrollableFrame) {
> +    return ((TouchAction)target
> +        ->GetContent()
> +        ->GetPrimaryFrame()

GetContent() and GetPrimaryFrame() can return null, so you need to check for nullness.

@@ +39,5 @@
> +
> +  bool horizontalPanAvailable = true;
> +  bool verticalPanAvailable = true;
> +
> +  for (nsIFrame *frame = target;

Add a comment above this loop that says the loop is accumulating all the touch-action restrictions from applicable elements.

@@ +45,5 @@
> +    frame = frame->GetParent()) {
> +    TouchAction value =
> +      ((TouchAction)frame
> +        ->GetContent()
> +        ->GetPrimaryFrame()

Ditto. Might be better to extract this to a helper method with null checks and return TouchAction::UNKNOWN or ::AUTO if you hit nulls.

@@ +52,5 @@
> +
> +    if (value == TouchAction::NONE) {
> +      return value;
> +    }
> +    else if (value == TouchAction::PAN_X) {

Move the else up to follow the brace

@@ +55,5 @@
> +    }
> +    else if (value == TouchAction::PAN_X) {
> +      verticalPanAvailable = false;
> +    }
> +    else if (value == TouchAction::PAN_Y) {

Ditto

::: widget/xpwidgets/ContentHelper.h
@@ +8,5 @@
> +
> +#include "nsIWidget.h"
> +#include "mozilla/layers/APZCTreeManager.h"
> +
> +typedef mozilla::layers::APZCTreeManager::TouchActionValue TouchAction;

To avoid confusion I'd prefer the local name to also be TouchActionValue. Also, move this typedef inside the ContentHelper class.

@@ +16,5 @@
> +
> +class ContentHelper
> +{
> +public:
> +    static TouchAction HittestContent(nsIWidget* aWidget, const nsIntPoint& aPoint);

s/Hittest/HitTest/ (case change). This method also needs some documentation here. Just a brief overview of what it accomplishes, nothing fancy.
Attachment #8344605 - Flags: review?(bugmail.mozilla)
Comment on attachment 8344608 [details] [diff] [review]
Part 4. Changed order of dispatching touch events according to the touch-action value and started/not started default behavior in the apzc. v4.

Review of attachment 8344608 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +471,5 @@
> +  for (uint32_t i = 0; i < aOutValues.Length(); i++) {
> +    if (TouchAction::UNKNOWN == aOutValues[i]) {
> +      // performing hit testing fallback: asking content to perform hit testing itself
> +      // (in spite that this operation has high latency).
> +      aOutValues[i] = mWidget->ContentGetTouchActionValue(aTransformedEvent->touches[i]->mRefPoint);

Please correct me if I'm wrong here, I want to properly understand the architecture.

The above call to ApzcGetTouchActionValues will ask the APZC for the touch action values for the input event points. Currently that is hard-coded to UNKNOWN but once that is fully implemented it will be able to provide some values back. Then, if any of the values are unknown, this code will query content directly to find out what they should be.

On metro all this code is running on the gecko thread so you're able to call ContentGetTouchActionValue directly. Conceptually though this could be shuffled off to a dedicated input-handling thread. In that case we would want to keep ApzcGetTouchActionValue on the input-handling thread, but the calls to ContentGetTouchActionValue would be on the gecko thread. The calls to ContentGetTouchActionValue would either need to block for a return value or wait for a callback similar to ContentReceivedTouch.

Is that accurate?

If we go with the blocking approach, then we're potentially blocking the input thread for a long amount of time since Gecko could be doing anything. I would like to avoid that. If we go with the callback approach then we run into the problem that mTouchActionValues might not be populated in the APZC in the OnTouchMove handler in the mState==TOUCHING case. That's the problem I was talking about in comment 79 about not being able to rely on mTouchActionValues being available.

For now I'm not hugely concerned about this because both Metro and B2G deal with input on the gecko thread but it would be good to figure out a good solution to this problem as we will need it both when porting Fennec to use APZC and when tackling bug 930939.
Added patch with apzc tests. Still addressing kats' review comments, will update other patches according to them tomorrow.
Attachment #8345329 - Flags: review?(bugmail.mozilla)
Reply to recent kats' questions:

> >  pref("layers.async-pan-zoom.enabled", true);
> >  pref("layers.componentalpha.enabled", false);
> >  
> >  // Prefs to control the async pan/zoom behaviour
> > +pref("apz.touch_start_tolerance", "0.001"); // dpi * tolerance = pixel threshold

> Is this change intentional?

Yes. It was introduced to decrease pan threshold to make apzc trigger pan behavior right after the touchstart and first touchmove. The reason why we need this behavior is the pointer events specification - if the panning is triggered we should have only pointerdown and pointercancel (and no pointermove) events. Decreasing value to the new one panning is triggered “faster”(without additional move events) and tap isn’t considered as move at the same time.

> >    APZC_LOG("%p got a scale-begin in state %d\n", this, mState);
> > +
> > +  if (mTouchActionPropertyEnabled) {
> > +    // To allow zooming pointer events specification implies all touch points
> > +    // to have touch-action value equal to AUTO.

> all touch points? Or just the first two? (I don't really care about this in practice,
> but just curious if there's any specific definition of what a pinch is)

Actually there is not well defined info about it in the spec, so i just followed the general case.

> > +  for (uint32_t i = 0; i < aOutValues.Length(); i++) {
> > +    if (TouchAction::UNKNOWN == aOutValues[i]) {
> > +      // performing hit testing fallback: asking content to perform hit testing itself
> > +      // (in spite that this operation has high latency).
> > +      aOutValues[i] =   mWidget->ContentGetTouchActionValue(aTransformedEvent->touches[i]->mRefPoint);

> Please correct me if I'm wrong here, I want to properly understand the architecture.
> The above call to ApzcGetTouchActionValues will ask the APZC for the touch action values for
> the input event points. Currently that is hard-coded to UNKNOWN but once that is fully implemented it
> will be able to provide some values back. Then, if any of the values are unknown,
> this code will query content directly to find out what they should be.

Yes, the above is correct.

> If we go with the blocking approach, then we're potentially blocking the input thread for a long
> amount of time since Gecko could be doing anything. I would like to avoid that. If we go with the
> callback approach then we run into the problem that mTouchActionValues might not be
> populated in the APZC in the OnTouchMove handler in the mState==TOUCHING case. That's
> the problem I was talking about in comment 79 about not being able to rely on
> mTouchActionValues being available.

Yes, you might be right. And i agree that callback approach is better. To elaborate it i would suggest the following approach:
* Rename WAITING_LISTENERS state to the WAITING_CONTENT_RESPONSE

* Declare a new method that will check whether both touchActionValue (in case they’re enabled) and ContentReceivedTouch properties are set. If yes - it changes state to the TOUCHING state, otherwise stays in the WAITING_CONTENT_RESPONSE state.

* Add this new method invocation to the end of both methods - ContentReceivedTouch and SetTouchActionValues.

After such changes we should have safe accessing to mTouchActionValues in onTouchMove method since onTouchMove method is called only in TOUCHING state.

The only question i have with such approach is how to modify MetroInput code - do i need to leave as it currently is (simply query content in the method of handling first touchstart event) or move content querying into the async dispatching wrapper, like:

nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(this, &MetroInput::ContentGetTouchAction);
NS_DispatchToCurrentThread(runnable);
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8345329 [details] [diff] [review]
Part 6. Added gtests for the apzc to cover touch-action stuff. v1.

Review of attachment 8345329 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. Some minor comments below.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +53,5 @@
>  
> +  // Since touch-action-enabled property is global - setting it for each test
> +  // separately isn't safe from the concurrency point of view. To make tests
> +  // run concurrent and independent from each other we have a member variable
> +  // mTouchActionEnabled for each apzc and setter defined here.

Some of this explanation should go in AsyncPanZoomController.h.

@@ +141,5 @@
>  
> +/*
> + * Dispatches mock touch events to the apzc and checks whether apzc ignored them
> + * and DIDN'T triggered scrolling behavior.
> + */

I would rather add an extra parameter to ApzcPan that is a "bool expectPanIgnored = false" and just use that than duplicating the code here.

@@ +199,5 @@
> +    EXPECT_CALL(*mcc, SendAsyncScrollDOMEvent(_,_,_)).Times(AtLeast(1));
> +    EXPECT_CALL(*mcc, RequestContentRepaint(_)).Times(1);
> +  } else {
> +    // Apzc's TouchEnd handler triggers SendAsyncScroll event once. Since we're making two pans
> +    // - we have two touchend events and therefore have SendAsyncScroll method called two times.

This comment should be moved up to the "if" half of the block.

@@ +222,5 @@
> +    ApzcIgnorePan(apzc, tm, time, touchStart, touchEnd);
> +  }
> +  apzc->SampleContentTransformForFrame(testStartTime, &viewTransformOut, pointOut);
> +
> +  EXPECT_EQ(pointOut, aShouldTriggerScroll ? ScreenPoint(0, -(touchEnd-touchStart)) : ScreenPoint());

Would prefer to see this split into two conditions and moved into the if statement below, rather than using the ternary operator here.
Attachment #8345329 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Nick Lebedev (MS) from comment #93)
> Yes. It was introduced to decrease pan threshold to make apzc trigger pan
> behavior right after the touchstart and first touchmove. The reason why we
> need this behavior is the pointer events specification - if the panning is
> triggered we should have only pointerdown and pointercancel (and no
> pointermove) events. Decreasing value to the new one panning is triggered
> “faster”(without additional move events) and tap isn’t considered as move at
> the same time.

I think we can send the pointercancel from widget code if we know we're going to pan. That way we shouldn't have to fiddle with this threshold. The problem with changing the threshold is that it will make it much harder to do a tap because even a slight motion of your finger will abort the tap. Anyway until we have pointer events supported we don't need to change this value, right? I would prefer leaving it out of the patch.

> Yes, you might be right. And i agree that callback approach is better. To
> elaborate it i would suggest the following approach:
> * Rename WAITING_LISTENERS state to the WAITING_CONTENT_RESPONSE
> 
> * Declare a new method that will check whether both touchActionValue (in
> case they’re enabled) and ContentReceivedTouch properties are set. If yes -
> it changes state to the TOUCHING state, otherwise stays in the
> WAITING_CONTENT_RESPONSE state.
> 
> * Add this new method invocation to the end of both methods -
> ContentReceivedTouch and SetTouchActionValues.
> 
> After such changes we should have safe accessing to mTouchActionValues in
> onTouchMove method since onTouchMove method is called only in TOUCHING state.

This sounds reasonable enough but I'll think it over more once I see the code.

> The only question i have with such approach is how to modify MetroInput code
> - do i need to leave as it currently is (simply query content in the method
> of handling first touchstart event) or move content querying into the async
> dispatching wrapper, like:
> 
> nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(this,
> &MetroInput::ContentGetTouchAction);
> NS_DispatchToCurrentThread(runnable);

No, for now just leave it the way you had it with a direct call. But maybe add a comment there saying that if the input handling is moved to a dedicated thread then that is where we would do the thread split.
Flags: needinfo?(bugmail.mozilla)
Updated logic of state handling in the apzc: replace WAITING_LISTENERS state with the WAITING_CONTENT_RESPONSE state, which means we're in progress of getting both feedback from touch listeners and touch action values. And the transition to the TOUCHING state will happen only after both values are set (in case touch action property is enabled).
Attachment #8344603 - Attachment is obsolete: true
Attachment #8348232 - Flags: review?(bugmail.mozilla)
Slightly updated previous patch's logic of apzc's state machine (the part that waits for touch-action values and prevent default value from content).
Attachment #8348232 - Attachment is obsolete: true
Attachment #8348232 - Flags: review?(bugmail.mozilla)
Attachment #8349459 - Flags: review?(bugmail.mozilla)
Comment on attachment 8349459 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v5.

Review of attachment 8349459 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I believe this solves the architectural problem I brought up last time. All that's left is some stylistic issues. Please rebase the patches and address the comments I posted previously and below, and let's get this landed! :)

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +55,5 @@
> +    spt.x = touchEvent->touches[i]->mRefPoint.x;
> +    spt.y = touchEvent->touches[i]->mRefPoint.y;
> +
> +    nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(spt);
> +    aOutValues.AppendElement(apzc

Depending on how we do the hit testing you may need to apply a transformation to spt here. But we can figure that out later once we can actually do the hit testing. For now just stick a comment here with a TODO.

::: gfx/layers/composite/APZCTreeManager.h
@@ +26,5 @@
>  
>  namespace mozilla {
> +
> +enum AllowedTouchBehavior {
> +  NONE =               0,

I think I would prefer moving this enum inside the mozilla::layers namespace

@@ +29,5 @@
> +enum AllowedTouchBehavior {
> +  NONE =               0,
> +  BOTH_PAN =           1 << 0,
> +  VERTICAL_PAN =       1 << 1,
> +  HORIZONTAL_PAN =     1 << 2,

If these is a bitset then we shouldn't need BOTH_PAN, right? It should be equivalent to VERTICAL_PAN | HORIZONTAL_PAN.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +630,5 @@
>      case TOUCHING: {
>        float panThreshold = gTouchStartTolerance * APZCTreeManager::GetDPI();
>        UpdateWithTouchAtDevicePoint(aEvent);
>  
> +      nsEventStatus status = nsEventStatus_eIgnore;

This variable is unused

@@ +672,5 @@
>    APZC_LOG("%p got a touch-end in state %d\n", this, mState);
> +
> +  // In case no touch behavior triggered previously we can avoid sending
> +  // scroll events or requesting content repaint.
> +  if (NOTHING != mState)

If this is purely an optimization I would rather take it out because it clutters this patch. If it is not an optimization then the comment is misleading.

@@ +831,5 @@
>    }
> +
> +  // In case no touch behavior triggered previously we can avoid sending
> +  // scroll events or requesting content repaint.
> +  if (NOTHING != mState)

Ditto

@@ +1708,5 @@
> +
> +AsyncPanZoomController::TouchBehaviorFlags
> +AsyncPanZoomController::GetTouchBehavior(uint32_t touchIndex) {
> +  if (mAllowedTouchBehaviors.Length()) {
> +    return mAllowedTouchBehaviors[touchIndex];

Also check if touchIndex < mAllowedTouchBehaviors.Length()

@@ +1725,5 @@
> +  if (mTouchActionPropertyEnabled) {
> +    canProceedToTouchState &= mAllowedTouchBehaviorSet;
> +  }
> +
> +  if (canProceedToTouchState) {

To reduce a nesting level, please do "if (!canProceedToTouchState) { return; }" here.

@@ +1762,5 @@
> +AsyncPanZoomController::TouchBehaviorFlags
> +AsyncPanZoomController::GetAllowedTouchBehavior(ScreenIntPoint& aPoint) {
> +  // Here we need to perform a hit testing over the touch-action regions attached to the
> +  // layer associated with current apzc.
> +  // Currently they are in progress, for more info see ticket 928833.

s/ticket/bug/

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +13,5 @@
>  #include "mozilla/Monitor.h"
>  #include "mozilla/ReentrantMonitor.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/Atomics.h"
> +#include "mozilla/layers/APZCTreeManager.h"  // for TouchAction prop.

You don't need this include here. The only thing you might be including is enum AllowedTouchBehavior but you're not using that anywhere in this header file. Best to leave the #include in .cpp file and do a local typedef there if you want it.

@@ +56,5 @@
>  class AsyncPanZoomController {
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AsyncPanZoomController)
>  
>    typedef mozilla::MonitorAutoLock MonitorAutoLock;
> +  typedef mozilla::AllowedTouchBehavior AllowedTouchBehavior;

don't need this typedef.
Attachment #8349459 - Flags: review?(bugmail.mozilla) → feedback+
Comment on attachment 8349459 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v5.

Review of attachment 8349459 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1811,2 @@
>    ContentReceivedTouch(false);
> +  SetAllowedTouchBehavior(nsTArray<TouchBehaviorFlags>());

I just realized something here: we need some additional checks in this function to avoid clobbering any responses that did arrive. For example it is quite likely that when this timeout triggers, we will have gotten a call to SetAllowedTouchBehavior already, but have not yet gotten a ContentReceivedTouch call. In that case we don't want to clobber the TouchBehaviorFlags we already received. So this function should look like:

mContentResponseTimeoutTask = nullptr;
if (!mPreventDefaultSet) {
  ContentReceivedTouch(false);
}
if (!mAllowedTouchBehaviorSet) {
  SetAllowedTouchBehavior(...);
}

Does that make sense?
Attachment #8344604 - Attachment is obsolete: true
Attachment #8344609 - Attachment is obsolete: true
Attachment #8345329 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #99)
> Comment on attachment 8349459 [details] [diff] [review]
> Part 1. Added panning restriction and retrieving of touch-action value to
> apzc. v5.
> 
> Review of attachment 8349459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1811,2 @@
> >    ContentReceivedTouch(false);
> > +  SetAllowedTouchBehavior(nsTArray<TouchBehaviorFlags>());
> 
> I just realized something here: we need some additional checks in this
> function to avoid clobbering any responses that did arrive. For example it
> is quite likely that when this timeout triggers, we will have gotten a call
> to SetAllowedTouchBehavior already, but have not yet gotten a
> ContentReceivedTouch call. In that case we don't want to clobber the
> TouchBehaviorFlags we already received. So this function should look like:
> 
> mContentResponseTimeoutTask = nullptr;
> if (!mPreventDefaultSet) {
>   ContentReceivedTouch(false);
> }
> if (!mAllowedTouchBehaviorSet) {
>   SetAllowedTouchBehavior(...);
> }
> 
> Does that make sense?

Yes, makes sense, i've corrected it in the new version of patches.

I've also rebased patches to the new revision - 5c7fa2bfea8b, they seem to work fine. And also I run my patches thru the try server recently - https://tbpl.mozilla.org/?tree=Try&rev=0b0ff6433391 - no regression was mentioned. For certainty i've triggered try server builds/tests today again, will post it as soon as it done.
Attachment #8350264 - Flags: review?(bugmail.mozilla)
Attachment #8350264 - Attachment is obsolete: true
Attachment #8350264 - Flags: review?(bugmail.mozilla)
Attachment #8350301 - Flags: review?(bugmail.mozilla)
Attachment #8350265 - Attachment is obsolete: true
Attachment #8350269 - Attachment is obsolete: true
Attachment #8350305 - Flags: review?(dbaron)
Attachment #8350271 - Attachment is obsolete: true
Comment on attachment 8350301 [details] [diff] [review]
Part 1. Added panning restriction and retrieving of touch-action value to apzc. v7.

Review of attachment 8350301 [details] [diff] [review]:
-----------------------------------------------------------------

It would have been nice to separate out the non-functional changes (all the renaming from waiting_listeners to content_response) into a separate patch but whatever.

::: gfx/layers/composite/APZCTreeManager.h
@@ +210,5 @@
> +                               nsTArray<TouchBehaviorFlags>& aOutValues);
> +
> +  /**
> +   * Sets allowed touch behavior values for current touch-session for specific apzc (determined by guid).
> +   * (should be invoked by the widget).

No need to parenthesize this line. You can say "This is intended to be invoked by the widget code."

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +683,1 @@
>    {

nit: move the opening brace up to the previous line
Attachment #8350301 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8350302 [details] [diff] [review]
Part 2. Added zoom prevention based on the touch-action value. v5.

Review of attachment 8350302 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +545,5 @@
>  
>    /*
> +   * Returns whether current touch behavior values allow zooming.
> +   */
> +  bool TouchActionAllowesZoom();

s/Allowes/Allows/

Rename implementation and call sites as well.
Attachment #8350302 - Flags: review+
Comment on attachment 8350303 [details] [diff] [review]
Part 3. Introduced ContentHelper class that will be responsible for retrieving touch-action info from content and setting it to widget. v3.

Review of attachment 8350303 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing for the implicit mapping of TouchActionValue as noted below. Should be straightforward to fix, I think.

::: widget/xpwidgets/ContentHelper.cpp
@@ +20,5 @@
> +    // If frame is invalid or null then return default value.
> +    return TouchActionValue::AUTO;
> +  }
> +
> +  if (!aFrame->IsFrameOfType(nsIFrame::eSVG) & !aFrame->IsFrameOfType(nsIFrame::eBlockFrame)) {

This should use a && instead of &

@@ +27,5 @@
> +    return TouchActionValue::AUTO;
> +  }
> +
> +  return ((TouchActionValue)aFrame->GetContent()->GetPrimaryFrame()
> +                                  ->StyleDisplay()->mTouchAction);

I don't like how you're assuming the NS_STYLE_TOUCH_ACTION_xxx values defined in part 5 will always map exactly to the TouchActionValue enum defined in ContentHelper.h. There's no documentation of this and so it will be very easy for them to get out of sync. I would prefer if you just used an int32_t instead of TouchActionValue, and then in UpdateAllowedBehaviour you used the NS_STYLE_TOUCH_ACTION_xxx values instead of TouchActionValue::xxx. (This means you'll have to move part 5 up because this patch will depend on that one).

@@ +59,5 @@
> +  nsIFrame *viewFrame = view->GetFrame();
> +
> +  nsPoint relativePoint = nsLayoutUtils::GetEventCoordinatesRelativeTo(aWidget, aPoint, viewFrame);
> +
> +  nsIFrame *target = nsLayoutUtils::GetFrameForPoint(viewFrame, relativePoint,  nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME);

nit: remove one of the two spaces before the last argument.

::: widget/xpwidgets/ContentHelper.h
@@ +19,5 @@
> +class ContentHelper
> +{
> +  enum TouchActionValue {
> +    AUTO,    /* Panning in both direction is available
> +    and other touch actions like zoom/pinch/etc. are available too*/

Please indent the carryover comment lines so that they line up with the start of the comment on the previous line.
Attachment #8350303 - Flags: review-
Attachment #8350301 - Attachment is obsolete: true
Attachment #8356080 - Flags: review?(dbaron)
Attachment #8350303 - Attachment is obsolete: true
Attachment #8356098 - Flags: review?(bugmail.mozilla)
Attachment #8350304 - Attachment is obsolete: true
Attachment #8350304 - Flags: review?(jmathies)
Attachment #8356099 - Flags: review?(bugmail.mozilla)
Attachment #8350305 - Attachment is obsolete: true
Attachment #8350305 - Flags: review?(dbaron)
Attachment #8356100 - Flags: review?(bugmail.mozilla)
Attachment #8356103 - Flags: review?(bugmail.mozilla)
Comment on attachment 8356080 [details] [diff] [review]
Part 1. Added touch-action css property declaration. v5.

I'm reviewing this relative to http://www.w3.org/TR/pointerevents/#the-touch-action-css-property and https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property

># HG changeset patch
># Parent 0a000f699ef5606f6c10976e81de69bee263febd
># User Nick Lebedev <nicklebedev37@gmail.com>
>Bug 795567. Part 1: Added touch-action css property declaration. r=dbaron

Change "Added" to "Add" since commit messages should be in present tense, remove "declaration" since I don't think the term makes sense in this context, and add "to the style system" since that's what this patch is doing.

>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h
>+ CSS_PROP_DISPLAY(
>+    touch-action,
>+    touch_action,
>+    TouchAction,
>+    CSS_PROPERTY_PARSE_VALUE |
>+        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER,
>+    "layout.css.touch_action.enabled",
>+    VARIANT_HK,
>+    kTouchActionKTable,
>+    CSS_PROP_NO_OFFSET,
>+    eStyleAnimType_None)

I think it shouldn't apply to ::placeholder, so I think you should remove CSS_PROPERTY_APPLIES_TO_PLACEHOLDER.  (It would be odd for a property to apply to ::placeholder when it doesn't apply to ::first-letter.)

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
> CSSValue*
>+nsComputedDOMStyle::DoGetTouchAction()
>+{
>+  nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+  val->SetIdent(
>+    nsCSSProps::ValueToKeywordEnum(StyleDisplay()->mTouchAction,
>+                                   nsCSSProps::kTouchActionKTable));
>+  return val;
>+}

I don't see how this could correctly handle the 'pan-x pan-y' value.

>diff --git a/layout/style/nsStyleConsts.h b/layout/style/nsStyleConsts.h
>--- a/layout/style/nsStyleConsts.h
>+++ b/layout/style/nsStyleConsts.h
>@@ -738,16 +738,22 @@ static inline mozilla::css::Side operato
> // See nsStyleText
> #define NS_STYLE_TEXT_TRANSFORM_NONE            0
> #define NS_STYLE_TEXT_TRANSFORM_CAPITALIZE      1
> #define NS_STYLE_TEXT_TRANSFORM_LOWERCASE       2
> #define NS_STYLE_TEXT_TRANSFORM_UPPERCASE       3
> #define NS_STYLE_TEXT_TRANSFORM_FULLWIDTH       4
> 
> // See nsStyleDisplay
>+#define NS_STYLE_TOUCH_ACTION_AUTO            0
>+#define NS_STYLE_TOUCH_ACTION_NONE            1
>+#define NS_STYLE_TOUCH_ACTION_PAN_X           2
>+#define NS_STYLE_TOUCH_ACTION_PAN_Y           3

I don't see how this handles pan-x and pan-y being set at the same time either.

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
>--- a/layout/style/test/property_database.js
>+++ b/layout/style/test/property_database.js
>@@ -4325,16 +4325,27 @@ function get_computed_value(cs, property
> 		}
> 		return results.join(" ; ");
> 	}
> 	if (info.get_computed)
> 		return info.get_computed(cs, property);
> 	return cs.getPropertyValue(property);
> }
> 
>+if (SpecialPowers.getBoolPref("layout.css.touch_action.enabled")) {
>+    gCSSProperties["touch-action"] = {
>+        domProp: "touchAction",
>+        inherited: false,
>+        type: CSS_TYPE_LONGHAND,
>+        initial_values: ["auto"],
>+        other_values: ["none", "pan-x", "pan-y"],

Please add "pan-x pan-y" and "pan-y pan-x"

>+        invalid_values: ["zoom", "pinch", "tap", "10px", "2"]

Please add "auto pan-x", "pan-x auto", "none pan-x", "pan-x none", (and the same 4 for pan-y), and "pan-x pan-y none", "none pan-x pan-y" and the same 2 for auto instead of none.

(And, if you haven't already, make sure all the mochitests in layout/style/test/ pass with the changes.


You also need manual parsing code to support the pan-x || pan-y aspect of the syntax, rather than the flag you currently have in nsCSSPropList.h

Marking review- because it doesn't match the syntax in the spec.  (If that's intentional, please explain why in the commit message.)
Attachment #8356080 - Flags: review?(dbaron) → review-
Attachment #8356082 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8356098 [details] [diff] [review]
Part 3: Added panning restriction and retrieving of touch-action value to apzc.

Review of attachment 8356098 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1760,5 @@
> +void AsyncPanZoomController::SetAllowedTouchBehavior(const nsTArray<TouchBehaviorFlags>& aBehaviors) {
> +  mAllowedTouchBehaviors.Clear();
> +  for (size_t i = 0; i < aBehaviors.Length(); i++) {
> +    mAllowedTouchBehaviors.AppendElement(aBehaviors[i]);
> +  }

You should just be able to do mAllowedTouchBehaviours.AppendElements(aBehaviors);
Attachment #8356098 - Flags: review?(bugmail.mozilla) → review+
Attachment #8356099 - Flags: review?(bugmail.mozilla) → review+
Attachment #8356100 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8356103 [details] [diff] [review]
Part 7: Added gtests for the apzc to cover touch-action stuff.

Review of attachment 8356103 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +341,5 @@
> +
> +  apzc->SetAllowedTouchBehavior(values);
> +  ApzcPinch(apzc, 250, 300, 1.25);
> +
> +  // The frame metrics should stay the same since touch-action:none makes

Strictly speaking the values you put in the array above imply that one touch point landed on "touch-action: pan-y" and another landed on.. something impossible. (I don't think there's any touch-action value that would only allow ZOOM, is that right?)
Attachment #8356103 - Flags: review?(bugmail.mozilla) → review+
David, i've updated the patch according to your comment. Also all the mochitests are fine with this property added.
Attachment #8356080 - Attachment is obsolete: true
Attachment #8358955 - Flags: review?(dbaron)
Try build with all 7 patches applied: https://tbpl.mozilla.org/?tree=Try&rev=91518de709d4.
No regression is mentioned.
Comment on attachment 8356101 [details] [diff] [review]
Part 6: Changed order of dispatching touch events according to the touch-action value and triggered default behavior in the apzc.

Looks good, thanks for all the effort.
Attachment #8356101 - Flags: review?(jmathies) → review+
Fixed minor thing - setting SETDSC_UNSET_INITIAL while calling SetDiscrete.
Attachment #8358955 - Attachment is obsolete: true
Attachment #8358955 - Flags: review?(dbaron)
Attachment #8359206 - Flags: review?(dbaron)
(In reply to Nick Lebedev (MS) from comment #129)
> Fixed minor thing - setting SETDSC_UNSET_INITIAL while calling SetDiscrete.

Was this difference caught by a failing test?  (Which test?)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #130)
> (In reply to Nick Lebedev (MS) from comment #129)
> > Fixed minor thing - setting SETDSC_UNSET_INITIAL while calling SetDiscrete.
> 
> Was this difference caught by a failing test?  (Which test?)

Nope, I caught it by running firefox.exe manually and getting assert http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#1228 failed.
Comment on attachment 8359206 [details] [diff] [review]
Part 1: Add touch-action css property to the style system. v7.

nsCSSParser.cpp:

+  // Auto and None keywords aren't allowed in conjunction with others.
+  if (aValue.GetUnit() == eCSSUnit_None ||
+      aValue.GetUnit() == eCSSUnit_Auto) {
+    return true;
+  }
+
+  if (eCSSUnit_Enumerated != aValue.GetUnit()) {
+    return true;
+  }

Drop the first of these if-blocks and just use the second one (but
leave the comment, though fix it up to reflect that inherit/initial/unset
also count).

nsRuleNode.cpp:

>+              SETDSC_ENUMERATED | SETDSC_AUTO | SETDSC_NONE | SETDSC_UNSET_INITIAL,

Wrap this line at <80 characters.

r=dbaron with those changes
Attachment #8359206 - Flags: review?(dbaron) → review+
carrying r+
Attachment #8359206 - Attachment is obsolete: true
Attachment #8360392 - Flags: review+
carrying r+
Attachment #8356098 - Attachment is obsolete: true
Attachment #8360394 - Flags: review+
carrying r+
Attachment #8360394 - Attachment is obsolete: true
Attachment #8360395 - Flags: review+
carrying r+
Attachment #8356100 - Attachment is obsolete: true
Attachment #8360397 - Flags: review+
carrying r+
Attachment #8356103 - Attachment is obsolete: true
Attachment #8360399 - Flags: review+
Whiteboard: [checkin-needed]
Finally got patches polished and reviewed. Now it would be great if someone could checkin it on behalf of me since i have only level 1 access.

Latest tryserver results: https://tbpl.mozilla.org/?tree=Try&rev=5bf9a9febdac. No regression is mentioned.
checkin-needed is a keyword these days :) I'm sure a sheriff will get to it soon.
Keywords: checkin-needed
Whiteboard: [checkin-needed]
Curious, did we push the final set to try to be sure no tests break?
(In reply to Nick Lebedev (MS) from comment #141)
> Finally got patches polished and reviewed. Now it would be great if someone
> could checkin it on behalf of me since i have only level 1 access.
> 
> Latest tryserver results:
> https://tbpl.mozilla.org/?tree=Try&rev=5bf9a9febdac. No regression is
> mentioned.

Ah, NM!

Congrats Nick, and thanks for all the effort!
Blocks: 960316
Depends on: 964750
Whiteboard: [DocArea=CSS]
No longer blocks: 964421
Depends on: 973660
Depends on: 979345
Depends on: 1001440
Depends on: 1005937
You need to log in before you can comment on or make changes to this bug.