Closed Bug 981804 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: tnguyen, Assigned: pchang)

References

()

Details

(Keywords: regression, smoketest)

Attachments

(1 file)

46 bytes, text/x-github-pull-request
vingtetun
: review+
Details | Review
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.
Duplicate of this bug: 981810
Duplicate of this bug: 981810
Duplicate of this bug: 981810
Component: Gaia::Browser → Video/Audio
Product: Firefox OS → Core
blocking-b2g: --- → 1.4?
Keywords: qaurgent
QA Contact: 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
Sotaro - Does anything jump out to you in the push log above on what the cause of this bug is?
Flags: needinfo?(sotaro.ikeda.g)
CJ,

Please review and reassign appropriately. this is a smoke test issue and needs a fairly quick turnaround time.
Flags: needinfo?(cku)
Duplicate of this bug: 982164
Peter, please check this issue.
Flags: needinfo?(cku) → needinfo?(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.
Flags: needinfo?(pchang)
It is easy to see video control after touching status bar...
Flags: needinfo?(sotaro.ikeda.g)
The push log looks incorrect here - there's a large amount of changesets here. Reflagging regressionwindow-wanted.
I just found this issue was caused by the following commit.

changeset:   172525:fd02f1217e4c
user:        Benoit Jacob <bjacob@mozilla.com>
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
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.
Flags: needinfo?(bjacob)
backout commit
https://hg.mozilla.org/mozilla-central/rev/44ae8462d6ab
Flags: needinfo?(bjacob)
Assignee: nobody → pchang
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.
Flags: needinfo?(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.
Flags: needinfo?(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.
(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?
Flags: needinfo?(21)
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.
Flags: needinfo?(pchang)
Attached file Gaia PR
Attachment #8389810 - Flags: review?(21)
Flags: needinfo?(21)
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.
Attachment #8389810 - Flags: review?(21) → review+
(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.
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.
Blocks: 977757
(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.
Flags: needinfo?(pchang)
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → 1.4+
> 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?
(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.
(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.
(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 :)
Target Milestone: --- → mozilla30
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
Status: RESOLVED → VERIFIED
Depends on: 982839
You need to log in before you can comment on or make changes to this bug.