829362 - don't use mozRequestFullscreen in the gallery app

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

Background: Gaia apps have two ways of being in fullscreen mode.  Some apps (like Camera) are always in fullscreen mode and they acheive this simply by setting a flag in their manifest file.  Apps like this are fine.

Most apps don't use fullscreen at all, and they are fine too.

That leaves just a couple of apps like Gallery (and Video) that switch back and forth between regular and fullscreen mode using mozRequestFullScreen().  But mozRequestFullScreen() turns out to be a pit of despair and the cause of a lot of bugs:

Bug 823327 - White flash when going into/out of fullscreen in gallery
Bug 825848 - [GALLERY] Graphical issues when rotating photos in full screen
Bug 824586 - [Open_]unlock the lockscreen ,cant not return to the gallery preview interface
Bug 822182 - When back from fullscreen-ed app, the homescreen should display immediately.
Bug 829321 - Can't resume fullscreen mode when returning to app from homescreen

So this bug is about changing gallery so that it never calls mozRequestFullScreen() at all.

The patch I'm about to attach will also set the manifest flag to turn on fullscreen permanently.  This means that we'll always be in fullscreen for the thumbnail list, edit mode, etc.  If that's a problem it is trivial to change the manifest again so that we're never in fullscreen mode.

Its actually amazing how much more responsive the app feels without the flashing and the pauses required to go in and out of fullscreen.  Cc'ing Josh and Casey about this.  I think you're really going to like the app better this way.

Comment 1

[David Flanagan [:djf]]

Attachment: link to patch on github

Dale,

I'm asking you for another review, because you understand the pain of mozRequestFullScreen().

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 5 different fullscreen-related bugs. see description 
User impact if declined: gallery will be sucky 
Testing completed: yes
Risk to taking this patch (and alternatives if risky): not risky

Comment 2

[David Flanagan [:djf]]

Josh and Casey,

I think you'll like this. Its a pretty trivial patch that works around 5 different fullscreen bugs. And the app ends up feeling much faster.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 700763 [details]
link to patch on github

Reassigning review to Dominic because he is awake

Comment 4

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

Comment on attachment 700763 [details]
link to patch on github

This sounds a sane change to me. It will also prevent weird case where the edit mode UI seems broken.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 700763 [details]
link to patch on github

David, this patch is really nice! r+++++

I feel the repainting of Gallery app is much more faster than before. I believe UX people, like Josh and Casey will love this change.

And I have also tested all the bugs you mentioned above, all of them are fixed.

Vivien, I think this patch should be safe to land because now Gallery app is always in fullscreen mode, we don't have to handle some events like mozfullscreenchange or resize, which might cause Gallery app to have some behaviours that shouldn't have.

Comment 6

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

https://github.com/mozilla-b2g/gaia/commit/bb1e1ffe3807d6dc91a977cf288db94388bb75b3

Comment 7

[Casey Yee]

UX Hero achievement unlocked!  \o/ Very nice!