Closed Bug 950300 Opened 11 years ago Closed 11 years ago

The main panel of the solitary view of "12 Games in 1" is pannable while playing

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch for check-in
4.05 KB, patch
Details | Diff | Splinter Review 
Steps to reproduce:
 - Install the app at https://marketplace.firefox.com/app/12-games-in-1?src=search
 - Alternatively open the marketplace from your FirefoxOS device and search "12 games" in the search box in order to install it
 - Once the game is installed, launch it
 - Pan to the bottom to launch the 'solitary'
 - try to move one of the card

Expected result:
 - The card is moved

Actual result:
 - The card is moved and the panel is scrolling by some pixels


I tried to load the game into an other 320x480 pixel device that support viewport and the game works fine.
Will take a look, assuming botond is too busy/ill.
blocking-b2g: --- → 1.3?
hmm, I can't reproduce this on a Geeksphone Peak and it's the only phone I have with me right now - I'll make sure to bring one of the 320x480 phones with me tomorrow.
I see it on unagi.  Chris, if you manage to reproduce this, please grab the bug.
Assignee: nobody → botond
blocking-b2g: 1.3? → 1.3+
I looked a bit into it and by loading http://ks388498.kimsufi.com/mozilla/12in1/games/solitaire/index.html directly inside chrome devtools to emulate viewport i can see a scrollbar when I set the size to 320x460 which seems what we are rendering too.

But with chrome devtools I can't pan the page. So I wonder if the game is doing something special to cancel scrolling that make us fail.
There is a <body id="game-container" ontouchmove="event.preventDefault();"> at the beginning of the script.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #5)
> There is a <body id="game-container" ontouchmove="event.preventDefault();">
> at the beginning of the script.

Sigh. From what I remember the spec says that browser scrolling can be stopped from the touchstart or the first touchmove. Seems like we ignored it here. With a dirty hack in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#464 by checking TOUCH_MOVE as well then the app works fine.

I will try to see if I can provide a patch that check just the first touchmove.
I think a more correct solution might be to modify this TabChild code to send ContentReceivedTouch only if touchstart is prevented or the first touchmove has been dispatched:
http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1910

This is what MetroInput does, using the mCancelable flag in this code:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#1179

Keeping this logic on the "child" side will reduce the number of messages, because it will send only one ContentReceivedTouch per touch session (instead of one per touch event as it does now).
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> I think a more correct solution might be to modify this TabChild code to
> send ContentReceivedTouch only if touchstart is prevented or the first
> touchmove has been dispatched:
> http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1910

To clarify, the bug is caused because TabChild sends a ContentReceivedTouch after the first touchstart event is dispatched, which cancels the waiting state and causes subsquent ContentReceivedTouch calls to be ignored.
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> I think a more correct solution might be to modify this TabChild code to
> send ContentReceivedTouch only if touchstart is prevented or the first
> touchmove has been dispatched:
> http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1910
> 
> This is what MetroInput does, using the mCancelable flag in this code:
> http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/
> MetroInput.cpp#1179
> 
> Keeping this logic on the "child" side will reduce the number of messages,
> because it will send only one ContentReceivedTouch per touch session
> (instead of one per touch event as it does now).

Would this mean introducing a state machine to TabChild so it knows what is the first touchmove of a touch session?
Attached patch bug950300 (obsolete) — — Splinter Review 
Assignee: botond → 21
Status: NEW → ASSIGNED

        Attachment #8349791 -
        Flags: review?(bugmail.mozilla)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug950300 (obsolete)
        — 
          — Splinter Review
      

    


  
Oups. I forgot a part. Sorry for the noise.

        Attachment #8349791 -
        Attachment is obsolete: true

        Attachment #8349791 -
        Flags: review?(bugmail.mozilla)

        Attachment #8349792 -
        Flags: review?(bugmail.mozilla)

    
    

    
  
Comment on attachment 8349792 [details] [diff] [review]
bug950300

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

I think we want to keep the bits in AsyncPanZoomController.* as well. Without that, if the content does a while(true) infinite loop in their touchstart or touchmove handler it will block scrolling from happening. We need to keep the timeout to guard against that and let the content scroll even if JS is nonresponsive.

        Attachment #8349792 -
        Flags: review?(bugmail.mozilla) → review-

    
    

    
  
Comment on attachment 8349792 [details] [diff] [review]
bug950300

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

Would also like matt to take a look at the bits in TabChild.cpp. I'm not convinced it deals with all cases (particularly involving multiple touch points) properly but he's the expert here.

        Attachment #8349792 -
        Flags: review?(mbrubeck)

    
    

    
  
Comment on attachment 8349792 [details] [diff] [review]
bug950300

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

The TabChild parts look good to me.  They are a little different than MetroInput (see below), but I'm not certain which is more correct.

gTouchListenerTimeout seems useful in theory, to keep panning from locking up if the content process is busy.  But I have no idea whether it is ever used in practice.  I'd rather not remove it in this bug if possible; if we decide to remove it we should do that separately.

I don't understand BrowserElementPanning enough to review that change.

::: dom/ipc/TabChild.cpp
@@ +1930,5 @@
> +
> +  switch (aEvent.message) {
> +  case NS_TOUCH_START: {
> +    bool isPrevented = nsIPresShell::gPreventMouseEvents ||
> +                       localEvent.mFlags.mMultipleActionsPrevented;

In MetroInput if we get an NS_TOUCH_START with multiple touches, we do the equivalent of "mWaitingTouchListeners = false;" and skip the isPrevented check.

I'm not actually sure whether the MetroInput behavior is correct for cases like preventDefault on the second touchstart of a touch session...

@@ +1942,5 @@
> +
> +  case NS_TOUCH_MOVE:
> +  case NS_TOUCH_END:
> +  case NS_TOUCH_CANCEL: {
> +    if (mWaitingTouchListeners) {

I think this code is needed only for NS_TOUCH_MOVE.  (At least, MetroInput runs its equivalent code for NS_TOUCH_MOVE only.)

        Attachment #8349792 -
        Flags: review?(mbrubeck) → feedback+

    
    

    
  
(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> Comment on attachment 8349792 [details] [diff] [review]
> bug950300
> 
> Review of attachment 8349792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The TabChild parts look good to me.  They are a little different than
> MetroInput (see below), but I'm not certain which is more correct.
> 
> gTouchListenerTimeout seems useful in theory, to keep panning from locking
> up if the content process is busy.  But I have no idea whether it is ever
> used in practice.  I'd rather not remove it in this bug if possible; if we
> decide to remove it we should do that separately.
> 
> I don't understand BrowserElementPanning enough to review that change.
> 

BrowserElementPanning.js is used for a few things like highlighting. In the pre-APZC world the touchmove was cancelled on a pan on a scrollable element which was fine since BEC was the last thing to catch it. Now with APZC if we keep cancelling the event, then we cancel scroll for each scrollable frame :)

> ::: dom/ipc/TabChild.cpp
> @@ +1930,5 @@
> > +
> > +  switch (aEvent.message) {
> > +  case NS_TOUCH_START: {
> > +    bool isPrevented = nsIPresShell::gPreventMouseEvents ||
> > +                       localEvent.mFlags.mMultipleActionsPrevented;
> 
> In MetroInput if we get an NS_TOUCH_START with multiple touches, we do the
> equivalent of "mWaitingTouchListeners = false;" and skip the isPrevented
> check.
> 
> I'm not actually sure whether the MetroInput behavior is correct for cases
> like preventDefault on the second touchstart of a touch session...
> 

Makes sense. I will take a look if I can reduce the amount of messages sent from the parent at the same time.

> @@ +1942,5 @@
> > +
> > +  case NS_TOUCH_MOVE:
> > +  case NS_TOUCH_END:
> > +  case NS_TOUCH_CANCEL: {
> > +    if (mWaitingTouchListeners) {
> 
> I think this code is needed only for NS_TOUCH_MOVE.  (At least, MetroInput
> runs its equivalent code for NS_TOUCH_MOVE only.)

If we don't do that then fast clicking does not work once we are in a WAITING_LISTENERS state in APZC since the events are never processed. So maybe Metro ends up in the timeout in such case?

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> Comment on attachment 8349792 [details] [diff] [review]
> bug950300
> 
> Review of attachment 8349792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we want to keep the bits in AsyncPanZoomController.* as well.
> Without that, if the content does a while(true) infinite loop in their
> touchstart or touchmove handler it will block scrolling from happening. We
> need to keep the timeout to guard against that and let the content scroll
> even if JS is nonresponsive.

Well that's a good theory but the timeout guard actually break the spec too. If the user hold the finger on the screen for 300ms before moving (which happens when you play a game where you may have to think a little bit before doing your action) then we will start a scroll on something where scrolling is not expected. I don't think that's what we want.

We probably need something smarter that is able to do the difference between a user lag and a JS lag, but even in that case that does not seems a good enough reason to violate the spec.

    
    

    
  
I will fix the multiple touch case (and hopefully reduce some of the message passing from the parent) and I will try to convince you on IRC about the timeout ;)

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> Well that's a good theory but the timeout guard actually break the spec too.

Yes, but all browsers have do something similar in order to avoid waiting indefinitely.

> If the user hold the finger on the screen for 300ms before moving (which
> happens when you play a game where you may have to think a little bit before
> doing your action) then we will start a scroll on something where scrolling
> is not expected. I don't think that's what we want.

Correct. We have this problem in Fennec as well. In practice we have found that this results in fewer problems than the alternative because often Gecko is very busy doing all sorts of things and we don't hear back about the preventDefault in time.

> We probably need something smarter that is able to do the difference between
> a user lag and a JS lag, but even in that case that does not seems a good
> enough reason to violate the spec.

I think we should be consistent across all of our platforms, which means keeping the timeout. That being said, the touch-action spec is designed to address precisely this problem, and we are getting very close to landing that, see bug 795567. Once that is in, content can use the touch-action CSS property to prevent scrolling on whichever elements they want and it doesn't require waiting for any JS to decide.

We can debate this on IRC if you want but even if we do I would like to split that into a separate bug because it is separate from this issue and we may want to back it out later if we do end up landing it.

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> > Well that's a good theory but the timeout guard actually break the spec too.
> 
> Yes, but all browsers have do something similar in order to avoid waiting
> indefinitely.

hmm. I wonder if others are doing tricks with when they fire the touchmove event. Like if they send the touchstart only when there is the a move, or a touchend/cancel. That would allow that is not lagging to always be able to prevent the click and will minimize the issue for case where it is really lagging.

Not sure if doing that can break anything though.
 
> We can debate this on IRC if you want but even if we do I would like to
> split that into a separate bug because it is separate from this issue and we
> may want to back it out later if we do end up landing it.

I agree that it is a separate issue, so let's talk about it after the break :)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug950300 (obsolete)
        — 
          — Splinter Review
      

    


  
Same patch without the removal of the timeout. I also added a line since I was breaking panning for web sites that does not have any touch handlers.

        Attachment #8349792 -
        Attachment is obsolete: true

        Attachment #8350112 -
        Flags: review?(bugmail.mozilla)

    
    

    
  
Comment on attachment 8350112 [details] [diff] [review]
bug950300

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

::: dom/ipc/TabChild.cpp
@@ +1931,5 @@
> +
> +  switch (aEvent.message) {
> +  case NS_TOUCH_START: {
> +    bool isTouchPrevented = nsIPresShell::gPreventMouseEvents ||
> +                            localEvent.mFlags.mMultipleActionsPrevented;

You can move the isTouchPrevented above the switch, that way you don't need to duplicate it in both sets of case statements.

        Attachment #8350112 -
        Flags: review?(bugmail.mozilla) → review+

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=883ba981158d

I also move the initialization of mWaitForListeners at the end. There are warning as error on try.
Target Milestone: --- → mozilla28

    
    

    
  
Setting flags so that the bug will be ready for uplift after it is merged to m-c.

          status-b2g-v1.2:
          --- → wontfix

          status-b2g-v1.3:
          --- → affected

          status-firefox27:
          --- → wontfix

          status-firefox28:
          --- → affected

          status-firefox29:
          --- → affected
Target Milestone: mozilla28 → ---

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4587af159a9e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

        Attachment #8350112 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8350539 [details] [diff] [review]
Patch for check-in

>+  nsCOMPtr<nsPIDOMWindow> outerWindow = do_GetInterface(mWebNav);
>+  nsCOMPtr<nsPIDOMWindow> innerWindow = outerWindow->GetCurrentInnerWindow();
>+
>+  if (!innerWindow || !innerWindow->HasTouchEventListeners()) {
>+    SendContentReceivedTouch(aGuid, false);
>+    return true;
>+  }
Why do we have this code?
What if some iframe has touch event listeners?
Flags: needinfo?(21)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8350539 [details] [diff] [review]
> Patch for check-in
> 
> >+  nsCOMPtr<nsPIDOMWindow> outerWindow = do_GetInterface(mWebNav);
> >+  nsCOMPtr<nsPIDOMWindow> innerWindow = outerWindow->GetCurrentInnerWindow();
> >+
> >+  if (!innerWindow || !innerWindow->HasTouchEventListeners()) {
> >+    SendContentReceivedTouch(aGuid, false);
> >+    return true;
> >+  }
> Why do we have this code?
> What if some iframe has touch event listeners?

Is the question related to the innerWindow check ? If so this code was here before my change, so I can't really tell.
Flags: needinfo?(21) → needinfo?(bugs)

    
    

    
  
That code wasn't there. There wasn't that early return.
Flags: needinfo?(21)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #29)
> That code wasn't there. There wasn't that early return.

16:17:02 - vingtetun: smaug: not sure what you want me to do for bug 950300 ?
16:17:41 - smaug: vingtetun: I just don't understand the code
16:18:49 - smaug: vingtetun: you added some code and explicitly don't run it if the top level window doesn't have touch event listeners
16:19:01 - vingtetun: smaug: IIRC this code path was to avoid to have the 300ms delay if there is no listener, and send back instantly a notification to the APZC gesture detector that it does not have to wait to start panning
16:19:54 - smaug: vingtetun: so the code doesn't deal with iframes
16:20:02 - vingtetun: i don't think so
16:20:18 - vingtetun: the previous version of the code was: http://hg.mozilla.org/mozilla-central/diff/1acc5e2112f2/dom/ipc/TabChild.cpp
16:20:50 - vingtetun: which is the one i based my patch on. i didn't thought at all about iframes tbh
Flags: needinfo?(21)

    
    

    
  
(I think the irc conversation was enough for the ni)
Flags: needinfo?(bugs)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: