1055198 - [Video] Hide soft home key during full screen video playback

Status: RESOLVED WONTFIX

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

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

Once bug 1037255 and bug 1054126 land, apps with `fullscreen_layout` in their manifest can show and hide the software home button using the fullscreen API. At that point, we need to update our Video app to leverage this functionality to follow the spec:

https://bug1036339.bugzilla.mozilla.org/attachment.cgi?id=8473080, page 7

Comment 1

[Gregor Wagner [:gwagner]]

Hema, can someone from the media team pick this up?

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

[Blocking Requested - why for this release]: I assume this is 2.1+ -- can someone flag it as such so the gueries pick it up?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

For more info on the UX, see https://bugzilla.mozilla.org/show_bug.cgi?id=1037255#c5

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Over to russn. (Thanks!)

Comment 5

[Russ Nicoletti [:russn]]

Michael, my understanding of the changes to the video app for this feature are:

* add 'fullscreen_layout: true' to video app manifest
* call mozRequestFullScreen() when video is being shown (when video app is in fullscreen mode)
* call mozCancelFullScreen() when video app is exiting fullscreen

Can you confirm? Thanks.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Russ Nicoletti [:russn] from comment #5)
> Michael, my understanding of the changes to the video app for this feature
> are:
> 
> * add 'fullscreen_layout: true' to video app manifest
> * call mozRequestFullScreen() when video is being shown (when video app is
> in fullscreen mode)
> * call mozCancelFullScreen() when video app is exiting fullscreen
> 
> Can you confirm? Thanks.

Yes, at the least we will need to do the above things. The other things to think about are, hiding/showing the video controls to be in sync with the soft home button. And making sure the soft home button does not overlay any video controls (or controls while in the video selection list).

Comment 7

[Russ Nicoletti [:russn]]

My observations thus far from my POC:

When applying the 1037255 and 1054126 patches and enabling the soft home button, I found the soft home button covered the video footer. Caveat: I applied the patches manually as they failed to apply automatically -- will attach screenshot.

I also found mozRequestFullScreen did not behave as expected: the video gets split in two -- will attach screenshot.

This is the PR for my POC: https://github.com/mozilla-b2g/gaia/pull/23340.

ni? for mhenretty and gduan to comment on the behavior I'm seeing and on my patch. Thanks.

Comment 8

[Russ Nicoletti [:russn]]

Attachment: Screenshot of soft home covering video footer

Comment 9

[Russ Nicoletti [:russn]]

Attachment: Screenshot of video player after mozRequestFullScreen

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

I think bug 1037255 will do hide software home-key when calling requestFullScreen in video app.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Russ Nicoletti [:russn] from comment #7)
> When applying the 1037255 and 1054126 patches and enabling the soft home
> button, I found the soft home button covered the video footer.

Yeah, this is something you will have to account for in the video app. It is what I meant in comment 6 when I said "And making sure the soft home button does not overlay any video controls (or controls while in the video selection list)." So, when in other parts of the video app, the video app needs to be aware of the soft-home button and adjust its display accordingly (ie, move the footer up, etc).
 
> I also found mozRequestFullScreen did not behave as expected: the video gets
> split in two -- will attach screenshot.

This is very strange, mozRequestFullScreen should not change the video display at all, merely hide the home button. Perhaps this is a gecko issue. ni? Etienne and Markus to see if they have any ideas.

Comment 12

[Russ Nicoletti [:russn]]

Why does the video app need to account for the software home button? Do other apps need to account for it? It seems the software home button area is essentially part of the system and not owned by the app and therefore the app shouldn't be aware of it and shouldn't be able to use that part of the screen. It seems inefficient for the app to have to be aware of the software home button and to manage that area. I thought that's what the purpose of mozRequestFullScreen/mozCancelFullScreen was, for the app to temporarily reclaim the software home button area and then to be able to give it back to the system.

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Michael Henretty [:mhenretty] from comment #11)
> (In reply to Russ Nicoletti [:russn] from comment #7)
> > When applying the 1037255 and 1054126 patches and enabling the soft home
> > button, I found the soft home button covered the video footer.
> 
> Yeah, this is something you will have to account for in the video app. It is
> what I meant in comment 6 when I said "And making sure the soft home button
> does not overlay any video controls (or controls while in the video
> selection list)." So, when in other parts of the video app, the video app
> needs to be aware of the soft-home button and adjust its display accordingly
> (ie, move the footer up, etc).
>  
> > I also found mozRequestFullScreen did not behave as expected: the video gets
> > split in two -- will attach screenshot.
> 
> This is very strange, mozRequestFullScreen should not change the video
> display at all, merely hide the home button. Perhaps this is a gecko issue.
> ni? Etienne and Markus to see if they have any ideas.

Definitely looks like a gecko bug. I think I saw this once but it depends on the device.

Comment 14

[Etienne Segonzac (:etienne)]

(In reply to Russ Nicoletti [:russn] from comment #12)
> Why does the video app need to account for the software home button? Do
> other apps need to account for it? It seems the software home button area is
> essentially part of the system and not owned by the app and therefore the
> app shouldn't be aware of it and shouldn't be able to use that part of the
> screen. It seems inefficient for the app to have to be aware of the software
> home button and to manage that area. I thought that's what the purpose of
> mozRequestFullScreen/mozCancelFullScreen was, for the app to temporarily
> reclaim the software home button area and then to be able to give it back to
> the system.

This still works. But the transition is ugly and will stay ugly because resizing the frame when you reclaim/give back the software home area is very costly.

This why we're introducing "fullscreen_layout", it's way for the app to opt-in a mode where the frame stays the same size when you request/cancel fullscreen, only the software home is changing. Enabling us to do nice transitions.

It's not ideal and a better spec is in the work but this is what we have for 2.1 and maybe 2.2.

Comment 15

[Russ Nicoletti [:russn]]

I've updated my PR to handle swb events and to adjust some video elements accordingly. However, the app is not getting the events. I've verified https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/software_button_manager.js#L167 is firing when enabling the swb but my logging in video.js (handleSWHBEnabled) is not appearing. Is there something I'm missing about these events?

Comment 16

[Russ Nicoletti [:russn]]

Vivien, I've added a media query to video.css and updated the PR. It's not having an effect (I'm testing with a device that has a hardware button -- I've tried 'setprop moz.has_home_button 1', still no effect). Can you check this query?

Comment 17

[Russ Nicoletti [:russn]]

Attachment: POC PR: https://github.com/mozilla-b2g/gaia/pull/23340

Comment 18

[Russ Nicoletti [:russn]]

One other issue related to video app knowing if swb is present: It seems the js needs to know if swb is present (or if there is no hardare button) so that it knows when to call mozRequestFullScreen. Or, is it not harmful to call mozRequestFullScreen regardless of whether swb is present? For me it is harmful (https://bugzilla.mozilla.org/attachment.cgi?id=8479551) but perhaps that is an anomaly.

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Russ Nicoletti [:russn] from comment #18)
> One other issue related to video app knowing if swb is present: It seems the
> js needs to know if swb is present (or if there is no hardare button) so
> that it knows when to call mozRequestFullScreen. Or, is it not harmful to
> call mozRequestFullScreen regardless of whether swb is present? For me it is
> harmful (https://bugzilla.mozilla.org/attachment.cgi?id=8479551) but perhaps
> that is an anomaly.

I think this is a bug, and I don't see harm in the fullscreen_layout app calling mozRequestFullscreen whether or not the softhomebutton is available. By the way though, you can determine if the property is set using javascript. I believe the settings app is doing that here:

https://github.com/mozilla-b2g/gaia/blob/8c0be26ed3668d9061e75af1c80087c681cc5e13/apps/settings/js/developer.js#L11

Comment 20

[Russ Nicoletti [:russn]]

The media query to detect when there is no hardware home button is working (when using 'setprop ro.moz.has_home_button 0'). With that the following elements are not covered by the software button:

* thumbnail list view
* the thumbnail select view
* video player

Open issues:

* Elements still covered by swb:
  - Options menu
  - Confirm dialog
  - Info view

* Elements are moved up (above a non-existent swb) in landscape view. Ideally, the media query would take care of this but up till now I've not been successful with that.

Comment 21

[Russ Nicoletti [:russn]]

The assumption now is we don't need to listen for software-button events, that media queries will handle everything. Removing ni? for Etienne.

Comment 22

[Russ Nicoletti [:russn]]

I've updated the patch to handle thumbnail list view, thumbnail select view, video player view when in landscape orientation. So, AFAIK, the open issues are:

* Elements still covered by swb:
  - Options menu
  - Confirm dialog
  - Info view

Comment 23

[Russ Nicoletti [:russn]]

Patch updated -- confirm dialog is now swb-compliant in both portrait and landscape.

TBD: Options menu and Info view

Comment 24

[Russ Nicoletti [:russn]]

My patch is complete. I have tested on master after bug 1054126 landed. I'm still seeing the issue with mozRequestFullScreen as described in comment 7.

There is one known issue with button placement that I haven't been able to resolve. The overlay cancel button on the option menu in landscape is too long, it gets truncated by the swb area. This css line is not being applied: https://github.com/mozilla-b2g/gaia/pull/23340/files#diff-6c9949e0dfcdee5c170ee410a331d6a8R290. I found that when the 'position' property (line 270 of action_menu.css in the PR diff) is 'fixed' the 'right' property is not applied. I tried using 'width: calc(100% - 5rem)' but that is also not applied (although 'width' is applied if simply a percentage).

Comment 25

[Russ Nicoletti [:russn]]

Attachment: Overlay cancel button truncated

Comment 26

[Russ Nicoletti [:russn]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

options-cancel-button issue has been resolved. Patch is ready to be reviewed.

Comment 27

[Punam Dahiya [:pdahiya]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Russ
Patch looks good but few key things that needs to be looked into
1. With the patch, for devices such as flame ,hamachi where we have physical home button soft home key button is showing for video app. Am i missing some dependency patch?

2. On testing with today's m-c build on Nexus 4, video is not displaying correct inside player in full screen view. it could be same issue as comment #7.

3.We should bring changes in shared/style/confirm.css inside video.css as it will impact other apps using shared style.


Thanks
Punam

Comment 28

[Punam Dahiya [:pdahiya]]

4. Missed one, in landscape mode while sharing video 'share with' options are  getting truncated.

Comment 29

[Russ Nicoletti [:russn]]

(In reply to Punam Dahiya from comment #27)
> Comment on attachment 8481591 [details] [review]
> Github PR: https://github.com/mozilla-b2g/gaia/pull/23500
> 
> Hi Russ
> Patch looks good but few key things that needs to be looked into
> 1. With the patch, for devices such as flame ,hamachi where we have physical
> home button soft home key button is showing for video app. Am i missing some
> dependency patch?
> 
> 2. On testing with today's m-c build on Nexus 4, video is not displaying
> correct inside player in full screen view. it could be same issue as comment
> #7.
> 
> 3.We should bring changes in shared/style/confirm.css inside video.css as it
> will impact other apps using shared style.
> 
> 
> Thanks
> Punam

1. Yes, 'setprop ro.moz.has_home_button 0' (bug 1060449)
2. Very likely what I mentioned in comment #7. Bug created: 1062022
3. and 4. I am working on addressing these issues.

Comment 30

[Russ Nicoletti [:russn]]

Attachment: Gallery share mozActivity

I've updated the PR to address issue 3. from comment 27. Regarding issue 4., that screen is from the mozActivity; it's not controlled by the app as I understand it. In fact, the two apps I tested (gallery and sms) have the same issue with mozActivity dialogs being truncated by the software home button. See attachment of gallery share activity dialog. I believe this is related to bug 1058675. I'm changing the review back to r? as there are separate bugs for issues 1., 2. and 4.

Comment 31

[Russ Nicoletti [:russn]]

Eli, is the issue of mozActivity dialogs being overlaid by the software home button the same problem as bug 1058675? See issue 4. in comment 30.

Comment 32

[:Eli Perelman]

No, it technically isn't the same issue as bug 1058675, it relates more to bug 1061962 in which action and object menus have their buttons overlaid. I'll see about modifying my patch there to account for both of these scenarios.

Comment 33

[Punam Dahiya [:pdahiya]]

(In reply to Russ Nicoletti [:russn] from comment #30)
> Created attachment 8483203 [details]
> Gallery share mozActivity
> 
> I've updated the PR to address issue 3. from comment 27. Regarding issue 4.,
> that screen is from the mozActivity; it's not controlled by the app as I
> understand it. In fact, the two apps I tested (gallery and sms) have the
> same issue with mozActivity dialogs being truncated by the software home
> button. See attachment of gallery share activity dialog. I believe this is
> related to bug 1058675. I'm changing the review back to r? as there are
> separate bugs for issues 1., 2. and 4.

Thanks Russ,can you also please check landscape mode, I think comment #28 issue is more specific to landscape mode

Comment 34

[Russ Nicoletti [:russn]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Punam, I've updated the patch to remove the confirm.css file I added. Back to r?

Comment 35

[Punam Dahiya [:pdahiya]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Russ
Patch looks good, one small update needed that I have commented in github. The PR has my r+ but it's not ready to land as this fix is dependant on 1060449, 1062022 and 1061962 and should be landed only after testing once dependencies has resolved. Thanks

Comment 36

[Jason Smith [:jsmith]]

Removing from meta as this is not a blocker for 2.1.

Comment 37

[Russ Nicoletti [:russn]]

Now that bug 1062022 has been resolved and this PR has been updated, the PR is ready to be reviewed again (and removing the NI for Markus).

There is one outstanding issue that is being tracked separately (bug 1065654)

Comment 38

[Russ Nicoletti [:russn]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Punam, can you review the PR again? I have made changes to resolve bug 1062022.

Comment 39

[Punam Dahiya [:pdahiya]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Russ

I still see the issue 1,2,& 4 in comment #27 and comment #28 with the attached patch. I tested on Nexus-4 with today's master build. Here's the link to video

http://youtu.be/yyWSVmJWE4k

Comment 40

[Russ Nicoletti [:russn]]

To clarify Punam's remarks, the issue she is describing is that in fullscreen mode, when changing orientation of the device, the video is distorted in the same way as described in the original description of this bug. I am able to reproduce this on my flame. 

After investigating, I've found the height and width of the video player container (<div id="video-container">) are not updated when the orientation is first changed. This causes the 'transform' to be set incorrectly (when resizing the video to fit within the newly resized container). For example, when entering fullscreen, that is, after calling mozRequestFullScreen, if in portrait orientation where the width is 320px and the height 569px, when orientation is first changed to landscape, the width and height of #videoContainer are still 320px, 569px and the transform is set to 'translate(0px, 194.57px) scale(0.3747)' instead of 'translate(0px, 0.0937px) scale(0.6663)' as it should be when in landscape. 

Interestingly, the width and height of the container are updated on subsequent orientation changes, which cause the width and height to be always reversed and for the transform to be always incorrect while in fullscreen.

For reference, the relevant html elements are: https://github.com/mozilla-b2g/gaia/blob/master/apps/video/index.html#L75-L80

mozRequestFullScreen is being called on the #player-view, the video is being fit into the #video-container.

Comment 41

[Etienne Segonzac (:etienne)]

I'm not sure I have anything of value to provide. 
Tested again with another app, and the appWindow frame is correctly set in this case.

It looks like this is a VideoUtils.fitContainer issue and I'm not familiar at all with this code.

Comment 42

[Russ Nicoletti [:russn]]

Michael, according to Djf (https://bugzilla.mozilla.org/show_bug.cgi?id=1037255#c5), the gallery and video apps have had issues with mozRequestFullScreen in the past when switching orientation. To summarize the issue I'm seeing (from comment 40), After having called mozRequestFullScreen, when first switching orientation, the width and height of the video container are not updated. When subsequently changing orientation, the width and height of the video container are updated but to the values corresponding to the previous orientation. The result is when changing to landscape, the width/height correspond portrait; when changing back to portrait, the width/height correspond to landscape, and so on.

I have confirmed this issue does not reproduce when not calling mozRequestFullScreen. 

The video app already enables the "fullscreen" manifest flag so it seems like overkill to request fullscreen via the API. But since we're so far down this road I don't think we can go back.

How would you like to proceed?

Comment 43

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Russ Nicoletti [:russn] from comment #42)
> Michael, according to Djf
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1037255#c5), the gallery and
> video apps have had issues with mozRequestFullScreen in the past when
> switching orientation. To summarize the issue I'm seeing (from comment 40),
> After having called mozRequestFullScreen, when first switching orientation,
> the width and height of the video container are not updated. When
> subsequently changing orientation, the width and height of the video
> container are updated but to the values corresponding to the previous
> orientation. The result is when changing to landscape, the width/height
> correspond portrait; when changing back to portrait, the width/height
> correspond to landscape, and so on.

It's possible there is a platform bug here since we do have special css rules when an app requests fullscreen. Does this only happen with video content, or is it any divs inside #player-view not getting the appropriate dimensions? Also, did you verify this wasn't an issue with VideoUtils.fitContainer as Etienne suggested? If it is not, can you come up with a simplified test case where this happens and file a blocking bug with me cc'ed? I'll make sure to get eyes on it.


> The video app already enables the "fullscreen" manifest flag so it seems
> like overkill to request fullscreen via the API. But since we're so far down
> this road I don't think we can go back.

Well, we decided a while back from an API perspective that calling mozRequestFullscreen in a fullscreen_layout app is the best way to hide SHB. We should fix bugs with this approach rather than reconsider it.

Comment 44

[Russ Nicoletti [:russn]]

I verified that VideoUtils.fitContainer is not causing the problem by logging the container's width and height at the very beginning of the function. The width and height were incorrect as I described in comment 42. Given the incorrect width and height, VideoUtils.fitContainer was working as expected. Also, when I commented-out the mozRequestFullScreen call, I no longer saw the incorrect width and height.

I updated the testfullscreen html written by John Hu (bug 1062022) to log clientWidth and clientHeight when the window is resized [1]. But when I test it on my device, the window is not getting the resize events when in fullscreen (after calling mozRequestFullScreen). Not sure what's happening with that. Can you take a look?

[1] http://russnicoletti.github.io/requestFullScreen.html

Comment 45

[Punam Dahiya [:pdahiya]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Russ

I am cancelling review request, please set it again when the issue around using mozRequestFullScreen API is resolved. Thanks

Comment 46

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Russ Nicoletti [:russn] from comment #44)
> I verified that VideoUtils.fitContainer is not causing the problem by
> logging the container's width and height at the very beginning of the
> function. The width and height were incorrect as I described in comment 42.
> Given the incorrect width and height, VideoUtils.fitContainer was working as
> expected. Also, when I commented-out the mozRequestFullScreen call, I no
> longer saw the incorrect width and height.

If it's the video container (not the video element itself?) which is having problems, then perhaps this is not a video element problem at all. Please come up with a reduced test case where calling mozRequestFullscreen results in improper layout properties on an element. Looking back on this bug and bug 1062022, it's still not clear where the problem is.

> I updated the testfullscreen html written by John Hu (bug 1062022) to log
> clientWidth and clientHeight when the window is resized [1]. But when I test
> it on my device, the window is not getting the resize events when in
> fullscreen (after calling mozRequestFullScreen). Not sure what's happening
> with that. Can you take a look?

I took a look, but it was hard to tell what was going on because of the timing of alerts (they would show up late probably cause the phone was busy with playback). Please don't use alerts to debug async code like this, they only complicate timing issues. Use console.log or dump instead.

Comment 47

[Russ Nicoletti [:russn]]

Attachment: KK v180, fullscreen, portrait, software home button

Micheal, using KK v180 on my flame, I'm seeing differences in the behavior of the software home button compared to the spec you referenced in the original description [1]. As you can see in the screenshot attached -- of a video in fullscreen mode (after having called mozRequestFullScreen) -- the software home button is not hidden. Can you comment on:

1) Whether using KK v180 is a valid configuration to test swb functionality
2) If the behavior I described and shown in the attachment is expected.

[1] https://bug1036339.bugzilla.mozilla.org/attachment.cgi?id=8473080

Comment 48

[Russ Nicoletti [:russn]]

More differences in the behavior of the software home button compared to the spec using KK v180 image:

1) The system is updating the layout of the elements on the screen based on the presence of the software home button. Therefore, the video app need not -- should not -- update the layout to account for the presence of the software home button.
2) The software home button always appears at the bottom of the screen regardless of the orientation of the device.

Comment 49

[Michael Henretty [:mikehenrty][:mhenretty]]

I spoke with Russ today about this on IRC. We don't believe this is related to KK vs JB, but it is possible. He is currently looking into why fullscreen_layout apps no longer hide the SHB when requesting fullscreen. Sounds like a regression in gaia rather than the base image.

Comment 50

[Russ Nicoletti [:russn]]

(In reply to Michael Henretty [:mhenretty] from comment #49)
> I spoke with Russ today about this on IRC. We don't believe this is related
> to KK vs JB, but it is possible. He is currently looking into why
> fullscreen_layout apps no longer hide the SHB when requesting fullscreen.
> Sounds like a regression in gaia rather than the base image.

Bug 1073611 has been created based on my investigation of this issue.

Comment 51

[Russ Nicoletti [:russn]]

Comment on attachment 8481591 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/23500

Hi Punam, this patch is ready to review. All issues are resolved except for bug 1073611 (system app regression).

In order to test this patch, you'll need to flash this build (as explained in bug 1073611):

Gaia      6cb5e0100d70313e4922c8d34bf20dcdd66ef616
Gecko     https://hg.mozilla.org/mozilla-central/rev/2db5b64f6d49
BuildID   20140912040204
Version   35.0a1

And then follow the instructions here: https://bugzilla.mozilla.org/show_bug.cgi?id=1073611#c1

Comment 52

[Russ Nicoletti [:russn]]

The PR has been updated to reflect changes to the height/width of the software home button in system app. 

Also, there was an oversight in the original PR where confirms.css should be have been lazy loaded in 
showInfoView because the info-view is a "confirm" dialog and recently we no longer load confirm.css when the app starts (commit 9e486fbdc28424eeb3ebd346df725feee38e571d, Aug-22).

Comment 53

[:Eli Perelman]

This seems like a scenario where it would be beneficial to be able to share the CSS variable of the SHB with resources outside of the System app. Etienne, do you think there is a possibility of making this shared in some fashion?

Comment 54

[:Eli Perelman]

I guess worst-case scenario, you could re-declare the CSS variable in the video.css file and use that throughout.

Comment 55

[Russ Nicoletti [:russn]]

(In reply to :Eli Perelman from comment #54)
> I guess worst-case scenario, you could re-declare the CSS variable in the
> video.css file and use that throughout.

Agreed. For now, I will re-declare the variable. Will wait to hear from Etienne about sharing the variable.

Comment 56

[Etienne Segonzac (:etienne)]

(In reply to Russ Nicoletti [:russn] from comment #55)
> (In reply to :Eli Perelman from comment #54)
> > I guess worst-case scenario, you could re-declare the CSS variable in the
> > video.css file and use that throughout.
> 
> Agreed. For now, I will re-declare the variable. Will wait to hear from
> Etienne about sharing the variable.

Duplicating it is a bit painful, but we need to feel the pain otherwise we'll never push for the proper gecko fix that will fix it for third party too. (I think Vivien has a plan.)

Comment 57

[Russ Nicoletti [:russn]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/24715

Creating new PR -- existing PR cannot be reopened as the branch was force-pushed.

Comment 58

[Punam Dahiya [:pdahiya]]

Comment on attachment 8499007 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/24715

Hi Russ
Patch looks good and viewing fullscreen video experience by hiding software home button looks great. 
I have commented in github. Most of the comments are nits but main reason that's not related to your changes but reason for r- to land this patch is handle pick activity,  shows a prompt, 'app://video.gaiamobile.org is now fullscreen' that's unexpected for user. Hitting allow in prompt shows SHB button issue seen in 1073611 . Hitting disallow and click on thumbnail again shows the prompt.
Can you please check and set the review flag when its resolved. Thanks for your patience and work on this patch.

Comment 59

[Russ Nicoletti [:russn]]

Attachment: pick activity app permission

Shows the "app permission" prompt displayed when user selects "Video" during pick activity.

Comment 60

[Russ Nicoletti [:russn]]

Comment on attachment 8499007 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/24715

Hi Punam. Thanks for the review. And thanks for catching the pick activity issue. I have addressed that by not requesting full screen during pick activities. I have elaborated on that in the PR comments. I have also addressed your other review comments.

Back to you :)

Comment 61

[Punam Dahiya [:pdahiya]]

Comment on attachment 8499007 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/24715

Hi Russ
Thanks for the updated patch, it looks good and has Video app updated to handle fullscreen_layout property. As discussed over IRC, I see following open issues outside scope of video app

1. Select video thumbnail to play video -> Rotate to view in landscape mode -> click top right options icon -> click share -> Share with screen in landscape mode is hidden by SHB on the right.

2. Play video -> click options -> share -> bluetooth -> Confirmation dialog ( Bluetooth is disabled) footer cancel and ok button stays hidden behind SHB.

3.--software-home-button-height variable is re-declared and duplicated in video.css and confirm.css, fix is tied to Vivian and Etienne plan on how to handle it best (#comment56).

Comment 62

[Russ Nicoletti [:russn]]

Attachment: activity sharing screen in landscape

Attached is a screenshot of issue 1 from comment 61

Comment 63

[Russ Nicoletti [:russn]]

Attachment: bluetooth confirmation

Attached is a screenshot of issue 2 from comment 61

Comment 64

[Russ Nicoletti [:russn]]

Micheal, can you comment on issue 1 and 2 from comment 61? They seem like bugs. Do you agree?

Comment 65

[Michael Henretty [:mikehenrty][:mhenretty]]

Hi Russ, yup those are definitely bugs in the system app. Please file bugs for these and CC me.

Thank you Russ and Punam for finding/debugging these issues!

Comment 67

[Michael Henretty [:mikehenrty][:mhenretty]]

The patch here currently has no tests, please add an integration test for the SHB hiding case before landing.

Comment 68

[Patryk Adamczyk [:patryk] UX]

Can we resurrect this patch and bring it into Spark, this really hurts the video playback UX. 
Thanks!

Comment 69

[Candice Serran (:cserran)]

[Blocking Requested - why for this release]: putting the spark? tag for triaging and review with :drs

Comment 70

[Doug Sherk (:drs) (inactive)]

Russ, how hard would it be to take this to completion?

Comment 71

[Russ Nicoletti [:russn]]

There are two tasks remaining for this bug:

1) update the PR (the existing PR is surely stale) and manually test
2) create integration tests for the feature

Estimating 2) is not completely straightforward for me because I am not very familiar with gaia integration tests as I have struggled to get them to run on my laptop and as a result not written any.

I can get 1) done by the end of the week.

Comment 72

[Russ Nicoletti [:russn]]

1) from comment 71 is proving to be more difficult than I anticipated. I recreated the patch against the latest from master but it is not functioning as expected. When the video is supposed to be fullscreen (when calling mozRequestFullScreen) it is getting distorted. I recall this was a problem previously (comment 7), needs more investigation. Also, it appears the height of the shb has changed since the original patch was created so the css needs some tweaking. Finally, the "cancel" button background of the action menu is not being moved up and is partially hidden by the shb. The button is moved up but not the background.

Comment 74

[BMO Automation]

Firefox OS is not being worked on