981804 - [B2G][Browser][YouTube] Unable to navigate out of watching video in fullscreen mode

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Tony Nguyen]

Description:
Video toolbar menu doesn't come up when tapping on the video while in fullscreen mode.  

Repro Steps:
1) Updated Buri to BuildID: 20140310080111
2) Navigate into Browser App
3) Navigate to www.youtube.com
4) Tap on any video to start watching
5) Tap on the fullscreen icon
6) Tap on the video to bring up video toolbar

Actual Result:
Video control options aren't displayed when user taps on video and is unable to exit fullscreen mode

Expected Result:
Video control options are displayed as soon as the user taps on video and is able to exit fullscreen mode

Environmental Variables:
Device: Buri Master M-C mozRIL
BuildID: 20140310080111
Gaia: 8c9191df3c107df4073f3ca63816a1d36c51af5d
Gecko: 923f1411f42f
Version: 30.0a1
v1.2-device.cfg

Attached: YouTube link

Note: Tapping or pulling down on the status bar will display a toolbar that cannot be used. None of the buttons will work, and the user is unable to move the slider. The only way to exit fullscreen mode is by pressing the home button. This issue happens on both landscape and portrait mode.

Comment 4

[Peter Bylenga [:PBylenga]]

Determined issue was a gecko issue from the Gaia/Gecko swap test.

Last working Inbound build
Environmental Variables:
Device: Buri v1.4 Mozilla RIL
BuildID: 20140307124427
Gaia: b3758a90b8888e9d95128846b2833b4d9444ef7f
Gecko: f952ac387d75
Version: 30.0a1
Firmware Version: v1.2_device.cfg

First broken Inbound build
Environmental Variables:
Device: Buri v1.4 Mozilla RIL
BuildID: 20140307124627
Gaia: 6c109df47006b08c082761c0ddd6ba53a864983a
Gecko: a3bf4be14ce0
Version: 30.0a1
Firmware Version: v1.2_device.cfg


Last Working Gaia/First Broken Gecko: Issue DOES Reproduce
 Gaia: b3758a90b8888e9d95128846b2833b4d9444ef7f
 Gecko: a3bf4be14ce0
 
First Broken Gaia/Last Working Gecko: Issue Does NOT Reproduce
 Gaia: 6c109df47006b08c082761c0ddd6ba53a864983a
 Gecko: f952ac387d75
 

Push Log: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f952ac387d75&tochange=a3bf4be14ce0

Comment 5

[Jason Smith [:jsmith]]

Sotaro - Does anything jump out to you in the push log above on what the cause of this bug is?

Comment 6

[Preeti Raghunath(:Preeti)]

CJ,

Please review and reassign appropriately. this is a smoke test issue and needs a fairly quick turnaround time.

Comment 8

[u459114]

Peter, please check this issue.

Comment 9

[Peter Chang[:pchang]]

I could reproduce this issue based on latest gecko.

The video control show up under fullscreen mode if I keep touching somewhere on the screen randomly. But there is no response when I click the control buttons.

It looks like the touch focus problem for browser under fullscreen mode.

Comment 10

[Peter Chang[:pchang]]

It is easy to see video control after touching status bar...

Comment 11

[Jason Smith [:jsmith]]

The push log looks incorrect here - there's a large amount of changesets here. Reflagging regressionwindow-wanted.

Comment 12

[Peter Chang[:pchang]]

I just found this issue was caused by the following commit.

changeset:   172525:fd02f1217e4c
user:        Benoit Jacob <[e-mail]>
date:        Fri Mar 07 12:14:25 2014 -0500
files:       layout/style/nsCSSParser.cpp layout/style/nsCSSPropList.h layout/style/nsCSSProps.cpp layout/style/nsCSSProps.h
description:
Bug 977757 - 2/3 - add nsCSSProps::eEnabledInChromeOrCertifiedApp bit, and use it for will-change - r=dbaron,bz

Comment 13

[Peter Chang[:pchang]]

loop bjacob, with above commit, I will see the status bar show up during youtube fullscreen mode.
For the normal condition of youtube fullscreen, we shouldn't see the status bar.

Comment 14

[Peter Chang[:pchang]]

There are three dependency patches need to backout to prevent build break.

https://hg.mozilla.org/mozilla-central/rev/75217485ce5a
https://hg.mozilla.org/mozilla-central/rev/fd02f1217e4c
https://hg.mozilla.org/mozilla-central/rev/45253e02bde3

Comment 15

[Peter Chang[:pchang]]

backout commit
https://hg.mozilla.org/mozilla-central/rev/44ae8462d6ab

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you're going to back a patch out, you need to reopen the bug that you backed out.

But beyond that, I think backing out is probably the wrong path forward.  Bug 977757 made some CSS in gaia that was previously not supported become supported.  I think the thing to figure out is which use of 'will-change' in gaia is responsible for the problem and back *that* out until it can be fixed, rather than backing out 'will-change' support entirely -- at least unless there's evidence that will-change is actualy broken.

Comment 17

[Peter Chang[:pchang]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #16)
> If you're going to back a patch out, you need to reopen the bug that you
> backed out.
> 
> But beyond that, I think backing out is probably the wrong path forward. 
> Bug 977757 made some CSS in gaia that was previously not supported become
> supported.  I think the thing to figure out is which use of 'will-change' in
> gaia is responsible for the problem and back *that* out until it can be
> fixed, rather than backing out 'will-change' support entirely -- at least
> unless there's evidence that will-change is actualy broken.

Comment 18

[Peter Chang[:pchang]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #16)
> If you're going to back a patch out, you need to reopen the bug that you
> backed out.
> 
> But beyond that, I think backing out is probably the wrong path forward. 
> Bug 977757 made some CSS in gaia that was previously not supported become
> supported.  I think the thing to figure out is which use of 'will-change' in
> gaia is responsible for the problem and back *that* out until it can be
> fixed, rather than backing out 'will-change' support entirely -- at least
> unless there's evidence that will-change is actualy broken.

I will try to loop the gaia guys to this bug to double check this problem.
So you suggest I just re-land those patches.

Comment 19

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #18)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #16)
> > If you're going to back a patch out, you need to reopen the bug that you
> > backed out.
> > 
> > But beyond that, I think backing out is probably the wrong path forward. 
> > Bug 977757 made some CSS in gaia that was previously not supported become
> > supported.  I think the thing to figure out is which use of 'will-change' in
> > gaia is responsible for the problem and back *that* out until it can be
> > fixed, rather than backing out 'will-change' support entirely -- at least
> > unless there's evidence that will-change is actualy broken.
> 
> I will try to loop the gaia guys to this bug to double check this problem.
> So you suggest I just re-land those patches.

vivien, are you the right person to check will-change modification in gaia side?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I'd like to understand why we're backing things out 40 minutes after identifying the changeset that caused the problem, exactly.  Unless the issue is a development blocker, I'd think we would give the patch author a chance to look at things before backing out.  That's how it normally works in Gecko, at least.

Comment 21

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8389810 [details] [review]
Gaia PR

Sounds like a reasonable workaround. I suggest that you open a followup to identify why will-change on a fullscreen'ed element has some issues.

Comment 23

[Etienne Segonzac (:etienne)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #22)
> Comment on attachment 8389810 [details] [review]
> Gaia PR
> 
> Sounds like a reasonable workaround. I suggest that you open a followup to
> identify why will-change on a fullscreen'ed element has some issues.

Filed bug 982665.

To be clear we're not loosing anything with this workaround since we don't need the will-change in this specific case.
Just waiting for a green travis before merging.

Comment 24

[Etienne Segonzac (:etienne)]

The fix landed in gaia:
https://github.com/mozilla-b2g/gaia/commit/c22466e71c6da1d472ce28733a001fae37bcdc5e

Keeping this bug open until we figure out the earlier backout issue.

Comment 25

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> I'd like to understand why we're backing things out 40 minutes after
> identifying the changeset that caused the problem, exactly.  Unless the
> issue is a development blocker, I'd think we would give the patch author a
> chance to look at things before backing out.  That's how it normally works
> in Gecko, at least.

It's a smoketest blocker & we're right before the QC quality deadline. We're going to get nailed to the wall if this stays in even for a small period of time by our partners.

Comment 26

[Jason Smith [:jsmith]]

Anyways, the regressing patch was backed out & a workaround has landed in Gaia, so I think we should be good here now. Feel free to reland bug 977757.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> It's a smoketest blocker & we're right before the QC quality deadline.

OK, that's a good reason.  

Can we please try to automate as many of these smoketests as we can, so we can actually catch these sort of failures on try?

Comment 28

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky [:bz] from comment #27)
> > It's a smoketest blocker & we're right before the QC quality deadline.
> 
> OK, that's a good reason.  
> 
> Can we please try to automate as many of these smoketests as we can, so we
> can actually catch these sort of failures on try?

Yup - we've been doing that. Seems this one wasn't caught - I'll file a bug to get a test for this.

Comment 29

[Peter Chang[:pchang]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> I'd like to understand why we're backing things out 40 minutes after
> identifying the changeset that caused the problem, exactly.  Unless the
> issue is a development blocker, I'd think we would give the patch author a
> chance to look at things before backing out.  That's how it normally works
> in Gecko, at least.

The reason to backout because the authour is on vacation and we are under different timezone.
Next time I will try to loop more related ppl before backout.

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to peter chang[:pchang][:peter] from comment #29)
> (In reply to Boris Zbarsky [:bz] from comment #20)
> > I'd like to understand why we're backing things out 40 minutes after
> > identifying the changeset that caused the problem, exactly.  Unless the
> > issue is a development blocker, I'd think we would give the patch author a
> > chance to look at things before backing out.  That's how it normally works
> > in Gecko, at least.
> 
> The reason to backout because the authour is on vacation and we are under
> different timezone.
> Next time I will try to loop more related ppl before backout.

Jason, Peter,
the main reason to have landed will-change with a flag that makes it only works for certified app is to be able to use it where it works, and to find bugs before exposing it to web content.

So for any bugs related to will-change we can usually just remove the rule in Gaia that expose the will-change issue. So for this case, it is always better to remove the will-change css rule in Gaia instead of removing the platform feature :)

Comment 31

[Tony Nguyen]

This issue is fixed now and no longer reproduces on the latest Buri Master M-C build. 

Environmental Variables:
Device: Buri Master M-C mozRIL
BuildID: 20140313131233
Gaia: ea9e23abea5933656555d849b922c8da7530c90b
Gecko: fe40387eba1a
Version: 30.0a1
v1.2-device.cfg