Closed Bug 814252 Opened 12 years ago Closed 12 years ago

Scrolling with a focused input forces scroll top

Categories

(Firefox OS Graveyard :: General, defect, P1)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: alberto.pastor, Assigned: schien)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

stack trace for auto scroll
8.06 KB, text/plain
Details
stack trace for SetScrollTop
14.19 KB, text/plain
Details
Should not process HandleDrag for touch event, v3
2.63 KB, patch
schien
: review+
Details | Diff | Splinter Review 
STR:

1.- Open Contacts app
2.- Click + to add a contact
3.- Click on a field
4.- Try to scroll, starting the event from inside the input

Expected:

It scrolls normally

Actual:

It scrolls to the top.

Comments:

I created a test case in https://github.com/albertopq/gaia/tree/scrolling-form-issues
Is the last option in UITests app.
The problem is related to the scrollable aera not being in top:0. VKB problems are discarded. I already tried to remove the Keyboard from gaia and it still happens.
blocking-basecamp: --- → ?
I found that there are two additional scrollings before and after we invoke the |scrollCallback| in BrowserElementScrolling.js. The scrolling distance of these unexpected scrolling is equal to the distance between the top of screen and the top of the scrollable area. 

here is the call stack for the unexpected scrolling:
#0  nsGfxScrollFrameInner::ScrollToWithOrigin (this=0x444e9680, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT, 
    aOrigin=0x42605c00, aRange=0xbed66300)
    at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsGfxScrollFrame.cpp:1572
#1  0x404d8ecc in nsGfxScrollFrameInner::ScrollTo (this=<value optimized out>, aScrollPosition=<value optimized out>, 
    aMode=nsIScrollableFrame::INSTANT, aRange=<value optimized out>)
    at /home/hellfire/workspace/hg/mozilla-central/layout/forms/../generic/nsGfxScrollFrame.h:167
#2  nsHTMLScrollFrame::ScrollTo (this=<value optimized out>, aScrollPosition=<value optimized out>, 
    aMode=nsIScrollableFrame::INSTANT, aRange=<value optimized out>)
    at /home/hellfire/workspace/hg/mozilla-central/layout/forms/../generic/nsGfxScrollFrame.h:484
#3  0x404c5774 in ScrollToShowRect (this=<value optimized out>, aFrame=0x444e9630, aRect=<value optimized out>, 
    aVertical=<value optimized out>, aHorizontal=..., aFlags=0)
    at /home/hellfire/workspace/hg/mozilla-central/layout/base/nsPresShell.cpp:3209
#4  PresShell::ScrollFrameRectIntoView (this=<value optimized out>, aFrame=0x444e9630, aRect=<value optimized out>, 
    aVertical=<value optimized out>, aHorizontal=..., aFlags=0)
    at /home/hellfire/workspace/hg/mozilla-central/layout/base/nsPresShell.cpp:3349
#5  0x4050a03e in mozilla::Selection::DoAutoScroll (this=0x444ea440, aFrame=0x444e97d0, aPoint=<value optimized out>)
    at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsSelection.cpp:4382
#6  0x4050a134 in mozilla::Selection::StartAutoScrollTimer (this=0x444ea440, aFrame=0x444e97d0, aPoint=..., aDelay=30)
    at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsSelection.cpp:4348
#7  0x4050a166 in nsFrameSelection::StartAutoScrollTimer (this=0x431e5700, aFrame=0x444e97d0, aPoint=..., aDelay=30)
    at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsSelection.cpp:1535
#8  0x404efd84 in nsFrame::HandleDrag (this=<value optimized out>, aPresContext=<value optimized out>, aEvent=0xbed66908, 
    aEventStatus=<value optimized out>) at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsFrame.cpp:3083
#9  0x404e9552 in nsFrame::HandleEvent (this=0x444ea440, aPresContext=0x444e97d0, aEvent=0x0, aEventStatus=0x42605c00)
    at /home/hellfire/workspace/hg/mozilla-central/layout/generic/nsFrame.cpp:2423
#10 0x404c3ba0 in nsPresShellEventCB::HandleEvent (this=<value optimized out>, aVisitor=...)
    at /home/hellfire/workspace/hg/mozilla-central/layout/base/nsPresShell.cpp:496
#11 0x4063b624 in nsEventTargetChainItem::HandleEventTargetChain (this=<value optimized out>, aVisitor=..., aFlags=6, 
    aCallback=0xbed66600, aMayHaveNewListenerManagers=false, aPusher=0xbed66580)
    at /home/hellfire/workspace/hg/mozilla-central/content/events/src/nsEventDispatcher.cpp:362
#12 0x4063badc in nsEventDispatcher::Dispatch (aTarget=<value optimized out>, aPresContext=0x4438dc00, aEvent=0xbed66908,
If it helps, a long press before start scolling, makes it work.
Maybe related to bug 806596?
blocking-basecamp: ? → +
(In reply to Alberto Pastor from comment #2)
> If it helps, a long press before start scolling, makes it work.
Gecko will invoke nsEventStateManager::sClickHoldCallback 500ms later after the |touch start| event, which will disable auto scrolling in later |touch move| event.
So, I think the root cause is we somehow give a wrong scrolling distance on Y axis during auto scrolling.
roc, could you provide some advices on this bug?
Flags: needinfo?(roc)
StartAutoScrollTimer controls the behavior where you click in a Web page and then drag outside the page and we scroll the page to bring the dragged-over content into view. Do we want that to happen at all for B2G? It certainly shouldn't happen while we're doing a regular scrolling gesture.

Actually, if you're trying to make a scroll gesture, I would hope that we don't even hit nsFrame::HandleDrag. We shouldn't be dispatching mouse-moves to the app while scrolling. I don't know the code that translates touch events to mouse events, or identifies scrolling gestures.

Maybe these events are synthetic mouse events? I thought we turned those off for B2G though. A full stack would be helpful.
Flags: needinfo?(roc)
Tentatively assigned to shien.
Assignee: nobody → schien
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> StartAutoScrollTimer controls the behavior where you click in a Web page and
> then drag outside the page and we scroll the page to bring the dragged-over
> content into view. Do we want that to happen at all for B2G? It certainly
> shouldn't happen while we're doing a regular scrolling gesture.
I don't think we can drag elements outside the page on B2G, disabling auto scrolling on B2G could be a safer way solving this bug.
> 
> Actually, if you're trying to make a scroll gesture, I would hope that we
> don't even hit nsFrame::HandleDrag. We shouldn't be dispatching mouse-moves
> to the app while scrolling. I don't know the code that translates touch
> events to mouse events, or identifies scrolling gestures.
> 
> Maybe these events are synthetic mouse events? I thought we turned those off
> for B2G though. A full stack would be helpful.
Yes, the mouse events are synthetic from touch event in TabChild::RecvRealTouchEvent.
http://dxr.mozilla.org/mozilla-central/dom/ipc/TabChild.cpp.html#l1370
The scroll gesture is detected by frame script |BrowserElementScrolling.js|, which listens these synthetic mouse events. On B2G platform, we'll trigger nsFrame::HandleDrag first and then enter the BrowserElementScrolling.js for gesture detection.

Full stack for auto scrolling and scroll gesture are in attachments.
The stack in comment #7 indicates that these are actually touch events triggering the AutoScrollTimer (see frame 8, PresShell::DispatchTouchEvent). nsFrame::HandleEvent calls HandleDrag on touch events, which I didn't know before.

So what happens is that the touch events trigger AutoScrollTimer and then later the synthetic mouse events generated from those touch events trigger BrowserElementScrolling.

I think we should fix this by having BrowserElementScrolling listen for touch events on touch-enabled platforms, and for mouse events on non-touch-enabled platforms (controlled by a pref, if necessary). onTouchMove calls stopPropagation and preventDefault on the event, which will stop nsFrame::HandleDrag from being called.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I think we should fix this by having BrowserElementScrolling listen for
> touch events on touch-enabled platforms, and for mouse events on
> non-touch-enabled platforms (controlled by a pref, if necessary).
> onTouchMove calls stopPropagation and preventDefault on the event, which
> will stop nsFrame::HandleDrag from being called.
I tried to use touch event in BrowserElementScrolling and found |stopPropagation| and |preventDefault| will not stop nsFrame::HandleDrag from being called.
The reason is that nsPresShellEventCB, which invoke nsFrame::HandleDrag, is in system event group and will discard the NS_EVENT_FLAG_STOP_DISPATCH flag.
http://dxr.mozilla.org/mozilla-central/content/events/src/nsEventDispatcher.cpp.html#l368
I'll need to find another solution.
OK, so nsFrame::HandleEvent still gets called, but we can still access the preventDefault state via the event status. So how about in nsFrame::HandleDrag, after frameselection->StopAutoScrollTimer, add
  if (nsEventStatus_eConsumeNoDefault == *aEventStatus) {
    return NS_OK;
?

This is a behavior change for the Web, in particular it means that preventDefault on a mousemove event would prevent drag-selection as well as auto-scroll. But I think that's OK --- in fact, I think that's good. (Note that nsFrame::HandlePress and nsFrame::HandleRelease do the same check and their selection behaviors are completely disabled by preventDefault.) We could try this on trunk for all platforms and wrap it in MOZ_B2G for Aurora/Beta.
preventDefault on a mousemove event disables selection in IE9, but it doesn't in Chrome and Opera.
Here is my first attempt of using touch event in BrowserElementScrolling.js. This patch will successfully disable auto scrolling and allows us panning on top of a input element. However, it will affect the panning/zooming gesture in browser app. I discovery some strange behavior:
1. Sometimes, swiping over an inner scrollable area in web page causes both web page and inner scrollable area to scroll. (I expect only inner scrollable area will be scrolled)
2. pinch gesture will not take effect if any of your finger is point on an inner scrollable area.

@cjones, I need some guidance about defining the interaction between BrowserElementScrolling and AsyncPanZoomController
Attachment #685555 - Flags: feedback?(roc)
Attachment #685555 - Flags: feedback?(jones.chris.g)
Shih-Chiang, it might be a good idea to break out your nsFrame patch and get review on it from Olli Pettay so we can get it landed ASAP. I think it's a good change even if we end up not depending on it to fix this bug.
(In reply to Shih-Chiang Chien [:schien] from comment #14)
> Created attachment 685555 [details] [diff] [review]
> WIP - prevent auto scrolling while detecting panning/scrolling gesture
> 
> @cjones, I need some guidance about defining the interaction between
> BrowserElementScrolling and AsyncPanZoomController

Can you point me at the test cases you're using?  I'd like to get a feel for these bugs.
Comment on attachment 685555 [details] [diff] [review]
WIP - prevent auto scrolling while detecting panning/scrolling gesture

This change seems mostly OK but it'll break panning in desktop-b2g builds.  Please try to find a way to not break that if you can.
Attachment #685555 - Flags: feedback?(jones.chris.g) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Shih-Chiang, it might be a good idea to break out your nsFrame patch and get
> review on it from Olli Pettay so we can get it landed ASAP. I think it's a
> good change even if we end up not depending on it to fix this bug.

I file bug 815943 for the nsFrame part of modification.
1. handle both touch and mouse event in BrowserElementScrolling, in order to adapt both phone and desktop environment.
2. should continue gesture detection even if cancel-async-pan-zoom, so that we can do pinch/tap in browser app.
Attachment #685555 - Attachment is obsolete: true
Attachment #686059 - Flags: feedback?(jones.chris.g)
Comment on attachment 686059 [details] [diff] [review]
WIP - prevent auto scrolling while detecting panning/scrolling gesture, v2

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js

>   handleEvent: function cp_handleEvent(evt) {
>+    switch (this.evtType) {
>+      case 'touch':
>+        switch (evt.type) {
>+          case 'mousedown':
>+          case 'mousemove':
>+          case 'mouseup':
>+            return;
>+        }
>+        break;
>+      case 'mouse':
>+        switch (evt.type) {
>+          case 'touchstart':
>+          case 'touchmove':
>+          case 'touchend':
>+            return;
>+        }
>+      default:
>+        if (evt.type === 'touchstart') {
>+          this.evtType = 'touch';
>+        } else if (evt.type === 'mousedown') {
>+          this.evtType = 'mouse';
>+        }
>+    }

Hm, I'm not entirely sure what this code is trying to do.  It looks
like it's trying to filter "unexpected" events, but I don't believe
this code is correct for that.

A comment would help a lot :).

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

> nsEventStatus AsyncPanZoomController::HandleInputEvent(const InputData& aEvent) {
>   nsEventStatus rv = nsEventStatus_eIgnore;
> 
>-  if (mGestureEventListener && !mDisableNextTouchBatch) {
>+  if (mGestureEventListener) {

Can you explain this change?  We need this check to implement
synchronous scrolling of sub-frames in "browser tabs", and to allow
content in "browser tabs" to process touch events themselves.  So I'm
not sure this is correct.

I don't understand the other code very well now.
Attachment #686059 - Flags: feedback?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Comment on attachment 686059 [details] [diff] [review]
> WIP - prevent auto scrolling while detecting panning/scrolling gesture, v2
> 
> >diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js
> 
> >   handleEvent: function cp_handleEvent(evt) {
> >+    switch (this.evtType) {
> >+      case 'touch':
> >+        switch (evt.type) {
> >+          case 'mousedown':
> >+          case 'mousemove':
> >+          case 'mouseup':
> >+            return;
> >+        }
> >+        break;
> >+      case 'mouse':
> >+        switch (evt.type) {
> >+          case 'touchstart':
> >+          case 'touchmove':
> >+          case 'touchend':
> >+            return;
> >+        }
> >+      default:
> >+        if (evt.type === 'touchstart') {
> >+          this.evtType = 'touch';
> >+        } else if (evt.type === 'mousedown') {
> >+          this.evtType = 'mouse';
> >+        }
> >+    }
> 
> Hm, I'm not entirely sure what this code is trying to do.  It looks
> like it's trying to filter "unexpected" events, but I don't believe
> this code is correct for that.
> 
> A comment would help a lot :).
What I'm trying to do here is to detect whether this platform using touch event or not. I don't know if there is any other efficient way to identify a touch-enabled device, so I need to check whether touch event or mouse event is sent first. I'll refine these code.
> 
> >diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp
> 
> > nsEventStatus AsyncPanZoomController::HandleInputEvent(const InputData& aEvent) {
> >   nsEventStatus rv = nsEventStatus_eIgnore;
> > 
> >-  if (mGestureEventListener && !mDisableNextTouchBatch) {
> >+  if (mGestureEventListener) {
> 
> Can you explain this change?  We need this check to implement
> synchronous scrolling of sub-frames in "browser tabs", and to allow
> content in "browser tabs" to process touch events themselves.  So I'm
> not sure this is correct.
If gesture detection is stopped while "browser tabs" processing touch event, we will not be able to zoom in page by pinch gesture. 

Currently in browser app, pinch works well because cancel-async-pan-zoom will never happen because the target of mouse event received in BrowserElementScrolling is always the <iframe mozbrowser>. Therefore, this pinch problem is hidden by the bug of not-cancel-async-pan-zoom.
> 
> I don't understand the other code very well now.
(In reply to Shih-Chiang Chien [:schien] from comment #21)
> What I'm trying to do here is to detect whether this platform using touch
> event or not. I don't know if there is any other efficient way to identify a
> touch-enabled device, so I need to check whether touch event or mouse event
> is sent first. I'll refine these code.

('TouchEvent' in window) will tell you if touch events are supported.  If they are, you'll always see touch events first.  However, you may also see synthetic mouse events.

If touch events aren't supported, you'll never see them.

> > >-  if (mGestureEventListener && !mDisableNextTouchBatch) {
> > >+  if (mGestureEventListener) {
> > 
> > Can you explain this change?  We need this check to implement
> > synchronous scrolling of sub-frames in "browser tabs", and to allow
> > content in "browser tabs" to process touch events themselves.  So I'm
> > not sure this is correct.
> If gesture detection is stopped while "browser tabs" processing touch event,
> we will not be able to zoom in page by pinch gesture. 
> 

That's correct.  When web pages process touch events themselves, or the user attempts to pan a nested frame, we want to disable gesture detection.

> Currently in browser app, pinch works well because cancel-async-pan-zoom
> will never happen because the target of mouse event received in
> BrowserElementScrolling is always the <iframe mozbrowser>.

Did you test with pages that have touch-event listeners or scrollable sub-frames?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> (In reply to Shih-Chiang Chien [:schien] from comment #21)
> > What I'm trying to do here is to detect whether this platform using touch
> > event or not. I don't know if there is any other efficient way to identify a
> > touch-enabled device, so I need to check whether touch event or mouse event
> > is sent first. I'll refine these code.
> 
> ('TouchEvent' in window) will tell you if touch events are supported.  If
> they are, you'll always see touch events first.  However, you may also see
> synthetic mouse events.
> 
> If touch events aren't supported, you'll never see them.
> 
> > > >-  if (mGestureEventListener && !mDisableNextTouchBatch) {
> > > >+  if (mGestureEventListener) {
> > > 
> > > Can you explain this change?  We need this check to implement
> > > synchronous scrolling of sub-frames in "browser tabs", and to allow
> > > content in "browser tabs" to process touch events themselves.  So I'm
> > > not sure this is correct.
> > If gesture detection is stopped while "browser tabs" processing touch event,
> > we will not be able to zoom in page by pinch gesture. 
> > 
> 
> That's correct.  When web pages process touch events themselves, or the user
> attempts to pan a nested frame, we want to disable gesture detection.
It's kind wired to me since we don't detect pinch in BrowserElementScrolling. This means if you zoom in to an iframe in browser app, you'll not be able to zoom out via pinch since touch event in iframe disable the AsyncPanZoomController.
> 
> > Currently in browser app, pinch works well because cancel-async-pan-zoom
> > will never happen because the target of mouse event received in
> > BrowserElementScrolling is always the <iframe mozbrowser>.
> 
> Did you test with pages that have touch-event listeners or scrollable
> sub-frames?
I test this patch with these two sites.
http://people.mozilla.org/~cjones/scrolling.html
http://canvas.sjmorrow.com/
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22) 
> ('TouchEvent' in window) will tell you if touch events are supported.  If
> they are, you'll always see touch events first.
Unfortunately not true for example on Win8 devices which may have touchpad and touchscreen.
You may get mouse events, or you may get touch events and mouse events.
(In reply to Shih-Chiang Chien [:schien] from comment #23)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> > (In reply to Shih-Chiang Chien [:schien] from comment #21)
> > > What I'm trying to do here is to detect whether this platform using touch
> > > event or not. I don't know if there is any other efficient way to identify a
> > > touch-enabled device, so I need to check whether touch event or mouse event
> > > is sent first. I'll refine these code.
> > 
> > ('TouchEvent' in window) will tell you if touch events are supported.  If
> > they are, you'll always see touch events first.  However, you may also see
> > synthetic mouse events.
> > 
> > If touch events aren't supported, you'll never see them.
> > 
> > > > >-  if (mGestureEventListener && !mDisableNextTouchBatch) {
> > > > >+  if (mGestureEventListener) {
> > > > 
> > > > Can you explain this change?  We need this check to implement
> > > > synchronous scrolling of sub-frames in "browser tabs", and to allow
> > > > content in "browser tabs" to process touch events themselves.  So I'm
> > > > not sure this is correct.
> > > If gesture detection is stopped while "browser tabs" processing touch event,
> > > we will not be able to zoom in page by pinch gesture. 
> > > 
> > 
> > That's correct.  When web pages process touch events themselves, or the user
> > attempts to pan a nested frame, we want to disable gesture detection.
> It's kind wired to me since we don't detect pinch in
> BrowserElementScrolling. This means if you zoom in to an iframe in browser
> app, you'll not be able to zoom out via pinch since touch event in iframe
> disable the AsyncPanZoomController.

Hm, I don't think we should be disabling the zoom gesture in that case.  Can you work out a test case and see what happens?

> > 
> > > Currently in browser app, pinch works well because cancel-async-pan-zoom
> > > will never happen because the target of mouse event received in
> > > BrowserElementScrolling is always the <iframe mozbrowser>.
> > 
> > Did you test with pages that have touch-event listeners or scrollable
> > sub-frames?
> I test this patch with these two sites.
> http://people.mozilla.org/~cjones/scrolling.html
> http://canvas.sjmorrow.com/

OK cool.
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> (In reply to Shih-Chiang Chien [:schien] from comment #23)
> > It's kind wired to me since we don't detect pinch in
> > BrowserElementScrolling. This means if you zoom in to an iframe in browser
> > app, you'll not be able to zoom out via pinch since touch event in iframe
> > disable the AsyncPanZoomController.
> 
> Hm, I don't think we should be disabling the zoom gesture in that case.  Can
> you work out a test case and see what happens?
Here is the test case:
1. open browser app and connect to http://people.mozilla.org/~cjones/scrolling.html 
2. put left thumb on the yellow background area and put right thumb on white background area
3. start to pinch

In step 2, cancel-default-pan-zoom will be fired while you touch the yellow background area, which is a inner-scrollable <div>. Therefore, AsyncPanZoomController::mDisableNextTouchBatch will be set to true and GestureEventListener::mState will change from GESTURE_WAITING_PINCH to GESTURE_NONE. That's how we disable zoom gesture in this case.

However, if you re-touch the white background area again, APZC will be enabled because mDisabledNextTouchBatch is set to false after invoking AsyncPanZoomController::OnTouchFinish. Then, you'll be able to do the zoom gesture.

I think firing cancel-default-pan-zoom at touchstart/mousedown is too early since we don't know what gesture will be perform next. I'll mark the WIP to obsolete since it is not a correct approach.
Attachment #686059 - Attachment is obsolete: true
OK, that makes sense.  It would be nice to continue a pinch-zoom in this case, if nothing else (like a content touch-event listener) preventDefault()s the touch.
NOTE: Pinch problem mentioned in comment 28 is not solved in this patch. I found it hard to find a timing before APZC detecting panning gesture but after touchstart event fired to cancel the default pan-zoom action.
Attachment #688210 - Flags: feedback?(jones.chris.g)
Comment on attachment 688210 [details] [diff] [review]
WIP - prevent auto scrolling while detecting panning/scrolling gesture, v3

Thinking more about this approach, it's going to have a big negative impact on browser-tab panning, because the async pan-zoom controller will always think that there are content touch-event listeners.  That forces a round-trip through content for each touch series.

We need to only enable this code when we're not async pan-zooming.  Otherwise, the approach seems ok.
Attachment #688210 - Flags: feedback?(jones.chris.g) → feedback-
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> Comment on attachment 688210 [details] [diff] [review]
> WIP - prevent auto scrolling while detecting panning/scrolling gesture, v3
> 
> Thinking more about this approach, it's going to have a big negative impact
> on browser-tab panning, because the async pan-zoom controller will always
> think that there are content touch-event listeners.  That forces a
> round-trip through content for each touch series.
hmm, I found that mFrameMetrics.mMayHaveTouchEventListeners is still false while we listening touch event in BrowserElementScrolling.
> 
> We need to only enable this code when we're not async pan-zooming. 
> Otherwise, the approach seems ok.
If we only use async pan-zooming in browser app, then we'll need to solve bug 817845 in AsyncPanZoomController and I'm not sure how to do it. I thought scrolling inner scrollable region in browser app should be taken care of by BrowserElementScrolling?
(In reply to Shih-Chiang Chien [:schien] from comment #32)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > Comment on attachment 688210 [details] [diff] [review]
> > WIP - prevent auto scrolling while detecting panning/scrolling gesture, v3
> > 
> > Thinking more about this approach, it's going to have a big negative impact
> > on browser-tab panning, because the async pan-zoom controller will always
> > think that there are content touch-event listeners.  That forces a
> > round-trip through content for each touch series.
> hmm, I found that mFrameMetrics.mMayHaveTouchEventListeners is still false
> while we listening touch event in BrowserElementScrolling.

OK, great!

> > 
> > We need to only enable this code when we're not async pan-zooming. 
> > Otherwise, the approach seems ok.
> If we only use async pan-zooming in browser app, then we'll need to solve
> bug 817845 in AsyncPanZoomController and I'm not sure how to do it. I
> thought scrolling inner scrollable region in browser app should be taken
> care of by BrowserElementScrolling?

That's a slightly separate problem that can have a different solution, but if this isn't going to have a performance impact let's keep going with it.
1. using touch event to handle scrolling gesture on phone.
2. delay |cancel-default-pan-zoom| notification until we really can do scrolling on a subframe.
3. introduce |detect-scrollable-subframe| notification, send while we are touching on a scrollable subframe. APZC need to pending the first touch move event and let BrowserElementScrolling handle it first.

NOTE: with this patch, we are able to scroll the entire page while we've reach the end of scrollable subframe (like what we solved in bug 805638).
Attachment #688210 - Attachment is obsolete: true
Attachment #690110 - Flags: review?(jones.chris.g)
Try run for 71c7b741c89c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=71c7b741c89c
Results (out of 256 total builds):
    exception: 1
    success: 231
    warnings: 24
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-71c7b741c89c
Making this a C2 bug per triage.
Target Milestone: --- → B2G C2 (20nov-10dec)
Comment on attachment 690110 [details] [diff] [review]
using touch event to handle scrolling gesture in content process, v1

OK, this all makes sense now :).

The patch looks good.
Attachment #690110 - Flags: review?(jones.chris.g) → review+
hi :schien, please land this.  it's running a day behind from C2 completion.  thanks!
please help check-in this patch after bug 815943, thanks.
Keywords: checkin-needed
Attachment #690110 - Attachment description: WIP - using touch event to handle scrolling gesture in content process, v1 → using touch event to handle scrolling gesture in content process, v1
Question - is bug 818575 a dupe of this bug?
(In reply to Jason Smith [:jsmith] from comment #41)
> Question - is bug 818575 a dupe of this bug?
No, this patch cannot solve bug 818575.
https://hg.mozilla.org/mozilla-central/rev/34f4ca8d630c
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 819726
No simple fix is at hand, and this might be an unrelated casualty of bug 822558.  Time to back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
NOTE: Please do not reland without first fixing the regression tracked in bug 821583.
We need to resolve the broken features in bug 821583 bug 821864 bug 822336 bug 822501 bug 822590 bug 822592 and bug 822895.

Thankfully, except for bug 822592, these all seem to be due to bug 822558 which I'm investigating.
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
Shih-Chiang, you have a lot of blockers loaded on you for platform and Gaiai - Matt can you help untangle what needs to happen here?
Assignee: schien → matt.woodrow
There doesn't seem to be anything wrong with this patch.  Bug 822558 seems to be breaking it.
My theory is that we're not tracking touch IDs correctly.
I don't really know this code at all. Cjones seems to know what's happening :)
Assignee: matt.woodrow → nobody
Bug 822558 wasn't it.

So what we're after here is a difference in touch-event processing between m-c and aurora/b2g18 that incidentally fixed bug 822558.  Luckily, we can do a reverse bisect :).
(In reply to Ed Morley (Away 20th Dec-2nd Jan) [UTC+0; email:edmorley@moco] from comment #45)
> https://hg.mozilla.org/mozilla-central/rev/34f4ca8d630c

While bisecting, I just hit a commit after this but before bug 813445.  I'm guessing the bisect will find bug 813445, in which case we need to fix this patch.
Yep

The first bad revision is:
changeset:   116171:c7115b71fbb1
user:        Masayuki Nakano <masayuki@d-toybox.com>
date:        Sun Dec 16 10:26:03 2012 +0900
summary:     Bug 813445 part.1 Make widget::EventFlags and remove NS_EVENT_FLAG_TRUSTED r=roc+smaug, feedback=emk
(In reply to JP Rosevear [:jpr] from comment #51)
> Shih-Chiang, you have a lot of blockers loaded on you for platform and Gaiai
> - Matt can you help untangle what needs to happen here?
Most of my blockers are related to this bug. :(

I see two kinds of problem in these regressions:
1. touch event been consumed by BrowserElementScrolling: Basically, all our web apps are still using mouse move event. Mouse move events will be synthesized if preventDefault() is not invoked on the source touch move events. If the mouse move event listener is targeted on an element within a scrollable area, BrowserElementScrolling will consume the touch move events and web apps will not have the mouse move event they expected.

2. click event been consumed by BrowserElementScrolling: A short and quick swipe gesture will generate an additional click event. Mouse down events will carry the information about the click count for filtering the unwanted click event, however, we don't have the corresponding information in touch end event. So, the first click event after touch end event will mostly be recognized as the unwanted click event and filter it out.
1) Do we need a little bit of leeway in the event that a person's finger slips?

2) as per schien, I think we had some issues with touch events eating events before when we had a issue/fix in firefox for android.

I think wesj worked on this part?  I can't recall.
(In reply to Shih-Chiang Chien [:schien] from comment #59)
> (In reply to JP Rosevear [:jpr] from comment #51)
> > Shih-Chiang, you have a lot of blockers loaded on you for platform and Gaiai
> > - Matt can you help untangle what needs to happen here?
> Most of my blockers are related to this bug. :(

Okay, back to you :)
Assignee: nobody → schien
The original problem is caused by an incorrect type conversion.

We static cast all nsGUIEvent* to nsMouseEvent* in nsFrame::HandleDrag.
http://dxr.mozilla.org/mozilla-central/layout/generic/nsFrame.cpp.html#l3047
Attachment #690110 - Attachment is obsolete: true
Attachment #694665 - Flags: review?(bugs)
No longer depends on: 822389
Comment on attachment 694665 [details] [diff] [review]
Should not process HandleDrag for touch event, v1

yikes. Regression from bug 732052.
Have you pushed the patch to tryserver?
Attachment #694665 - Flags: review?(wjohnston)
Attachment #694665 - Flags: review?(bugs)
Attachment #694665 - Flags: review+
Comment on attachment 694665 [details] [diff] [review]
Should not process HandleDrag for touch event, v1

Actually, we should just assert in HandleDrag (MOZ_ASSERT)
and never call HandleDrag with wrong event struct.
So change nsFrame::HandleEvent and push the patch to tryserver.
Attachment #694665 - Flags: review+ → review-
1. do not invoke HandleDrag for touch event in nsFrame::HandleEvent
2. use NS_ASSERTION to check eventStructType in nsFrame::HandleDrag

try server link
https://tbpl.mozilla.org/?tree=Try&rev=d326f80fcff0
Attachment #694665 - Attachment is obsolete: true
Attachment #694665 - Flags: review?(wjohnston)
Attachment #694887 - Flags: review?(bugs)
Comment on attachment 694887 [details] [diff] [review]
Should not process HandleDrag for touch event, v2

Use MOZ_ASSERT, please. Wesj should see this change too.
Attachment #694887 - Flags: review?(bugs) → review+
Try run for 71c7b741c89c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=71c7b741c89c
Results (out of 257 total builds):
    exception: 2
    success: 231
    warnings: 24
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-71c7b741c89c
1. update patch according to comment 66, carry r+
Attachment #694887 - Attachment is obsolete: true
Attachment #695074 - Flags: review+
Attachment #695074 - Flags: feedback?(wjohnston)
https://hg.mozilla.org/mozilla-central/rev/30555d40da20
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment on attachment 695074 [details] [diff] [review]
Should not process HandleDrag for touch event, v3

I have a feeling someday we'll want this back, but we can do it that day (and more carefully than I did).
Attachment #695074 - Flags: feedback?(wjohnston)
No longer blocks: 819726
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: