Closed Bug 888300 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: User events and focus handling, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox24 --- verified
firefox25 --- verified

People

(Reporter: avih, Assigned: Felipe)

References

Details

Attachments

(2 files)

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.
Blocks: 888304
Attached patch PatchSplinter Review
Patch from bug 736048 comment 27
Attachment #769080 - Flags: review?(jmathies)
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))
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
Attachment #769080 - Flags: review?(jmathies) → review+
(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.
> 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
https://hg.mozilla.org/mozilla-central/rev/276dcc6af9ee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #769080 - Flags: approval-mozilla-aurora?
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
Attachment #769080 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #769080 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Avi, do you have examples of websites where this was 100% reproducible?
Flags: needinfo?(avihpit)
(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?
Flags: needinfo?(avihpit) → needinfo?(felipc)
Attached file 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.
Flags: needinfo?(felipc)
(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.
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.
Status: RESOLVED → VERIFIED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.