1001440 - Support touch-action on Windows widget

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, defect)

Description

[Makoto Kato [:m_kato]]

Since MetroFox that supports touch-action is gone, we should support this on Windows widget.  Windows backend has gesture support.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: WIP

Comment 2

[Makoto Kato [:m_kato]]

for testing , I remove pointer-event check, but it should not be removed when final.

Comment 3

[Makoto Kato [:m_kato]]

When using APZ, we always call RegisterTouchWindow().  So WM_GESTURE message isn't posted.

To support Direct Manipulation API, it may be able to detect gesture by OS level.

Comment 4

[Maksim Lebedev]

Attachment: gesture_touch_action_ver1.diff

+ ContentHelper::GetAllowedTouchBehavior() allocated for frame
+ Gestures behavior is configurated according with touch-action css property

Suggestions and comments and objections are very welcome.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8624087 [details] [diff] [review]
gesture_touch_action_ver1.diff

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

Could you replace gesture code with PanGestureInput etc?  APZC can handle CSS touch-action.

Also, WM_GESTURE only works when dom.w3c_touch_events.enabled = 0.  If to support gesture with touch API support, I think that we re-implement it using direct manipulation API

Comment 6

[Maksim Lebedev]

Unfortunately, I haven't understood your thought.
So should we implement fix for this bug or put resolved state into bug like unwanted?

Comment 7

[Makoto Kato [:m_kato]]

See bug 1101628 and https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#4962

If enabling APZC, we should use mAPZC->ReceiveInput to dispatch pan gesture.

For Zoom, we should replace with PinchGestureInput.  (no good implementation sample now)

You should ask :kats for this.

Comment 8

[Maksim Lebedev]

If APZC disabled (it happens on nsWindow without e10s) or touch_events disabled (it has special preference about it) should we use system gesture for resolving current bug? Or in otherwise we should close current bug as [wontfix] ?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I'm not following - is this about APZ enabled or APZ disabled? If APZ is enabled then nothing needs to be done, as it should already be working. If APZ is disabled then I'm not the right person to be asking as I don't really know how the touch-based scrolling works in windows, but the patch from comment 1 looks like the right approach.

Comment 10

[Maksim Lebedev]

If I correctly understand sentense "on Windows widget" - I assume that APZ is disabled, because APZ work only with PuppetWidget on e10s. So I assume that in such case system gesture works. As I correctly understand that system it uses two events: WM_GESTURENOTIFY and WM_GESTURE. At WM_GESTURENOTIFY we call SetWinGestureSupport with allowed direction. And after that all available events go through WM_GESTURE.
Who should be reviewer for current bug in that case?

Comment 11

[Jim Mathies [:jimm]]

Why are we worried about the WM_GESTURE code, isn't it going to be obsolete as soon as we turn apzc on by default?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

yeah, whatever we do here, lets focus on making sure things work right with APZ enabled.
(Things need to work correctly both in browser UI and in web pages)

Comment 13

[Maksim Lebedev]

Could You specify information about this bug? What should be done? What should be used? What You assume?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Make sure touch-action works correctly when apz is enabled. And test then both browser UI and web pages (so that both parent process and child process handling is tested).

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Oh, comment 13 was a question to Makoto, oops :)

Comment 16

[Makoto Kato [:m_kato]]

(In reply to Maksim Lebedev from comment #13)
> Could You specify information about this bug? What should be done? What
> should be used? What You assume?

You know, APZC can hadle touch-action property.  Now, even if APZC is turned on and dom.w3c_touch_events.enabled is 0 (this is default on Desktop for Web compatibility), CSS touch-action doesn't work on Windows PC with touch input.  Big reason is that gesture event isn't dispatched to APZC.

So,

If APZC is tuned on, use APZCTreeManager::ReceiveInputEvent to dispatch gesture event (Pan and Zoom) on WM_GESTURE.  Current code (see nsWindow::OnGesture) uses NS_WHEEL_WHEEL, but we should use PanGestureInput instead of WidgetWheelEvent if APZC is turned on.

I don't know whether touch-action should be supported without APZC.  Although your latest patch only consider non-APZC, you should ask smaug whether we should support it without APZC too.

smaug, should we support touch-action property without APZC?

Comment 17

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #16)
> You know, APZC can hadle touch-action property.  Now, even if APZC is turned
> on and dom.w3c_touch_events.enabled is 0 (this is default on Desktop for Web
> compatibility), CSS touch-action doesn't work on Windows PC with touch
> input.  Big reason is that gesture event isn't dispatched to APZC.
Reason is that CSS touch-action works only if dom.w3c_touch_ACTION.enabled is true.

Case when touch_events is 0 means that APZ does not work,
so WM_GESTURE should work (if I correctly understand).
WM_GESTURE have no CSS touch_action support so I provide patch,
which turns on touch_action support by direct way.

And the main issue should we add touch_action support to WM_GESTURE or not?
If not - what is the aim of current bug?

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Maksim Lebedev from comment #17)
> (In reply to Makoto Kato (:m_kato) from comment #16)
> > You know, APZC can hadle touch-action property.  Now, even if APZC is turned
> > on and dom.w3c_touch_events.enabled is 0 (this is default on Desktop for Web
> > compatibility), CSS touch-action doesn't work on Windows PC with touch
> > input.  Big reason is that gesture event isn't dispatched to APZC.
> Reason is that CSS touch-action works only if dom.w3c_touch_ACTION.enabled
> is true.
> 
> Case when touch_events is 0 means that APZ does not work,

No.  Mouse wheel event uses APZC if APZC is turned on.

When touch_events is 0, WM_TOUCH isn't posted from OS layer. So touch event isn't supported.  Since not calling RegisterTouchWindow, WM_GESTURE is posted from OS instead.

> so WM_GESTURE should work (if I correctly understand).

I think yes.  Because default of dom.w3c_touch_events.enabled is 0.

> WM_GESTURE have no CSS touch_action support so I provide patch,
> which turns on touch_action support by direct way.
> 
> And the main issue should we add touch_action support to WM_GESTURE or not?
> If not - what is the aim of current bug?

If we turn on touch event for desktop on both nightly and release channel, we may be able to remove WM_GESTURE support.  But, actually, we have no plan to enable touch event on desktop Firefox as long as I know.

Currently, touch-action implementation status is the following.

- dom.w3c_touch_events.enabled=1 with APZC ... works
- dom.w3c_touch_events.enabled=0 with APZC ... doesn't work (*1)
- dom.w3c_touch_events.enabled=1 with no-APZC ... doesn't work (*2)
- dom.w3c_touch_events.enabled=0 with no-APZC ... doesn't work (*3)

Since as long as touch_event is turned off as default, we need support *1 situation at least.
*3 situation is your patch.   But I don't know whether we need fix for *2 and *3 situation because touch-action is implemented into APZC.  So I ask smaug for it.

Comment 19

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #18)
> - dom.w3c_touch_events.enabled=1 with APZC ... works
> - dom.w3c_touch_events.enabled=0 with APZC ... doesn't work (*1)
> - dom.w3c_touch_events.enabled=1 with no-APZC ... doesn't work (*2)
> - dom.w3c_touch_events.enabled=0 with no-APZC ... doesn't work (*3)
In all (*1, *2, *3) cases we do not call RegisterTouchWindow, so we do not receive WM_TOUCH events,
so we receive WM_GESTURE in that case and can use it. Is it right or not?
So I have provided patch for direct touch-action support into WM_GESTURES for all (*1, *2, *3) cases.

> ... because touch-action is implemented into APZC...
It is not very correct I think. More correct sentence is like: APZC has touch-action support.

Comment 20

[Makoto Kato [:m_kato]]

(In reply to Maksim Lebedev from comment #19)
> (In reply to Makoto Kato (:m_kato) from comment #18)
> > - dom.w3c_touch_events.enabled=1 with APZC ... works
> > - dom.w3c_touch_events.enabled=0 with APZC ... doesn't work (*1)
> > - dom.w3c_touch_events.enabled=1 with no-APZC ... doesn't work (*2)
> > - dom.w3c_touch_events.enabled=0 with no-APZC ... doesn't work (*3)
> In all (*1, *2, *3) cases we do not call RegisterTouchWindow, so we do not
> receive WM_TOUCH events,
> so we receive WM_GESTURE in that case and can use it. Is it right or not?

Right.

> So I have provided patch for direct touch-action support into WM_GESTURES
> for all (*1, *2, *3) cases.

*1 should replace with APZCTreeManager::ReceiveInputEvent to dispatch gesture event for APZC

Comment 21

[Maksim Lebedev]

(In reply to Makoto Kato (:m_kato) from comment #20)
> *1 should replace with APZCTreeManager::ReceiveInputEvent to dispatch gesture event for APZC
Is it correct or not?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Makoto Kato (:m_kato) from comment #18)
\> When touch_events is 0, WM_TOUCH isn't posted from OS layer. So touch event
> isn't supported.  Since not calling RegisterTouchWindow, WM_GESTURE is
> posted from OS instead.

I think this is a key point. When dom.w3c_touch_events.enabled is 0, WM_TOUCH isn't posted from the OS layer, and touch events are NOT delivered to content. Since touch-action is specific to _touch_ events, I don't think need to support touch-action when dom.w3c_touch_events.enabled is 0. We only need to support touch-action when dom.w3c_touch_events.enabled is 1.

> If we turn on touch event for desktop on both nightly and release channel,
> we may be able to remove WM_GESTURE support.  But, actually, we have no plan
> to enable touch event on desktop Firefox as long as I know.

I would like to enable touch events on desktop Firefox. I'm not sure in what release that will happen, but I do plan on working on that as well eventually.

> Currently, touch-action implementation status is the following.
> 
> - dom.w3c_touch_events.enabled=1 with APZC ... works
> - dom.w3c_touch_events.enabled=0 with APZC ... doesn't work (*1)
> - dom.w3c_touch_events.enabled=1 with no-APZC ... doesn't work (*2)
> - dom.w3c_touch_events.enabled=0 with no-APZC ... doesn't work (*3)
> 
> Since as long as touch_event is turned off as default, we need support *1
> situation at least.

I disagree, based on my argument above. In *1, the content is not receiving touch events, and I don't think we need to support touch-action. Conceptually the browser is just picking up "gestures" from the OS and using those to drive particular actions, whereas the touch-action CSS property deals specifically with touch input and not "gestures". The same thing applies to *3.

*2 is probably something where touch-action should work, but since we are planning on turning on APZ in nightly very soon I don't think we need to bother supporting that.

(In reply to Makoto Kato (:m_kato) from comment #20)
> 
> *1 should replace with APZCTreeManager::ReceiveInputEvent to dispatch
> gesture event for APZC

I think this is an orthogonal decision. In the latest Nightly, if I do APZ=enabled and dom.w3c_touch_events.enabled=0, I am still able to pan and zoom using the WM_GESTURE codepaths on the main thread. We *could* change this implementation to pan and zoom using APZCTreeManager::ReceiveInputEvent instead but it will involve some work and I would rather spend that time to enable dom.w3c_touch_events.enabled instead.

Does that all make sense? If you guys agree with me then I think this bug is INVALID (or WONTFIX, depending on how you look at it).

Comment 23

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Conceptually the browser is just picking up "gestures" from the OS and using
> those to drive particular actions, whereas the touch-action CSS property
> deals specifically with touch input and not "gestures".
Imho any user will be suprised, when some content can be scrolled by gestures,
but cannot be scrolled by touches (If CSS property touch-action disables panning).
> Does that all make sense? If you guys agree with me then I think this bug is
> INVALID (or WONTFIX, depending on how you look at it).
But if we try to enable APZ and touches by default, I have no objections for closing current bug.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

(In reply to Maksim Lebedev from comment #23)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Imho any user will be suprised, when some content can be scrolled by
> gestures,
> but cannot be scrolled by touches (If CSS property touch-action disables
> panning).

The same applies to scrolling using the scroll wheel vs touch input. Scroll wheel scrolling is not affected by touch-action either. I think it's up to the content author to make sure that the user experience is consistent with respect to the different input and scrolling mechanisms.

> But if we try to enable APZ and touches by default, I have no objections for
> closing current bug.

Yes, that is definitely my plan. I filed bug 1180706 yesterday to start tracking stuff that's needed for this.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Based on the discussion above, I'm closing this as wontfix. We don't need to support touch-action when we don't have touch events, and if we have touch events we already have must have APZ and APZ's implementation of touch-action.

See also bug 1029631 where I intend to turn on touch-action shortly.