970346 - Add platform API for per-tab control of touch events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[persona]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140127194636

Steps to reproduce:

Mozilla has posted a way to detect whether touch was enabled, by using the JavaScript ..

'ontouchstart' in window

If this is false, then we are not using a touch device. If it is true, then we are.

In Firefox 26, on a non-touch device.

'ontouchstart' in window -> false.

In Firefox 27, same device.

'ontouchstart' in window -> true.

This is making web pages that uses the previous advice detect Firefox 27 as a touch device and tailor the user experience accordingly. Whether this breaks the page or not depends on what exactly you implemented, but it is possible that it is a breaking change for many pages.


Actual results:

In Firefox 26, on a non-touch device.

'ontouchstart' in window -> false.

In Firefox 27, same device.

'ontouchstart' in window -> true.


Expected results:

Expectation is that this behavior not change between 26 and 27. The browser should accurately know/detect whether the device it is running on is touch enabled, and then elevate that to JavaScript via this event.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What is the value of the "dom.w3c_touch_events.enabled" preference for you in about:config?

Comment 2

[persona]

Hmm, it is set to 1, however I never set this value. I have used the dev tools to emulate touch events (to just play with it) on this particular computer. I upgraded Firefox on my home computer and did not see this behavior (but I had never used the dev tools to emulate touch there, and of course it is different hardware).

If I was reading this I would assume the user changed this value without knowing/realizing it and it is user error :) but I really never did touch this value. I had what I was developing open, working correctly, then upgraded Firefox to 27, and then the page no longer worked as expected, with the difference being this value.

Anyway, changing the value to 0 and then restarting Firefox fixed it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I have used the dev tools to emulate touch events 

That code definitely changes that pref.  I wonder whether we can end up in a situation where we don't set it back if the browser is quit/restarted while the emulation is active?  Paul?

Comment 4

[persona]

Ok now it is starting to make sense. I did have dev tools freeze on me the other day while using responsive design mode. Firefox got into a very strange state (in just that window I was using with dev tools) where clicking on things in the DOM would not work, I closed the dev tools (without killing firefox), and then refreshed that window and everything then appeared to work.

It wasn't until I upgraded to 27 today that my browser exe was killed and a fresh one opened.

Comment 5

[Paul Rouget [:paul]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > I have used the dev tools to emulate touch events 
> 
> That code definitely changes that pref.  I wonder whether we can end up in a
> situation where we don't set it back if the browser is quit/restarted while
> the emulation is active?  Paul?

Looking at the code, I see situations where the pref could not be reset correctly.

Comment 6

[Paul Rouget [:paul]]

Confirmed. It's at least possible to miss the pref reset if 2 Firefox windows are open with 2 responsive mode enabled + touch events enabled.

Comment 11

[Maksim Lebedev]

Is issue still exist on trunc version?

Comment 12

[Alex Druhet]

Hi, I'm still facing this issue on 38.0.5

Comment 13

[deadparsnip]

I have also come across this issue today. Using version 39.0

Comment 14

[gnatko]

For no particular reason, my Firefox 39 started to give me ('ontouchstart' in window) == true (I noticed this when a widget that used to emulate touch events on desktop browsers suddenly stopped working).

I haven't downloaded any updates to Firefox since July 7, at that time (and several days after) everything was fine.

Maybe some internal setting got overwritten somehow? I was developing a JavaScript program that used "localStorage". Can that be related?

Comment 15

[gnatko]

Also, I was hitting ctrl+shift+m (mobile) view quite often, with many tabs open. Also I had a few sudden power outages (when firefox wasn't shut down properly).

Comment 16

[gnatko]

The dom.w3c_touch_events.enabled setting was 1. I changed to 0 and everything works fine now.

Still, that doesn't explain the cause of the problem.

Comment 17

[Luke]

Same here on Ubuntu 14.04 and latest Firefox. What causes this?

Comment 18

[Binyamin]

Happens also on Windows 10 Firefox 44 Nigtly, "ontouchstart" in window -> true

Comment 19

[Binyamin]

Why this annoying bug still hasn't been fixed and landed?
Are there currently any way to check the real support of "ontouchstart" (we can't require all users to play around about:config core settings)?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Why this annoying bug still hasn't been fixed and landed?

Because it's not quite clear why it's happening.  It looks like the developer tools responsive mode somehow leaves this preference in a weird state that exposes ontouchstart unconditionally.

> we can't require all users to play around about:config core settings

Users who never use the developer tools responsive mode (which is pretty much all users) don't run into this bug.

Comment 21

[Binyamin]

@Boris, even when developer tools > "responsive mode" is unchecked (disabled) or user tries to install again Firefox, still we have this issue.

`"ontouchstart" in window` returns `false` (`true` in WebIDE) as excepted after this steps:

1. uninstall all Mozilla programs
2. remove folder "Mozilla" from C:\Users\%username%\AppData\Local, C:\Users\%username%\AppData\LocalLow and C:\Users\%username%\AppData\Roaming
3. run CCleaner > Registry Cleaner
4. fresh Firefox install

Comment 22

[Boris Zbarsky [:bzbarsky]]

> even when developer tools > "responsive mode" is unchecked (disabled)

Right, this bug is about the fact that responsive mode somehow leaves the pref in the wrong state even after being turned off.

> or user tries to install again Firefox

This doesn't affect the user profile and hence the preferences.

In any case, seems to me like the right fix is for responsive mode to stop setting this preference and instead use some API we add to trigger the behavior it wants in a non-persistent way.  Joe, it looks like Paul is not actively working on this stuff anymore.  Do you have someone who has time to look into what sort of API devtools need here?

Comment 23

[Joe Walker [:jwalker]]

Ryan, could you take a look at this please?

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I think we need to expose a new per-docshell testing API for this, so that DevTools can tune it per tab, without affecting behavior generally.

In the past we've done such things through either:

A. docshell settings like allowJavascript and the proposed customUserAgent in bug 1137681
B. windowUtils settings like serviceWorkersTestingEnabled

I would think something like touchEventsEnabled on the docshell may work, but I don't know for sure how easy it is to check against that in all the current callsites[1] that check the pref (some are specific to the pref itself, so would need to investigate their callers).

:bz, would a new setting on the docshell make sense here?

[1]: https://dxr.mozilla.org/mozilla-central/search?q=w3c_touch_events&case=true&=mozilla-central&redirect=true

Comment 25

[Boris Zbarsky [:bzbarsky]]

A docshell setting would make sense if the intent is that navigating the page stays in the "touch events enabled" mode.

As far as existing callers, that depends on what the goal is here.  Why is the responsive design mode setting the pref to start with?  Is the goal to make SupportsApzTouchInput test true?  Is the goal to make TouchEvent and the various createTouch/createTouchList/ontouch* APIs appear in the DOM?  Something else?

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Boris Zbarsky [:bz] from comment #25)
> A docshell setting would make sense if the intent is that navigating the
> page stays in the "touch events enabled" mode.

Yes, I believe that makes sense.  A tab should have this behavior as long as responsive design is active on it, even when it navigates.

> As far as existing callers, that depends on what the goal is here.  Why is
> the responsive design mode setting the pref to start with?  Is the goal to
> make SupportsApzTouchInput test true?

My guess is we may not need SupportsApzTouchInput?  There is no "true" touch input involved AIUI, and perhaps this check is about true touch input coming from a touchscreen?

> Is the goal to make TouchEvent and
> the various createTouch/createTouchList/ontouch* APIs appear in the DOM? 

Yes, we want content touch* event handlers to be bound and respond to events.  A content script (devtools/shared/touch/simulator-content.js) is activated which listens for mouse events and re-fires simulated touch events using createTouch, createTouchList, and createEvent("touch*").

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK.  Just teaching the DOM stuff about this new docshell flag should be pretty straightforward.

Comment 28

[Diego]

I am having the same issue on Firefox 42 for Ubuntu on my desktop. 

'ontouchstart' in window is returning true. But I am on a desktop machine.

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1188270 comment 4 describes a clear cut way to trigger the issue under the current setup: open RDV, enabled touch simulator, then close the browser.  The pref value is not reset on browser close, so you're stuck with touch enabled forever after.

We could be smarter about the pref, but the right fix is the per-tab API purposed here.

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Setting ni? as a reminder to add more details here.

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

For a rough guide on what this would look like, you could look through :ntim's work to add per-tab control of the user agent in bug 1137681 (platform side) and bug 828008 (DevTools side).

The major steps would look like:

1. Add an attribute to nsIDocShell to allow chrome code to override the default choice of whether touch events are available for a tab, such as "allowTouchEvents" or similar (maybe the name should suggest it is an override?).  For maximum control, I think we would want to have a tri-state value so we can say on, off, or fallback to the global default.
2. Examine code paths that currently test the global pref "dom.w3c_touch_events.enabled".  Such functions would now need to be have access to the docshell so they can check the new attribute as well as the existing pref.  (I am assuming we'll want to keep the existing pref to avoid too much change in one go.)  I would expect the docshell attribute to be checked first, and if it says "fallback to default", then do whatever the existing code did.
  * If you have to change various different platform modules, you may want separate patches for each area so that different reviewers can look over the portion they own separately
  * If you are not used to changing C++ code, try using "mach build binaries" after an initial build to rebuild only C++ files
3. Add some kind of DevTools actor support to flip the new platform attribute.  Historically we've managed these things from webbrowser.js, and it's probably best to keep going with this pattern.  User agent is a good example[1].
4. The RDM UI needs to use a DevTools client to change the actor setting.  So far, we have not done this from the new RDM UI.  The old RDM UI does it for user agent[2].  This might be something that happens in manager.js for the new UI, I haven't completely thought through that part.

Please ask as questions early as they come up!

[1]: https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/devtools/server/actors/webbrowser.js#1755
[2]: https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/devtools/client/responsivedesign/responsivedesign.jsm#975

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Marco, let's move this into the current release instead of bug 1240912.

Comment 33

[Marco Mucci [:MarcoM]]

Added to Release 49.

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If you prefer, we could split this into 2 bugs and use this one to build up just the platform side (steps 1 - 2 from comment 31) and have another to actually use it in new RDM (steps 3 - 4).  The work to use it in RDM involves some setup steps (creating an RDP client, etc.) that aren't specific to this one feature, since we'll need them for other things as well.

Comment 35

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

I'm happy to do any platform-side work needed for this bug, as it would great to fix this before we let touch events on Windows ride the trains (which I would like to do in Fx 50).

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #35)
> I'm happy to do any platform-side work needed for this bug, as it would
> great to fix this before we let touch events on Windows ride the trains
> (which I would like to do in Fx 50).

@kats, let me post up my current patches, which implements some of the amount

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

some of the above*

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add a DocShell attribute to override the default touch event options to a tab [1.0]

This simply adds a tri-state attribute to nsIDocShell. It defaults to 2, which stands for the default 'fallback' option, and 0 for 'off' and 1 for 'on'.

@kats, since you are interested in taking this on, I am also assuming you would be a good reviewer for this. Please change the reviewer as necessary.

Comment 39

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

Comment on attachment 8767990 [details] [diff] [review]
Part 1: Add a DocShell attribute to override the default touch event options to a tab [1.0]

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

Unfortunately I'm not as comfortable with the docshell code to review it. From my quick look at the patch it mostly looks fine, but I have one concern below. I don't know if there's a good way to avoid that problem.

::: dom/events/TouchEvent.cpp
@@ +167,5 @@
>  TouchEvent::PrefEnabled(JSContext* aCx, JSObject* aGlobal)
>  {
>    static bool sPrefCached = false;
>    static int32_t sPrefCacheValue = 0;
> +  nsCOMPtr<nsPIDOMWindowInner> win = Navigator::GetWindowFromGlobal(aGlobal);

I'm not sure how well this will work if aGlobal is null, which happens in a number of places (that's the default value for the parameter). I think we might end up in a weird state where some of the code expects touch events to be enabled and the rest of the code expects it is disabled (or vice-versa) and that might cause subtle problems.

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #39)
> Comment on attachment 8767990 [details] [diff] [review]
> Part 1: Add a DocShell attribute to override the default touch event options
> to a tab [1.0]
> 
> Review of attachment 8767990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately I'm not as comfortable with the docshell code to review it.
> From my quick look at the patch it mostly looks fine, but I have one concern
> below. I don't know if there's a good way to avoid that problem.
> 
> ::: dom/events/TouchEvent.cpp
> @@ +167,5 @@
> >  TouchEvent::PrefEnabled(JSContext* aCx, JSObject* aGlobal)
> >  {
> >    static bool sPrefCached = false;
> >    static int32_t sPrefCacheValue = 0;
> > +  nsCOMPtr<nsPIDOMWindowInner> win = Navigator::GetWindowFromGlobal(aGlobal);
> 
> I'm not sure how well this will work if aGlobal is null, which happens in a
> number of places (that's the default value for the parameter). I think we
> might end up in a weird state where some of the code expects touch events to
> be enabled and the rest of the code expects it is disabled (or vice-versa)
> and that might cause subtle problems.

@jryans, any idea on how we can get around this?

Comment 41

[Boris Zbarsky [:bzbarsky]]

If we now have a dependency on TouchEvent::PrefEnabled actually caring about which global is being asked about, we should probably make that argument required.

One simple option is to add a version of PrefEnabled that takes one argument (probably an nsPIDOMWindowInner or docshell or whatever is most convenient for the C++ callers) and move all the logic into that.  The two-arg version would start requiring both args and do the GetWindowFromGlobal() thing and call the one-arg version. Then you "just" have to fix all the places that call TouchEvent::PrefEnabled with no args.  Looks like we have 4 such callsites.

Comment 42

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems like :bz has a reasonable plan.

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Talked to :kats on IRC, they are interested in working on the platform side of this, so assigning to them.  They said they would start to take a look next week most likely.

I'll file a separate bug for the remaining DevTools work to make use of the platform API added here.

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Moving to Core :: Panning and Zooming where I saw other touch bugs filed since it's now only the platform API.

Comment 45

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9f2e4b2232

Comment 46

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

I think technically this bug is more appropriate under DOM:Events.

Comment 47

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

Attachment: Add a docshell attribute to override touch events

Turns out that starting docshell properties with "allow" is a bad idea because session restore will clobber them, so I renamed it to touchEventsOverride. As best I can tell this passes tests, but we've been having infra problems the last couple of days so I can't be certain about the try results [1][2]. Still I think it's probably safe to be reviewed now. bz, please feel free to redirect to somebody else if you think they would be a better reviewer - I'm not really sure who owns this code.

This patch is mostly the same as what Gabriel posted earlier, with additional bits to try and get a docshell into TouchEvent::PrefEnabled where possible.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9f2e4b2232&group_state=expanded
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaed13abbe15&group_state=expanded

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8771506 [details] [diff] [review]
Add a docshell attribute to override touch events

Please document what the values of the constant mean.  Specifically that NONE means "no override, do whatever you would normally do" and that the others force-disable and force-enable.

Does the test work correctly in e10s, given that it's poking at "content"?

>+nsContentUtils::GetDocShellForEventTarget(EventTarget* aTarget)

So in all cases except the node->OwnerDoc() one you go through nsPIDOMWindowInner, and even in that case you could have used GetScriptHandlingObject, right?

And once you have a nsPIDOMWindowInner you can GetDocShell() on it directly instead of going through GetExtantDoc....   Then again, I guess you just moved this code.  Maybe a followup bug on simplifying it, just in case that actually changes its behavior.

>+    nsCOMPtr<nsPIDOMWindowInner> win = Navigator::GetWindowFromGlobal(aGlobal);

How about just using xpc::WindowOrNull here instead?  We should really replace GetWindowFromGlobal with that.  So much code cleanup needed...

r=me with those nits.

Comment 49

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

(In reply to Boris Zbarsky [:bz] from comment #48)
> Please document what the values of the constant mean.  Specifically that
> NONE means "no override, do whatever you would normally do" and that the
> others force-disable and force-enable.

Updated patch to do this.

> Does the test work correctly in e10s, given that it's poking at "content"?

Yeah, it seems to. I ran it locally both with and without --disable-e10s, and the try push also shows it running and passing in both configurations (M-bc5 and M-e10s-bc3 in Linux opt).

> >+nsContentUtils::GetDocShellForEventTarget(EventTarget* aTarget)
> 
> So in all cases except the node->OwnerDoc() one you go through
> nsPIDOMWindowInner, and even in that case you could have used
> GetScriptHandlingObject, right?
> 
> And once you have a nsPIDOMWindowInner you can GetDocShell() on it directly
> instead of going through GetExtantDoc....   Then again, I guess you just
> moved this code.  Maybe a followup bug on simplifying it, just in case that
> actually changes its behavior.

Ok, filed bug 1287442 for this.

> >+    nsCOMPtr<nsPIDOMWindowInner> win = Navigator::GetWindowFromGlobal(aGlobal);
> 
> How about just using xpc::WindowOrNull here instead?  We should really
> replace GetWindowFromGlobal with that.  So much code cleanup needed...

Updated patch to do this as well, verified that it still passes tests.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c480a657e2ab
Add a DocShell attribute to override the default touch event options to a tab. r=bz

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c480a657e2ab

Comment 52

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No QE verification needed here since user visible portion was moved to bug 1285566.