1160408 - [Gallery] User can be prevented from replaying video in gallery if tapping screen as video playback ends

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Brogan Zumwalt [Inactive]]

Attachment: Logcat

Description:
If user plays a video in gallery app and taps the screen repeatedly as the video ends, the play button centered on screen after video playback completes becomes unresponsive and user must restart gallery app if they want to replay video.

Repro Steps:
1) Update a Flame to 20150430010201
2) Launch Gallery
3) Tap video thumbnail then play video
4) As the video nears end of playback rapidly tap on screen

Actual:
Center play button does not respond.

Expected:
User can press center play button to replay video.

Environmental Variables:
Device: Flame 3.0
Build ID: 20150430010201
Gaia: db8ea705c0fd1b1684807f5a8e837bb9a36a6f96
Gecko: 4b9b12c248dc
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Repro frequency: 1/3
See attached: Youtube video clip & logcat
Youtube video link: http://youtu.be/r1cheJ9DzqM

Comment 1

[Brogan Zumwalt [Inactive]]

Adding qawanted for Flame device branch checks

Comment 2

[Brogan Zumwalt [Inactive]]

Issue does NOT reproduce on Flame 2.2

User can press center play button to replay video.

Device: Flame 2.2
Build ID: 20150501002503
Gaia: 209bf4d6fcb16ea6834b8bd86976c012e5914fe6
Gecko: 79e7065ceefa
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

Nominating this 3.0? since this is broken functionality and a regression.

Comment 4

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

The changes for Bug 1068912 seem to have caused this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150405145759
Gaia: 723a80af3e8ce11bdea46685f3f68b934ed1aa0c
Gecko: 11fab330738e
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

First Broken 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150406080758
Gaia: 9d9d41e22d578105c9285809413dc5c2953d900d
Gecko: d22a8bcd2890
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 723a80af3e8ce11bdea46685f3f68b934ed1aa0c
Gecko: d22a8bcd2890

First Broken gaia / Last Working gecko - Issue DOES occur
Gaia: 9d9d41e22d578105c9285809413dc5c2953d900d
Gecko: 11fab330738e

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/723a80af3e8ce11bdea46685f3f68b934ed1aa0c...9d9d41e22d578105c9285809413dc5c2953d900d

Comment 5

[KTucker [:KTucker][Inactive 3/4/2016]]

Yura, can you take a look at this please? This might have been caused by the landing for bug 1068912.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

There seems to be an issue with gesture detector where tap event is not fired on the play button after the above STR (however click does). I'm still looking at why exactly tap is not triggered, but am wondering why click isn't used here. David would you know?

Comment 7

[David Flanagan [:djf]]

I can reproduce this. It sometimes reproduces with the controls shown and sometimes with them not shown.

If they are shown, then the back button works to go back to the list of thumbnails. But once I click on any of the thumbnails, gestures like taps, pinches and swipes just do not work. So somehow these STR are breaking the GestureDetector.

While trying to reproduce, I somehow managed to get this message in the logcat:

E/Gallery ( 7673): [JavaScript Error: "TypeError: handler is not a function" {file: "app://gallery.gaiamobile.org/js/frame_scripts.js" line: 94}]

That says to me that the gesture detector state machine has gotten itself into a very confused state.  But I can't figure out what I did to get that message in the logcat, so I don't know what is going on.

Comment 8

[David Flanagan [:djf]]

When I reproduce this bug, I can see that the gesture detector's FSM is stuck in afterTranformState. It thinks that two fingers touched the screen and only one was lifted. Once it is stuck here, when a new finger goes down, it thinks it has two touches and goes into transformState again, then when the finger goes it it goes back to afterTransformState.  This means that no single finger gesture events like tap or pan or swipe ever get emitted at this point.  (I'm not sure why transform events are not being emitted, though)

I suspect APZ involvement here, since it appears that we're getting more touchstart events than touchend events, but it is possible that this is just a gesture detector bug.

In either case bug 1068912 is probably not the culprit, though I suppose it could have made the issue easier to reproduce.

Kats: have there been any recentish changes to the way APZ handles touch events?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I think the last changes we made were bug 1140578 and bug 1146987. Both of those were uplifted to 2.2 so I doubt they caused this. Might be a pre-existing issue. If you log the event stream you're getting from gecko and it's not correct let me know and I can look into it.

Comment 10

[David Flanagan [:djf]]

Ah ha! I've got better STR for this bug that do not require rapid hammering on the screen.

As the video nears the end, touch the screen and hold it. This generates a touchstart event on the video element. Then, when the video stops playing, the video element is removed and the poster image is displayed in its place. Now when you lift your finger, there is no touchend event because the video element is no longer there.  This is what gets the gesture detector confused.  The reason this started manifesting when bug 1068912 landed is that we used to handle all events on an overlay on top of the video element, so having the video element change from shown to hidden never made a difference to event handling.

I've never understood what touchcancel events were for. Maybe this case of an element being hidden between touchstart and touchend is what they are for.  If so, I can use them to fix this.

I no longer think this is an APZ issue, but leaving the needinfo for Kats in case he knows anything about what is supposed to happen in this case where an element is hidden after touchstart and before touchend.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Ah that makes sense. I *think* in this case you should be getting either a touchend or touchcancel on the element that got the touchstart. If you don't get either then it's a bug in gecko that we should fix. I'm not 100% sure of this though, I'd have to check the gecko code that deals with this or ask smaug.

Comment 12

[David Flanagan [:djf]]

I'm not seeing a touchend or a touchcancel event in the gesture detector in this gallery app bug. But when I try to create a reduced test case where I hide an element on touchstart, I always seem to get a touchend event. 

So I think there is a gecko bug, but I can't isolate it.

I think I can probably create a gesture detector workaround for this in gaia, but it would be nice to fix it for real.

I'm going to go ahead and make this a 3.0 blocker since it is pretty easy to get into this state, and the only way to recover is to kill the app and restart.

Comment 13

[David Flanagan [:djf]]

So the issue with the touchend event not being sent if we hide the element after the touchstart happens for <video> and <audio control> elements, but not for <div> elements.  I suspect it has something to do with the xbl code that implements the touch controls for those elements. There is a test case demonstrating the bug at http://djf.net/tt.html.  This affects FirefoxOS and Firefox for Android, but does not affect Chrome on Android.

I've filed bug 1162771 as a followup to get a gecko fix for the issue, and will use this bug for a workaround in gesture_detector.js

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1160408 > mozilla-b2g:master

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8603072 [details] [review]
[gaia] davidflanagan:bug1160408 > mozilla-b2g:master

Punam,

This patch is to the gesture detector. Do you feel comfortable reviewing it?  The code is structured as a finite state machine, which may seem sort of weird or kind of cool, depending on how familiar you are with that sort of thing...

Let me know if you have questions or if you'd like me to find a different reviewer.

Comment 16

[Punam Dahiya [:pdahiya]]

Hi David
The patch looks good and resolves the issue in this bug for both video playing in gallery and camera - preview.
It will help if we can add comments to explain why we are switching to initialState at Line 339 instead of directly calling touchStartedState. Thanks!

Comment 17

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e6d21b2e3d9d366dddb3c0f7912639cd41665779

Comment 18

[Shally Li（leave from Mozilla）]

This bug has been verified as pass on latest Nightly build of Flame v3.0 and Nexus 5 v3.0 by the STR in Comment 0 and Comment 10.

Actual results: User can always press center 'Play' button to re-play the video.
See attachment: verified_v3.0.mp4
Reproduce rate: 0/8

-------------------------------------------------------------------
Device: Flame v3.0 build(Pass)
Build ID               20150526160204
Gaia Revision          8ca93673869a64e09ed6153c5402896822dfb253
Gaia Date              2015-05-26 19:31:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1e4e369822ac
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150526.195035
Firmware Date          Tue May 26 19:50:45 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 v3.0 build(Pass)
Build ID               20150526160204
Gaia Revision          8ca93673869a64e09ed6153c5402896822dfb253
Gaia Date              2015-05-26 19:31:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1e4e369822ac
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150526.195039
Firmware Date          Tue May 26 19:50:56 EDT 2015
Bootloader             HHZ12f

Comment 19

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.mp4