888300 - dom.w3c_touch_events.enabled=0 isn't fully respected

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Avi Halachmi (:avih)]

When setting the pref dom.w3c_touch_events.enabled=0 (tested on widnows 8 only), Firefox still doesn't always capture touch events and interprets them as scroll when the content-page registers touch events.

The patch at bug 736048 comment 27 might fix this problem.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Patch from bug 736048 comment 27

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 769080 [details] [diff] [review]
Patch

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

How often do these get called? I'm wondering if we should be using a pref observer instead.

::: widget/windows/nsWindow.cpp
@@ +1236,5 @@
>   *
>   **************************************************************/
>  
>  NS_METHOD nsWindow::RegisterTouchWindow() {
> +  if (Preferences::GetInt("dom.w3c_touch_events.enabled", 0) != 0) {

nit: (!Preferences::GetInt("dom.w3c_touch_events.enabled", 0))

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Not very often: it will get called on tab switch and on focus changes between frames (for pages with iframes)

I think it's not necessary to use a pref observer.. one thing we can do is to only check the pref on RegisterTouchWindow. It will have no ill effect and that will be called only for pages with touch event listeners

Comment 4

[Jim Mathies [:jimm]]

(In reply to :Felipe Gomes from comment #3)
> Not very often: it will get called on tab switch and on focus changes
> between frames (for pages with iframes)
> 
> I think it's not necessary to use a pref observer.. one thing we can do is
> to only check the pref on RegisterTouchWindow. It will have no ill effect
> and that will be called only for pages with touch event listeners

That sounds fine then.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

> nit: (!Preferences::GetInt("dom.w3c_touch_events.enabled", 0))

I did this change, but what I actually need is without the !

http://hg.mozilla.org/integration/mozilla-inbound/rev/276dcc6af9ee

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/276dcc6af9ee

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 769080 [details] [diff] [review]
Patch

This patch is a companion to bug 888304 which got uplifted to Aurora. That patch only changes the default pref value but this is the patch that actually fixes the problem that touch events cause in Desktop (some websites won't scroll)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): websites using touch events
User impact if declined: sites won't scroll if they have touch events listeners and the user has a touch-capable Win7/8 tablet
Testing completed (on m-c, etc.): working on m-c
Risk to taking this patch (and alternatives if risky): no additional risk (the risk consideration for disabling these events was done in bug 888304)
String or IDL/UUID changes made by this patch: none

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 769080 [details] [diff] [review]
Patch

With the merge today this now needs beta approval. Request text from comment 7 reposted for simplicity:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): websites using touch events
User impact if declined: sites won't scroll if they have touch events listeners and the user has a touch-capable Win7/8 tablet
Testing completed (on m-c, etc.): working on m-c
Risk to taking this patch (and alternatives if risky): no additional risk (the risk consideration for disabling these events was done in bug 888304)
String or IDL/UUID changes made by this patch: none

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 769080 [details] [diff] [review]
Patch

Approving to aid with the backout in https://bugzilla.mozilla.org/show_bug.cgi?id=888304, please add qawanted,verifyme if there is any testing qa can help with here to ensure there are no fallouts.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e99ffebd41e4

Comment 11

[u279076]

Avi, do you have examples of websites where this was 100% reproducible?

Comment 12

[Avi Halachmi (:avih)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #11)
> Avi, do you have examples of websites where this was 100% reproducible?

Not 100%, but we used the google results page as a benchmark IIRC>

I don't think we ever had a 100% working STR. In part because the fix was relatively easy and obvious (to felipe), so there was no real need for accurate STR.

The "isn't fully respected" part of the description meant "on a page X, the pref=0 is only sometimes respected" (versus only respected on some pages).

IIRC, this was roughly the STR:

1. Set dom.w3c_touch_events.enabled=0 and restart the browser.

2. Visit a page where w3c touch breaks touch-scroll (i.e. that when w3c touch is enabled and we try to touch-scroll that page - it selects text rather than scroll - e.g. google results page).

3. Touch-scroll the page.

If it selects text instead of scroll, that's the bug. Stop.

If it scrolled correctly:

4. Do some other stuff like switching to another tab and then get back to this one, scroll a bit, etc.

5. goto 3


felipe, can you create an STR for this before your patch landed? Alternatively, can you improve step 4 of the STR?

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: touch-testcase.htm

Yeah, the testcase is simple.

1- Set dom.w3c_touch_events.enabled=2 and restart
2- open testcase and try to touch scroll by doing a vertical movement.

Verify that text is selected and the page doesn't scroll. (this is just to validate the testcase)

3- Now, reset dom.w3c_touch_events.enabled=0 and restart.
4- open testcase and try to touch scroll by doing a vertical movement.

If it scrolls: Ok.  If text still selects: bug.


(note: a mostly horizontal touch movement will always select text, it's not a bug)


Avih: The extra step that you were looking for is not really needed, it was just a false-negative due to how the window touch is registered. But if these steps here confirms that it's fixed, there's nothing else to worry about.

Comment 14

[Avi Halachmi (:avih)]

(In reply to :Felipe Gomes from comment #13)
> Avih: The extra step that you were looking for is not really needed, it was
> just a false-negative due to how the window touch is registered. But if
> these steps here confirms that it's fixed, there's nothing else to worry
> about.

I clearly recall reports that setting the pref to 0 _seemed_ to work (i.e. it scrolled rather than selected), but then some time later with the same page open, it stopped working (scroll didn't work anymore and instead it started selecting).

Since I don't recall/know what was exactly required to trigger the bug, I added this "goto 3" step. Your STR (which is the same as mine with some validation setup but without the loop) suggests that the bug is 100% reproducible on the first time we try to scroll, which IIRC is not the case.

E.g. see bug 736048 comment 26.

Comment 15

[u279076]

Thanks Avi and Felipe.

I can confirm this is reproducible in Firefox 25.0a1 2013-06-27. It's not reproducible in Firefox 25.0a2 2013-09-13 and Firefox 24.0 RC. Calling this verified.

I tested on my Asus VivoTab running Windows 8.