Closed Bug 736048 Opened 12 years ago Closed 8 years ago

Some sites won't touch-scroll using a touch-screen

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
All
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Unfocused, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files, 7 obsolete files)

Minimal test case
12.05 KB, text/html
Details
touch_on_desktop_ver8.diff
12.37 KB, patch
Details | Diff | Splinter Review 
Testing on a Samsung Series 7 Slate running Windows 8 Consumer Preview, I've noticed that some sites won't scroll using touch-scrolling (placing finger in the content area, and moving up and down).

I don't have a reduced testcase, but twitter.com is a good example.
Correction: Twitter.com works on first load, then sometime after it stops working (eg, sometimes after switching to another tab and back again).
I can confirm this with the latest nightly on a number of sites.  Like Blair said, it sometimes works on the first load and then fails later on.  Whenever you try to scroll it just selects text.
I see this as well. If you experience this please post the uri of the site here.
I consistently see this on Google search pages, so it is a pretty major issue.  It works if I disable JavaScript, so it's likely an issue with touch events.  It works when tested in Chrome and IE10.
I was able to confirm that this is related to handling of touch events.  I could add an event listener to the document element and cause the scrolling to fail.  I could then remove the event listener and the scrolling would work again.  This listener was just a blank function, no calls to stopPropagation or preventDefault.
Wait, I take that back, removing the event listener does not fix it.
I can confirm this with Firefox 18 final - any touch event listener on the documentElement breaks the scrolling. However there is another edge case - attaching a touch event on any element that is not inside document.body breaks the touch scrolling - for instance an element inside a documentFragment (e.g. one created with jQuery before adding it to the DOM).
Is anyone looking into this?  It's very frustrating to use Firefox in Windows 8 with this bug.  I keep finding more and more sites where this shows up.
Urls for sites that exhibit this would help.
from an email discussion in this problem:

> From: Olli Pettay
>
> IIRC when MozTouch events (which are now removed) were implemented,
> it ended up working so that OS sent pixel scrolling events, which
> then got converted to MozPixelScroll  events.
> I don't recall how we handled the case when preventDefault of a
> MozTouch event was called.
>
> Do we perhaps still do similar thing with W3C touch events?

So we still support both. If the window is registered as a touch
capable window then we switch from gesture / pixel scroll to touch
events.

gesture / pixel scroll -
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#6378

touch events:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#6282

nsGlobalWindow::UpdateTouchState() -
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8210

I'm not sure how this should be changed to fix bug 736048. The
dom is making the decision about which input it wants. Do we
need to file a bug on sending both from win32 widget?

Winrt widget is different, it sends the first touch start and
touch move, checks the result and makes a decision from that.
It also ignores calls to RegisterTouchWindow() / UnregisterTouchWindow().
You can also reproduce this very easily on http://theverge.com
The verge  works again, but google-search results show the problem consistently. By the way, I somehow missed this bug-report when searching (sorry for that!), so I have created a new one here: https://bugzilla.mozilla.org/show_bug.cgi?id=847875 
It seems to be the exact same problem.
Assignee: nobody → jmathies
FWIW, seems twitter and google are treating our input better. I'm not having any issues touch panning either in nightly.
I still have this issue, even with the daily nightly build (23.0a1 (2013-04-21)). The bug only occurs if i reload or revisite the website!
The website i checked for the issue is: http://www.handelsblatt.com/
He is right. I am testing on G+. The scrolling works fine on first load, breaks on reload.
Happens every time on google search results pages.
Also the vertical scroll bar can't be touch-operated on google search result pages. I'm testing with a Surface Pro running the latest FF nightly build: 23.0a1 (2013-05-05)
Attached file Minimal test case — 
This is an infuriating bug (!) that makes pages like Google, GMail and others effectively unscrollable on Win8 tab.

I've attached a very simple file that exhibits the problem for me. As is, I can't touch-scroll the page. But if I comment out the "touchstart" event, I can. Also of note, I can't touch the 'OK' button on the alert dialog to dismiss it, I have to hit 'Enter' on the popup keyboard. Works fine on Chrome (and IE, though IE doesn't support 'touchstart' anyway).

I've no idea if the issue here is the same one that's present on Google pages, but I'd guess there's a good chance it is.
Attachment #754134 - Attachment mime type: text/plain → text/html
Just noticed one more interesting thing. If I click the attachment, and then hit back to this bug page, I can't scroll the bug page. Then I switch to another tab, and come back to this bug page and I can scroll it again.
No improvement after updating to 21, Windows 7 Professional 64bit
and Windows 7 Home Premium 32bit.
Seeing this every time on google.com and gmail.com

On nytimes.com, the first load exhibits this behaviour, then it is resolved by a reload.

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0

Cheers,

Jonathan
(In reply to Jonathan Guerin from comment #24)
> Seeing this every time on google.com and gmail.com
> 
> On nytimes.com, the first load exhibits this behaviour, then it is resolved
> by a reload.
> 
> Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> 
> Cheers,
> 
> Jonathan

I lie, a reload fixes it temporarily, then it stops working again.
In case people are looking for a (patchy) workaround:

Go into about:config and set dom.w3c_touch_events.enabled=0 (remember to re-enable when this bug is fixed).

With that, touch-scrolling usually works on Google and GMail, but not the test case. If you reload Google, it breaks and behaves like the test case. GMail breaks if you put the system to sleep and wake it up, but is fine after reload. But, in all cases you *can* now scroll using the side scrollbar (both Google and test case), so it's not the not-even-dogfood problem that it was before.

Of course, I imagine it would break touch-only sites, but I don't encounter any such beasts in my typical browsing.

BTW, the platform on this bug is wrong as it affects 32 bit Win8 as well.
For the websites where scrolling don't work, the problem here is that these websites are registering touch event listeners, and when they do that we stop receiving scrolling information, due to a limitation of the windows API in where we can either receive scrolling or raw touch information, but not both.

The way to properly fix that is to always receive touch information and have our own conversion of that to kinetic scrolling. We don't have that code yet but perhaps the work going on Win Metro will make it easier to do so.

In the meantime, I put a patch that makes the workaround in comment 26 always work properly. i.e.: if dom.w3c_touch_events.enabled=0 every site will be able to be scrolled (but then no sites will be able to use touch events for their own purposes).

Here is a try push for this patch: https://tbpl.mozilla.org/?tree=Try&rev=38fd2eb60def
The Windows builds will show up in the next hour at this link: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/felipc@gmail.com-38fd2eb60def
Hardware: x86_64 → All
(In reply to :Felipe Gomes from comment #27)
> The way to properly fix that is to always receive touch information and have
> our own conversion of that to kinetic scrolling. We don't have that code yet
> but perhaps the work going on Win Metro will make it easier to do so.

I get really worried when I keep hearing this. Android wrote their own custom Kinetic panning code. B2G ported it into the platform [1]. We're now working hard to unify Android and B2G's code [2] (i.e. even though this is in ipc code, it doesn't require e10s). Is Metro planning to reinvent the world again? Can we get some cross-polination/conversations here?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=839641
(In reply to Wesley Johnston (:wesj) from comment #28)
> (In reply to :Felipe Gomes from comment #27)
> > The way to properly fix that is to always receive touch information and have
> > our own conversion of that to kinetic scrolling. We don't have that code yet
> > but perhaps the work going on Win Metro will make it easier to do so.
> 
> I get really worried when I keep hearing this. Android wrote their own
> custom Kinetic panning code. B2G ported it into the platform [1]. We're now
> working hard to unify Android and B2G's code [2] (i.e. even though this is
> in ipc code, it doesn't require e10s). Is Metro planning to reinvent the
> world again? Can we get some cross-polination/conversations here?

Metro is going to use the c++ apzc. We're working on that right now. Should be out on Nightly in a couple weeks. Seems metro is playing the role of guinea pig for desktop.

Also felipe, you might be interested in a short dev.platform discussion we had on win32. There's a lot of bustedness here based on current assumptions regarding what the widget layer should be doing -

https://groups.google.com/forum/#!searchin/mozilla.dev.platform/touch/mozilla.dev.platform/QxgrqBlqAdk/GPN5rxhNmUIJ

I took this bug with the intent on making some of those changes, although I have yet to find the time to actually do it.
AFAICT, fixing this properly isn't going to happen overnight.

I'd suggest to file a new bug for properly respecting dom.w3c_touch_events.enabled=0 (e.g. with the patch from comment 27).

Also, once it works, I think we should consider setting dom.w3c_touch_events.enabled=0 by default (on windows 8 only?) until this bug is fixed. Otherwise, the current Firefox-desktop experience on Windows 8 is that some very high-profile pages (google search results, twitter, some MDN pages) are not scrollable using touch.

If I had to choose between having touch-events support for content and being able to scroll pages which I frequently visit, I'd choose the latter, especially if touch is my main interface to Firefox (e.g. while using a tablet) and other scroll inputs are not always available.
With Felipe's patch, things work perfectly. In fact, now that it's robust, I think it actually works better than IE10 and Chrome --- I can do every gesture I want, exactly the way I'd expect: touch dragging up/down scrolls, touch dragging left-to-right over text selects it, etc.

I agree with Avi. When I first used Firefox on Win8 touch, this bug made me think it was still heavily a work in progress for that platform, so I resorted to using Chrome (and it takes a *lot* to make me switch away from Fx). The patch makes Fx feel like a first class citizen, so if there's any chance of a delay for the proper fix, it seems like expediting the patch and disabling web touch events by default (on Win8) would be an excellent short-term win with no long-term downside (since the proper fix will not be affected by it).
Depends on: 888300
Felipe, if your patch improves things (sounds like it does) are you planning on kicking off a review?
Flags: needinfo?(felipc)
Blocks: 888304
Blocks: 888305
FWIW,
- I filed bug 888300 to fix support for dom.w3c_touch_events.enabled=0.
- I filed bug 888304 to disable content-touch events by default until this bug is fixed.
- I filed bug 888305 to enable content-touch events by default once this bug is fixed.
(In reply to Jim Mathies [:jimm] from comment #32)
> Felipe, if your patch improves things (sounds like it does) are you planning
> on kicking off a review?

Yeah, I posted the patch from the build in comment 27 to bug 888300, and it goes together with bug 888304
Flags: needinfo?(felipc)
(In reply to Wesley Johnston (:wesj) from comment #28)
> (In reply to :Felipe Gomes from comment #27)
> > The way to properly fix that is to always receive touch information and have
> > our own conversion of that to kinetic scrolling. We don't have that code yet
> > but perhaps the work going on Win Metro will make it easier to do so.
> 
> I get really worried when I keep hearing this. Android wrote their own
> custom Kinetic panning code. B2G ported it into the platform [1]. We're now
> working hard to unify Android and B2G's code [2] (i.e. even though this is
> in ipc code, it doesn't require e10s). Is Metro planning to reinvent the
> world again? Can we get some cross-polination/conversations here?

FWIW my comment was written without knowledge of the actual plans for Metro. I wasn't sure of how reusable the c++ APZC is meant to be, but now I think that by "don't have that code yet" it just means we don't have it hooked up between the widget and APZC, and the win32 and winrt code are different so I don't know if one will help the other. This means that supporting proper touch in traditional Desktop and Metro will be two different efforts, but hopefully it won't need any new handling code, just a matter of hooking both to the controller.

(But it still might be vastly different depending on if desktop will have to keep traditional pixel scrolling or if OMTC will allow layer panning for us)
It's been a few months, where are we with this?
I think we can pretty much write off real touch support on desktop. On Windows you can use the metro browser for a good touch experience. If you're on desktop all you need is basic scroll support, which we have now that our broken touch support is disabled. 

We should probably go through, find all the various touch specific bugs for desktop and just wontfix them.
I think we could get touch scrolling and touch events working properly together in desktop Firefox if and when it moves to AsyncPanZoomController for scrolling.  There's work going on to make that possible, but it's still a very long-term thing.
Is this being tracked in another bug report? Is it unreasonable to think that any progress will be made this quarter? If so, as the QA owner for touch events I will remove it from my deliverables in Q4 and focus on other work.
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #39)
> Is this being tracked in another bug report?

There are bugs filed for the overall unification of touch scrolling across platforms, like bug 775437 and bug 663286, but they are not really active right now.  The current focus is on refactoring the code used on Android/B2G/Metro to make more of it cross-platform (e.g. bug 776030 and dependencies), which in turn will enable future work on desktop.

> Is it unreasonable to think that any progress will be made this quarter?

I do not think anyone is planning to make any changes to touch handling on desktop Firefox this quarter.
Okay thanks, I will drop this from my plate for this quarter and monitor those bugs for progress. Feel free to email me directly if this becomes a higher priority so I can reprioritize.
Assignee: jmathies → nobody
Do we have any plan to fix this?

Now that firefox metro is gone, many (most?) windows laptops have touch screens, and tablets/hybrids are available, having support for HTML5/w3c touch sounds like a good thing to have (even if personally I don't really miss it - it's enough for me that the page touch-scrolls and I that can touch-click links).

The w3c touch support is currently disabled (bug 888304) because this bug is open.
Flags: needinfo?(jmathies)
We would like to get our desktop touch support improved, however we currently don't have an free full time engineers to work on it. Here's hoping a volunteer steps up..
Flags: needinfo?(jmathies)
Let's at least try to put this on the backlog.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog-
Just to be clear: this is firefox-backlog- because it's a Core::Event Handling bug, and thus not something that the Firefox front-end team is primarily responsible for. firefox-backlog- in those situations is not a judgment on the bug's importance/priority.
Attached patch touch_on_desktop_ver1.diff (obsolete) — — Splinter Review 
Hi folks. I try to start resolve our common issue.

My Plan are:
  Implement global class to check touch events.
  Implement support touch events and pointer events.
  Change behavior according our common needs.

Comments and suggestions are very welcome!

Let's go!
Attachment #8442875 - Flags: feedback?(jmathies)
Comment on attachment 8442875 [details] [diff] [review]
touch_on_desktop_ver1.diff

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

Just getting started here, I don't see any issues yet although there's not a lot here. Generally like the idea of moving the pres shell touch tracking logic out into a separate module.
Attachment #8442875 - Flags: feedback?(jmathies) → feedback+
Attached patch touch_on_desktop_ver2.diff (obsolete) — — Splinter Review 
+ Changes: Added ability to scrolling content by touch on desktop FF

Comments and suggestions are very-very welcome!
Attachment #8442875 - Attachment is obsolete: true
Attachment #8470079 - Flags: feedback?(tabraldes)
Attachment #8470079 - Flags: feedback?(oleg.romashin)
Attachment #8470079 - Flags: feedback?(nicklebedev37)
Attachment #8470079 - Flags: feedback?(bugs)
Attachment #8470079 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8470079 [details] [diff] [review]
touch_on_desktop_ver2.diff

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

I'm deferring to smaug/romaxa here since I don't know anything about desktop windows event flow.
Attachment #8470079 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8470079 [details] [diff] [review]
touch_on_desktop_ver2.diff

Move TouchManager to another file. But yeah, this kind of approach might be
pretty good clean up, and would let us handle touch events in various ways.
Need to probably have a pref for behavior, so that FFOS for example can
disable the desktop behavior.
Attachment #8470079 - Flags: feedback?(bugs) → feedback+
Attachment #8470079 - Flags: feedback?(tabraldes)
When I enable electrolysis, touch scrolling reverts to the previous broken behaviour --- i.e. swiping down causes text to get selected, rather than scrolling the page. I'm not sure if this is related to the the issue in this bug, or if it has a different cause.
Attachment #8470079 - Flags: feedback?(nicklebedev37)
Attached patch touch_on_desktop_ver3.diff (obsolete) — — Splinter Review 
I started work on this bug again.

+ Changes: Synchronized with new sources
+ Changes: Moved TouchManager into separated files
+ Changes: Added detection of DRAG state
+ Changes: Disabled dispatching POINTER and TOUCH events at DRAG state

Patch maybe rough, but comments and suggestions are very welcome!
Attachment #8470079 - Attachment is obsolete: true
Attachment #8470079 - Flags: feedback?(oleg.romashin)
Attachment #8555933 - Flags: feedback?(mbrubeck)
Attachment #8555933 - Flags: feedback?(jmathies)
Attachment #8555933 - Flags: feedback?(bugs)
Can anybody put bug "960316" into "Blocks" information? Unfortunately, I have no rights for do this.
Comment on attachment 8555933 [details] [diff] [review]
touch_on_desktop_ver3.diff

I like the idea of separating this touch code out into a module, fwiw.
Attachment #8555933 - Flags: feedback?(jmathies) → feedback+
Comment on attachment 8555933 [details] [diff] [review]
touch_on_desktop_ver3.diff

> nsFocusManager::ScrollIntoView(nsIPresShell* aPresShell,
>                                nsIContent* aContent,
>                                uint32_t aFlags)
> {
>+  static bool flag = false;
>   // if the noscroll flag isn't set, scroll the newly focused element into view
>-  if (!(aFlags & FLAG_NOSCROLL))
>+  if (!(aFlags & FLAG_NOSCROLL) || flag)
I guess this is just for some testing.

>-  if (mMayHaveTouchEventListener) {
>+  if (mMayHaveTouchEventListener || true) {
and this too

>+ * This Original Code has been modified by IBM Corporation.
>+ * Modifications made by IBM described herein are
>+ * Copyright (c) International Business Machines
>+ * Corporation, 2000
>+ *
>+ * Modifications to Mozilla code or documentation
>+ * identified per MPL Section 3.3
>+ *
>+ * Date         Modified by     Description of modification
>+ * 05/03/2000   IBM Corp.       Observer events for reflow states
Really?




>+    if (!mTouchManager->PreHandleEvent(aEvent, aStatus, touchIsNew, isHandlingUserInput)) {
>+      return NS_OK;
>+    }
TouchManager::Pre/PostHandleEvent calls could perhaps go to EventStateManager, where it would call those methods
if event's class is eTouchEventClass



>+nsresult
>+PresShell::DispatchEvent(WidgetEvent* aEvent,
>+                         nsEventStatus* aStatus,
>+                         nsPresShellEventCB* aEventCB)
Might be good to call this something else than just DispatchEvent, since we have
so may other DispatchEvents already. And you could do this kind of code move in a separate patch.
Maybe the method could be
DispatchEventToDOM


>+NS_IMETHODIMP nsFrame::HandleTouchDrag(nsPresContext* aPresContext,
>+                                       WidgetGUIEvent* aEvent,
>+                                       nsEventStatus* aEventStatus)
>+{
>+  MOZ_ASSERT(aEvent->mClass == eTouchEventClass,
>+             "HandleTouchDrag can only handle touch event");
>+
>+  WidgetTouchEvent* touchEvent = aEvent->AsTouchEvent();
>+  if (!touchEvent) {
>+    return NS_OK;
>+  }
>+
>+  static nsIntPoint pt(0, 0);
>+
>+  switch(touchEvent->message) {
>+  case NS_TOUCH_START:
>+    pt = GetPointFromTouchEvent(touchEvent);
>+    break;
>+  case NS_TOUCH_MOVE:
>+  {
>+    nsIScrollableFrame* scrollFrame =
>+      nsLayoutUtils::GetNearestScrollableFrame(this,
>+        nsLayoutUtils::SCROLLABLE_SAME_DOC | nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
>+    if (scrollFrame) {
>+      nsIntPoint new_pt = GetPointFromTouchEvent(touchEvent);
>+      scrollFrame->ScrollBy(pt - new_pt,
>+                            nsIScrollableFrame::DEVICE_PIXELS,
>+                            nsIScrollableFrame::SMOOTH);
>+      pt = new_pt;
>+    }
>+    break;
>+  }
>+  case NS_TOUCH_CANCEL:
>+  case NS_TOUCH_END:
>+    pt.x = pt.y = 0;
>+    break;
>+  }
>+  return NS_OK;
>+}
Hmm, why are we trying to do scrolling in this level when dealing with touch events. You want feedback from kats, but
this really should happen somewhere in APZ level.
Same applies to TouchManager::HandleTouchDrag

hmm, actually, does anyone ever call nsFrame::HandleTouchDrag? I see only TouchManager::HandleTouchDrag being called.



>+  NS_IMETHOD HandleTouchDrag(nsPresContext* aPresContext,
>+                             mozilla::WidgetGUIEvent* aEvent,
>+                             nsEventStatus* aEventStatus);
>+
What is touch drag? A drag-and-drop action initiated from touch event?
Based on the code the method is trying to detect a swipe gesture or something



Please do all the code moves separately in some other bugs. Otherwise reviewing the changes will be really difficult.
Attachment #8555933 - Flags: feedback?(bugs) → feedback-
(In reply to Olli Pettay [:smaug] from comment #55)
> >+    if (!mTouchManager->PreHandleEvent(aEvent, aStatus, touchIsNew, isHandlingUserInput)) {
> >+      return NS_OK;
> >+    }
> TouchManager::Pre/PostHandleEvent calls could perhaps go to
> EventStateManager, where it would call those methods
> if event's class is eTouchEventClass
Perhaps no. At first, there is case which blocks executing of EventStateManager and others.
At second, idea is collect all code related with touch events into one module,
but not only transfer from one place to another (like from PresShell into EventStateManager).
(In reply to Olli Pettay [:smaug] from comment #55)
> >+nsresult
> >+PresShell::DispatchEvent(WidgetEvent* aEvent,
> >+                         nsEventStatus* aStatus,
> >+                         nsPresShellEventCB* aEventCB)
> Might be good to call this something else than just DispatchEvent, since we
> have so may other DispatchEvents already. And you could do this kind of code move
> in a separate patch. Maybe the method could be DispatchEventToDOM
This is my third attempt to allocate this code into separate function :-)
Suggestion: DispatchEvent -> DispatchEventToDOM and DispatchTouchEvent -> DispatchTouchEventToDOM
(In reply to Olli Pettay [:smaug] from comment #55)
> >+NS_IMETHODIMP nsFrame::HandleTouchDrag(nsPresContext* aPresContext,
> >+                                       WidgetGUIEvent* aEvent,
> >+                                       nsEventStatus* aEventStatus)
> Hmm, why are we trying to do scrolling in this level when dealing with touch
> events. You want feedback from kats, but
> this really should happen somewhere in APZ level.
> Same applies to TouchManager::HandleTouchDrag
It was the first version. I forgot to remove this code.
All code was moved into TouchManager::HandleTouchDrag
Depends on: 1129397
Attached patch touch_on_desktop_ver4.diff (obsolete) — — Splinter Review 
-Changes: Removed code allocated into bug 1129397
-Changes: Removed unwanted code
+Changes: Added correct detection of RegisterTouchWindow()
+Changes: DRAG -> SCROLL
+Changes: Add check for scrolling related with touch-action property

Comments and suggestions are very welcome!
Attachment #8555933 - Attachment is obsolete: true
Attachment #8555933 - Flags: feedback?(mbrubeck)
Attachment #8559887 - Flags: feedback?(mbrubeck)
Attachment #8559887 - Flags: feedback?(bugs)
Attachment #8559887 - Flags: feedback?(bugmail.mozilla)
(In reply to Olli Pettay [:smaug] from comment #55)
> Please do all the code moves separately in some other bugs. Otherwise
> reviewing the changes will be really difficult.
If any code can be separately moved into m-c before my main stuff, please notify me about this.
Comment on attachment 8559887 [details] [diff] [review]
touch_on_desktop_ver4.diff

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

Moving stuff out into a TouchManager class seems like a good idea but as smaug already said please split this into multiple patches as it is very hard to review. For example you added a bunch of stuff to TouchManger to handle scrolling and looking up the touch-action property that wasn't in PresShell before. Please do that kind of stuff in a patch separate from the one that just moves the existing code from PresShell to TouchManager. Unflagging everybody for feedback since it is a waste of time to have three people look at the patch in this state.

Also note that I would rather not add code to do scrolling based on touch events at all. We are in the process of making APZ work on desktop (including windows) and that will take care of scrolling with touch events.
Attachment #8559887 - Flags: feedback?(mbrubeck)
Attachment #8559887 - Flags: feedback?(bugs)
Attachment #8559887 - Flags: feedback?(bugmail.mozilla)
Depends on: 1133492
May i humbly inquire is there a optimistic target this bug to be fixed for good? We have a product coming out in next months where we support touch gestures to manipulate objects in browser. This works in all other touchscreen laptops (and also in FF with touch enabled). Our biggest hurdle is that unfortunately we have not found a reliable way of telling our customers: you have touchscreen laptop, but touch is disabled by default. Also we cant as far as i know disable pinch-zoom to act as browser zoom. In the result the reaction of user coming from Chrome or IE and pinching in FF is that our product is broken.
(In reply to Priit Pirita from comment #62)
> May i humbly inquire is there a optimistic target this bug to be fixed for
> good? 

It looks like Maksim is working on this. All changes need to land on Nightly first and ride the trains. Assuming this lands in mozilla-central before February 23rd, it *might* ship in Firefox 38, assuming no regressions are found through testing as it rides up the Aurora and Beta branches respectively. If not it will be Firefox 39 or later.

More information about Firefox version scheduling can be found here:
https://wiki.mozilla.org/RapidRelease/Calendar
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #63)
> It looks like Maksim is working on this.
Yes. Comments and suggestions and feadbacks are very welcome.
If anybody can help me with test suite it will help me a lot.
Attached patch touch_on_desktop_ver5.diff (obsolete) — — Splinter Review 
+ Added dispatching events during SCROLL mode
- Removed code allocated in bug 1133492

Comments and suggestions are very welcome!
Attachment #8559887 - Attachment is obsolete: true
Attachment #8565987 - Flags: feedback?(bugs)
Attachment #8565987 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8565987 [details] [diff] [review]
touch_on_desktop_ver5.diff

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

So it seems to me like the whole point of this patch is to implement scrolling in TouchManager based on touch and pointer events, and I already said above that we don't want to be doing that. Instead if you want touch-based scrolling you need to route the touch events from the widget code into the APZ and that will take care of it for you.

::: dom/ipc/TabParent.cpp
@@ +1384,5 @@
>      mChildProcessOffsetAtTouchStart =
>        EventStateManager::GetChildProcessOffset(frameLoader, event);
> +    // !!!_ISSUE
> +    //MOZ_ASSERT((!sEventCapturer && mEventCaptureDepth == 0) ||
> +    //           (sEventCapturer == this && mEventCaptureDepth > 0));

Why is this an issue?

::: layout/base/TouchManager.cpp
@@ +119,5 @@
>            }
>            touch->mMessage = aEvent->message;
>            PresShell::gCaptureTouchList->Put(id, touch);
>          }
> +        MOZ_ASSERT(NOTHING == mState, "How there can be another state?");

If you don't have a useful error message to put in, you don't need to put anything. MOZ_ASSERT(NOTHING == mState) is fine. Same for the other assertions in this file.

@@ +189,5 @@
> +        if (START == mState) {
> +          if (aTouchIsNew) {
> +            mState = FIRST_MOVE;
> +          } else {
> +            MOZ_ASSERT(false, "Can it be happens?");

Better to rewrite this like so:

MOZ_ASSERT(aTouchIsNew);
mState = FIRST_MOVE;

@@ +198,5 @@
> +            NS_WARNING("Can it realy be happens?");
> +          } else {
> +            static bool flag = true;
> +            if (flag)
> +              mState = SCROLL;

What is "flag" for? It is never set to false.

@@ +306,5 @@
> +void
> +TouchManager::HandleScrolling(WidgetTouchEvent* aEvent,
> +                              nsEventStatus* aStatus,
> +                              nsPresShellEventCB* aEventCB,
> +                              bool aTouchIsNew)

As I said before, I don't want us to be doing scrolling based on touch events here in this code; that's what APZ is supposed to be doing.
Attachment #8565987 - Flags: feedback?(bugmail.mozilla) → feedback-
Attached patch touch_on_desktop_ver6.diff (obsolete) — — Splinter Review 
- Removed code allocated in bug 1133492
- Removed HandleScrollEvent with WidgetPointerEvent
+ Added APZ part for nsWindow (nsWindow::DispatchTouchEventForAPZ)
+ Some stuff were allocated into bug 1122090

Comments and suggestions and feadbacks are very welcome.

If any code can be separately moved into m-c with another bugs, please notify me about this.
Attachment #8565987 - Attachment is obsolete: true
Attachment #8565987 - Flags: feedback?(bugs)
Attachment #8571962 - Flags: feedback?(bugs)
Attachment #8571962 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8571962 [details] [diff] [review]
touch_on_desktop_ver6.diff

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

As I told you on IRC if you want to pursue this approach can do so locally but I think it's a waste of time as we don't want to land this in the tree and I'm just going to r- the patches.
Attachment #8571962 - Flags: feedback?(bugs)
Attachment #8571962 - Flags: feedback?(bugmail.mozilla)
Attachment #8571962 - Flags: feedback-
(The above comment is with respect to the changes in TouchManager/presShell). The changes you made to widget look promising, and that's the approach we want to be taking.
Attached patch touch_on_desktop_ver7.diff (obsolete) — — Splinter Review 
- Removed SCROLL mode from TouchManager
- Removed TouchManager::HandleScrolling for WidgetTouchEvent
- Removed changes in ContentHelper
Attachment #8571962 - Attachment is obsolete: true
Attachment #8572658 - Flags: feedback?(bugs)
Attachment #8572658 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8572658 [details] [diff] [review]
touch_on_desktop_ver7.diff

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

::: layout/base/TouchManager.h
@@ +42,5 @@
> +    FIRST_MOVE  = 2,
> +    MOVE        = 3,
> +  };
> +
> +  TouchManagerState     mState;

This state is never used anywhere (except in a couple of MOZ_ASSERTs) and so is unnecessary.

::: widget/windows/nsWindow.cpp
@@ +6238,5 @@
>                    0.0f, 0.0f);
>  
> +      if (apzEnabled) {
> +        inputToDispatch.mTouches.AppendElement(
> +          SingleTouchData(pInputs[i].dwID,                          // aIdentifier

This is the right idea, but this code can be structured better. Right now it's doing all this work to populate touchEventToSend and touchEndEventToSend even if it will never be used (because apzEnabled is true). We can clean this up to just create the MultiTouchInput object, and then if apzEnabled is true, send it to the APZ, otherwise you can just call ToWidgetTouchEvent on the MultiTouchInput and get the WidgetTouchEvent to dispatch using the old mechanism.
Attachment #8572658 - Flags: feedback?(bugmail.mozilla) → feedback+
- Removed TouchManager::TouchManagerState
+ Added nsWindow::onTouch works with MultiTouchInput by default

Comments and suggestions and feadbacks are very welcome.
Attachment #8572658 - Attachment is obsolete: true
Attachment #8572658 - Flags: feedback?(bugs)
Attachment #8575147 - Flags: feedback?(bugs)
Attachment #8575147 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8575147 [details] [diff] [review]
touch_on_desktop_ver8.diff

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

This is looking ok. I think we can clean it up more if we make some other changes to the existing code. For example we can probably get rid of the TryCapture stuff once bug 1116579 is fixed. If we make some slight changes to nsBaseWidget::DispatchAPZAwareEvent that dvander landed recently we can probably reuse that as well for some of this and trim this patch down further.

::: gfx/layers/apz/src/InputQueue.cpp
@@ +37,5 @@
>    switch (aEvent.mInputType) {
>      case MULTITOUCH_INPUT: {
>        const MultiTouchInput& event = aEvent.AsMultiTouchInput();
> +      /*return */ReceiveTouchInput(aTarget, aTargetConfirmed, event, aOutInputBlockId);
> +	  return aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());

What did you need this change for? I assume it will be removed for the final patch, just like the change to nsGlobalWindow.cpp
Attachment #8575147 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #73)
> > +      /*return */ReceiveTouchInput(aTarget, aTargetConfirmed, event, aOutInputBlockId);
> > +	  return aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());
> What did you need this change for? I assume it will be removed for the final
> patch, just like the change to nsGlobalWindow.cpp
I tried debug FF. Look's like, without this changes InputQueue accumulates a bundle of TouchBlocks that's why I cannot see expected behavior (content scrolling). If I make this changes - I see that content can be scrolled. Maybe there is more better way, but I didn't find it yet.
Did you try with or without your patch from bug 1122090?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #75)
> Did you try with or without your patch from bug 1122090?
I tested with changes from bug 1122090. I can provide two links for builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30054e5281bb (without changes in InputQueue.cpp)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fd3770750f8 (with changes in InputQueue.cpp)
(In reply to Maksim Lebedev from comment #74)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #73)
> > > +      /*return */ReceiveTouchInput(aTarget, aTargetConfirmed, event, aOutInputBlockId);
> > > +	  return aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());
> > What did you need this change for? I assume it will be removed for the final
> > patch, just like the change to nsGlobalWindow.cpp
> I tried debug FF. Look's like, without this changes InputQueue accumulates a
> bundle of TouchBlocks that's why I cannot see expected behavior (content
> scrolling). If I make this changes - I see that content can be scrolled.
> Maybe there is more better way, but I didn't find it yet.
Some investigation:
As I understand the main change of status is tracked in AsyncPanZoomController::HandleInputEvent.
This function can be called in CancelableBlockState::DispatchImmediate.
And this function can be called in InputQueue::MaybeHandleCurrentBlock.
But first comparison "block == CurrentBlock()" looks like always FALSE.
That's why APZC never change own status in HandleInputEvent.

Also: AsyncPanZoomController::HandleInputEvent can be called in TouchBlockState::HandleEvents.
And this can be called in InputQueue::ProcessInputBlocks.
But first condition "!curBlock->IsReadyForHandling()" breaks loop in function.
As result APZC cannot change own status in HandleInputEvent again.
(In reply to Maksim Lebedev from comment #77)
> Also: AsyncPanZoomController::HandleInputEvent can be called in
> TouchBlockState::HandleEvents.
> And this can be called in InputQueue::ProcessInputBlocks.
> But first condition "!curBlock->IsReadyForHandling()" breaks loop in
> function.

IsReadyForHandling() should be turning true once the notifications arrive from the main thread. If it's always false that means the notifications are not arriving, you should look into that.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #78)
> IsReadyForHandling() should be turning true once the notifications arrive
> from the main thread. If it's always false that means the notifications are
> not arriving, you should look into that.
Investigation:
CancelableBlockState* curBlock = CurrentBlock() returns first block in inputQueue.
But current process block (with current inputBlockId) can be not first, that's why IsReadyForHandling always returns false by the reason mAllowedTouchBehaviorSet = false.
It means that if inputQueue detect first block without behavior - after it inputQueue never handle this block and after it all another blocks will be accumulated in queue (and never will be processed)

That behavior can be set when mAPZC->ReceiveInputEvent() receives nsEventStatus_eConsumeNoDefault and after it this start event can be droped. All next events does not call mAPZC->SetAllowedTouchBehavior() and this block will be without behavior.
It can happen when we put 'touch' during previous scrolling. In this case 
InputQueue::ReceiveTouchInput()
  if (block->IsDuringFastMotion()) {
    result = nsEventStatus_eConsumeNoDefault;
and after it user loses ability to scroll page forever.

Solution:
Change place of detection behavior before drop events (This stuff is situated in bug 1122090).
Thanks for looking into that, it looks like a bug in the InputQueue code. I'll put together a patch.
Depends on: 1143618
Blocks: 1143655
I filed bug 1144112 for the issue you were seeing in comment 79 and attached some patches. While doing this I also did a build for windows and played around with it. I think there's a lot more we can do to clean up the code and make it easier to enable APZ and pointer events on windows, I'll file additional bugs with patches.
Comment on attachment 8575147 [details] [diff] [review]
touch_on_desktop_ver8.diff

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

This patch is pretty much obsolete now.

::: dom/base/nsGlobalWindow.cpp
@@ +9886,5 @@
>    if (!mainWidget) {
>      return;
>    }
>  
> +  if (mMayHaveTouchEventListener || Preferences::GetInt("dom.w3c_touch_events.enabled", 0)) {

In bug 1144324 I'm getting rid of this code so you shouldn't need this workaround any more.

::: gfx/layers/apz/src/InputQueue.cpp
@@ +37,5 @@
>    switch (aEvent.mInputType) {
>      case MULTITOUCH_INPUT: {
>        const MultiTouchInput& event = aEvent.AsMultiTouchInput();
> +      /*return */ReceiveTouchInput(aTarget, aTargetConfirmed, event, aOutInputBlockId);
> +	  return aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());

The patches in bug 1122090 should make this change unnecessary.

::: widget/windows/nsWindow.cpp
@@ +4063,5 @@
> +  // and the child process will take care of responding to the event as needed
> +  // so we don't need to do anything else here.
> +  if (TabParent* capturer = TabParent::GetEventCapturer()) {
> +    InputAPZContext context(aGuid, aInputBlockId);
> +    if (capturer->TryCapture(touchEvent)) {

This event-capturer code has been deleted from the tree. The rest of this function is basically equivalent to nsBaseWidget::DispatchAPZAwareEvent which dvander added recently. In bug 1144324 I make use of that function to do what we need here.

@@ +6177,4 @@
>      nsEventStatus status;
> +    MultiTouchInput inputToDispatch;
> +    inputToDispatch.mInputType = MULTITOUCH_INPUT;
> +    bool initialized = false;

You extracted this conversion to bug 1143618 which I'll review in a bit.
Attachment #8575147 - Flags: feedback?(bugs)
Attachment #8575147 - Flags: feedback+
Depends on: 1153935
I'm seeing this on all sites and I'm running Ubuntu.
This bug is for Windows. For Linux support of touch events you want bug 978679.
No longer blocks: 1180706
Depends on: 1180706
So AFAIK this bug shouldn't be open any more, because all sites currently scroll with a touch-screen, and no sites get access to DOM touch events. (Speaking only of Windows right now).
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: