Closed Bug 1105035 Opened 10 years ago Closed 9 years ago

[Window Management][YouTube] User is able to access the hidden Utility Tray behind Full Screen youtube video.

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master fixed)

RESOLVED FIXED
2.2 S5 (6feb)
blocking-b2g 2.2+
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified
b2g-master --- fixed

People

(Reporter: Marty, Assigned: tedders1)

References

()

Details

(Keywords: regression, Whiteboard: [2.2-Daily-Testing][systemsfe] Part 2 should only land on the b2g37_2.2 branch. Part 1 can land on master. )

Attachments

(3 files, 2 obsolete files)

Description:
If the user is in a full screen YouTube video, theya re able to use the pull-down gesture to bring down a non-visible Utility Tray.  This disrupts or prevents the user's ability to use the full screen video controls.  The user is also able to intermittently utilize Utility Tray functionality, such as enabling/disabling WiFi, Airplane mode, Data, as well as launching the Settings app.

Notes:
-This occurs both in the Browser app and in the YouTube Marketplace App.
-It seems to be easier to reproduce if the user performs the pull-down gesture twice in a row, back to back.
-The linked video demonstrates pulling down the non-visible Utility Tray, and tapping the lower left corner of the screen to launch the Settings app from the tray.

Repro Steps:
1) Update a Flame device to BuildID: 20141125040209
2) Enable and connect to a WiFi or Data network.
3) Launch the Browser app and navigate to YouTube.
4) Navigate to a video and begin streaming it in Full Screen
5) While in full screen, perform the Utility Tray pull-down gesture, and try to access Full Screen controls or use the hidden Utility Tray.
  
Actual:
A hidden Utility Tray is accessible, disrupting full screen video controls.
  
Expected: 
Utility Tray is not accessible in a Full Screen YouTube video.
  
Environmental Variables:
Device: Flame 2.2 Master (319MB)
BuildID: 20141125040209 (Full Flash)
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
  
Repro frequency: 4/5 
See attached: video clip (URL), logcat

--------------------------------------

This issue does NOT occur on Flame 2.1.
The Utility Tray is not accessible in a Full Screen YouTube video.

Environmental Variables:
Device: Flame 2.1 (319MB)
BuildID: 20141125001201 (Full Flash)
Gaia: 1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko: db893274d9a6
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Functional regression of a core feature.

Requesting a window.
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
QA Contact: ckreinbring
Able to repro on the below build, but any earlier and the repro rate becomes too low to search for the regression window.
Made 15 attempts per build, reproed on the fifth attempt on the below build.

BuildID: 20140830195315
Gaia: 2be78d83a760fa3b9638fe51c266b442d14597f1
Gecko: 1db35d2c9a2f
Platform Version: 34.0a1
Firmware Veresion: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Reproduction rate for this issue is too low to get a window on.  As a rule, we do not run regression windows on issues with less than a 50% chance of occurring consistently and builds earlier than the above were showing less than 10%.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
QA Contact: ckreinbring
blocking-b2g: 2.2? → 2.2+
Whiteboard: [2.2-Daily-Testing] → [2.2-Daily-Testing][systemsfe]
Assignee: nobody → tclancy
Status: NEW → ASSIGNED
Target Milestone: --- → 2.2 S3 (9jan)
Adding qawanted to see if it's still reproducible on today's 2.2.
Keywords: qawanted
This is possibly the same issue as bug 1113050.
See Also: → 1113050
QA Contact: ychung
This issue still reproduces on today's nightly and the latest engineer build.

Result: The hidden utility tray is accessible from the fullscreen mode on YouTube.
Repro rate: 3/10

[Nightly Build]  
Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150105010205
Gaia: c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko: 636498d041b5
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
[Engineer Build]  
Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20150105033341
Gaia: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Gecko: 912036eeb024
Version: 37.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
So, apparently when an app is in full-screen mode (such as a full-screen YouTube video), we shouldn't allow the user to swipe down from the top to access the notifications tray.

Should the user still be able to swipe sideways to switch between apps when the current app is in full-screen mode? Could someone in UX give me a clarification on that?

For the record, I'm hoping that you'll say "no, the user shouldn't be able to swipe between apps in full-screen mode".
Flags: needinfo?(firefoxos-ux-bugzilla)
Flagging Francis here, and ni? Jacqueline as back-up while Rob and I are out (I am on PTO in Mexico so did not see this until now) and Francis is traveling.
Flags: needinfo?(jsavory)
Flags: needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(fdjabri)
Attached file Bug 1105035 fix. (obsolete) —
Attachment #8551007 - Flags: review?(kgrandon)
It's taking a while to hear from UX, so I'm going to tentatively proceed with the patch I've got.

Green TBPL run here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=4e20f061f60e
Comment on attachment 8551007 [details] [review]
Bug 1105035 fix.

This does seem fine to be, but I want to get a second pair of eyes on this to make sure it's ok. Etienne - could you take a look?

I suppose this could also block the software home gesture in full screen mode? Probably not the end of the world.
Attachment #8551007 - Flags: review?(kgrandon) → review?(etienne)
Comment on attachment 8551007 [details] [review]
Bug 1105035 fix.

(In reply to Ted Clancy [:tedders1] from comment #7)
> So, apparently when an app is in full-screen mode (such as a full-screen
> YouTube video), we shouldn't allow the user to swipe down from the top to
> access the notifications tray.

I don't think that's true (contrary to what the bug says).

We always had the support to swipe once from the top to make the statusbar appear and a second time to drag the utility tray on top of a fullscreen app.
It's actually still working for fullscreen apps (like the videos app) but not for apps dynamically requesting fullscreen like youtube.

I don't see why the behavior should be different.

I suggest fixing this "dynamic fullscreen mode" case and adding a test to [1] to cover it.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/fullscreen_statusbar_test.js
Attachment #8551007 - Flags: review?(etienne) → review-
So, there's a big difference between how fullscreen apps work and how "dynamic fullscreen" works.

Fullscreen apps (apps which have "fullscreen: true" or "fullscreen-layout: true" in their manifest) are positioned by gaia using CSS. In this case, the system app applies the .fullscreen-app class or .fullscreen-layout-app class to the #screen element.

"Dynamic fullscreen" is when an app calls element.requestFullscreen() or element.mozRequestFullScreen(), and gecko positions the element above all others. In this case, the pseudo-class :-moz-full-screen matches the element, and :-moz-full-screen-ancestor matches ancestors of the element. 

Now, despite several lines in app/system/style/zindex.css which suggest otherwise, I don't think it's possible for gaia to position a sibling element on top of a full-screen element when requestFullscreen (or mozRequestFullScreen) has been called on that element. So, we can't show the statusbar or utility tray on top of a fullscreen YouTube video.

I'm currently looking for someone to confirm/correct this understanding of mine. If I *am* correct, I think it might be best to say that the user shan't switch apps during a full-screen YouTube video, and use the patch I already have. Otherwise, some big gecko work might be involved.
Actually, I'll probably want to revise my patch, now that I have a better understanding of what's going on.
(In reply to Etienne Segonzac (:etienne) from comment #12)
> Comment on attachment 8551007 [details] [review]
> Bug 1105035 fix.
> 
> (In reply to Ted Clancy [:tedders1] from comment #7)
> > So, apparently when an app is in full-screen mode (such as a full-screen
> > YouTube video), we shouldn't allow the user to swipe down from the top to
> > access the notifications tray.
> 
> I don't think that's true (contrary to what the bug says).
> 
> We always had the support to swipe once from the top to make the statusbar
> appear and a second time to drag the utility tray on top of a fullscreen app.
> It's actually still working for fullscreen apps (like the videos app) but
> not for apps dynamically requesting fullscreen like youtube.
> 
> I don't see why the behavior should be different.
> 
Sorry for the late reply, I was on PTO last week. I agree with Etienne, there's no reason why there should be any difference from a UX perspective. 

Also, app-to-app sheet gestures should also be accessible in full screen mode.
Flags: needinfo?(jsavory)
Flags: needinfo?(fdjabri)
So, I think we have a bit of a problem here. According to what I've been told by Chris Pearce (:cpearce), it shouldn't be possible to place any elements on top of an element that's been made full-screen with mozRequestFullScreen (a.k.a. requestFullscreen). So we can't have the status bar and utility tray on top of a full-screen YouTube video, unless we make changes to Gecko.

And is changing Gecko something we want to do?

"But this has worked before", I hear you say.

Well, at the moment, Gecko implements full-screen elements using CSS. There are rules in layout/style/ua.css that place full-screen elements at z-index: 2147483647, and rules in layout/style/full-screen-override.css that prevent any ancestors of the full-screen element from creating stacking contexts. This means the full-screen element appears in the root stacking context, at the highest possible z-index.

Our status bar and utility tray is also positioned at z-index: 2147483647, which means it's competing with the full-screen YouTube video to be on top. The result is non-deterministic, so sometimes it works, and sometimes it doesn't work.

Furthermore, Xidorn Quan (:xidorn) is currently working on a new implementation of mozRequestFullScreen which won't use CSS, and which will be closer to the standard. Once that work is done, we definitely won't be able to put the status bar or utility tray over a full-screen YouTube video, unless we make changes to Gecko.

I believe we have two options:

1) Change the behaviour so that the user can't see notifications, access the utility tray, or swipe to a different app while watching a full-screen YouTube video. (The user will have to exit full-screen before doing anything. This also means they won't be able to see the blue stripe at the top of the screen when they receive an SMS message while watching a full-screen YouTube video.)

or

2) Change Gecko. There will need to be some discussions about architecture, and this might end up being a big change.

I understand that UX would prefer option 2, but considering this is an end-of-life product, I'm not sure we should be making changes to Gecko to support it.

I'm not sure which direction to go in, so I'm going to needinfo Wilfred Mathanaraj, the product manager for SystemsFE.
Flags: needinfo?(wmathanaraj)
I would agree that gecko changes are not the solution here... what is the best UX work we can do with the current gecko.
Flags: needinfo?(wmathanaraj)
Attached file Bug-1105035-fix-part-1
Attachment #8551007 - Attachment is obsolete: true
Attachment #8558849 - Flags: review?(kgrandon)
Attached patch bug-1105035-fix-part-2 (obsolete) — Splinter Review
Right now, full-screen elements (such as full-screen YouTube videos) are positioned at z-index INT_MAX. This patch lowers them to INT_MAX - 1, so that we can reliably put gaia UI elements on top of the YouTube video.

This might seem like a hack (and it is), but :xidorn is currently re-writing how full-screen elements are rendered, so this will be unnecessary soon. But we need this for the b2g37_2.2 branch, which won't get xidorn's work.

I won't ask to land this on master. Only on b2g37_2.2.
Attachment #8558852 - Flags: review?(bzbarsky)
Comment on attachment 8558852 [details] [diff] [review]
bug-1105035-fix-part-2

Going to punt this one to roc, sorry.
Attachment #8558852 - Flags: review?(bzbarsky) → review?(roc)
Comment on attachment 8558849 [details] [review]
Bug-1105035-fix-part-1

I think this makes sense to me. Z-index is so fickle though, I think it's worth getting a second pair of eyes to at least take a quick look as well. Alive - if you have a quick minute I'd appreciate it if you could take a look. Thanks!
Attachment #8558849 - Flags: review?(kgrandon)
Attachment #8558849 - Flags: review+
Attachment #8558849 - Flags: feedback?(alive)
Comment on attachment 8558852 [details] [diff] [review]
bug-1105035-fix-part-2

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

::: layout/style/ua.css
@@ +262,5 @@
>    top: 0 !important;
>    left: 0 !important;
>    right: 0 !important;
>    bottom: 0 !important;
> +  z-index: 2147483646 !important;

It might be worth adding a comment why this is not at the highest level (like comment 19 in this bug).
Comment on attachment 8558849 [details] [review]
Bug-1105035-fix-part-1

LGTM
Attachment #8558849 - Flags: feedback?(alive) → feedback+
Adding a comment, as suggested by Kevin.
Attachment #8558852 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [2.2-Daily-Testing][systemsfe] → [2.2-Daily-Testing][systemsfe] Part 2 should only land on the b2g37_2.2 branch. Part 1 can land on master.
Master: https://github.com/mozilla-b2g/gaia/commit/6a54af26e4a42f6b07cad04000f47deaa0f2c50b

Please request v2.2/b2g37 uplift approval on these patches when you get a chance.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: needinfo?(tclancy)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.2 S3 (9jan) → 2.2 S5 (6feb)
Comment on attachment 8558849 [details] [review]
Bug-1105035-fix-part-1

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
I think it was introduced by Bug 1055977.

[User impact] if declined:
A user can't access the utility tray while watching a full-screen YouTube video (or other full-screen video content). Furthermore, if they try, confusing behaviour might result.

[Testing completed]:
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=d48bfd4629ed

[Risk to taking this patch] (and alternatives if risky):
None foreseen.

[String changes made]:
None
Attachment #8558849 - Flags: approval-gaia-v2.2?(fabrice)
Attachment #8558849 - Flags: approval-gaia-v2.2?(fabrice) → approval-gaia-v2.2+
Flags: needinfo?(tclancy)
This is NOT fixed on the latest Nightly 3.0 Flame build.

Actual Results: The Utility Tray can only occasionally be brought down and the user can only occasionally swipe to other apps while in a full screen mode on youtube.

This issue is fixed in the latest Nightly 2.0 Flame build.

Actual Results: The user can pull down the tray or edge swipe easily.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150325083046
Gaia: aebfbd998041e960cea0468533c0b5041b504850
Gecko: db0409de517a
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150325002503
Gaia: aeee2a54caa8ffb875b96264b61d742b70689f22
Gecko: 556aca3e50ac
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?][failed-verification]
Flags: needinfo?(ktucker)
Correction to the above comment 28: I verified that it was fixed on 2.2, not 2.0 flame.
Jayme, let's write up a new issue for 3.0 since this landed back in early Feb.
Flags: needinfo?(ktucker) → needinfo?(jmercado)
QA Whiteboard: [QAnalyst-Triage?][failed-verification] → [QAnalyst-Triage-][failed-verification]
I wrote up bug 1147688 per comment 30.  The title and intent of the bug has been altered to better fit the expected design as expressed in comment 12 and onward.
QA Whiteboard: [QAnalyst-Triage-][failed-verification] → [QAnalyst-Triage?][failed-verification]
Flags: needinfo?(jmercado) → needinfo?(ktucker)
No longer blocks: 1147688
Depends on: 1147688
QA Whiteboard: [QAnalyst-Triage?][failed-verification] → [QAnalyst-Triage+][failed-verification]
Flags: needinfo?(ktucker)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: