866174 - Use an icon/splashscreen for application cold launch

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Patch

Here the first shoot. If someone wants to take over this work that fine too.

Comment 2

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

Also I forgot to remove the generic splashscreen from the system app / cost control but I have a local fix for it.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks for the clean-up. Shouldn't we talk to UX before check-in this?

Comment 4

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> Thanks for the clean-up. Shouldn't we talk to UX before check-in this?

I'm speaking with them in a mail thread so they are fine with it. Also the current splash are temporary and they will update it later.

Comment 5

[Eric Pang [:epang] UX]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #4)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> > Thanks for the clean-up. Shouldn't we talk to UX before check-in this?
> 
> I'm speaking with them in a mail thread so they are fine with it. Also the
> current splash are temporary and they will update it later.

Hi Vivien, I was hoping to test your patch out, but can't seem to figure out how to get it onto my device.  Can you help walk me through it?  Or if you can create a branch on git, i should be able to figure it out from there :). Thanks!

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 742443 [details] [diff] [review]
Patch

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

Sorry for the really really late review. Overall the code looks good, given it remove a large part of screenshot storage part in window_manager.js (if we need it later, rewrite it and put it to another module is better).

r+ if the following concerns are addressed.

1. #ccc background for app iframe. web pages are traditionally loaded against a white background (not in any spec AFAIK but a de-facto standard). There should be a better argument here before we change the background color.
2. Removing the background-color of the app content will sometimes make active elements "floating" on the screenshot, and splash in this case. This might result a noticeable visual defect during code launch. I am not sure if you really want it.
3. the use of the largest image in the manifest.icons object as splash. I cannot see the actual image on Bugzilla, but if they are simply large icons, naming icons as 'splash.png' is misleading. Second of all, establish a convention in Gaia that the largest icon == image used in splash is more worse than, say, manifest.spalsh_image. I cannot say I agree the approach here. At least (A) the file name should be changed, and (B) instead of go for the largest icon in getIconForSplash(), the function should consider the screen size (and the actual icon placement on the splash). (B) can be addressed in a following bug.

::: apps/browser/index.html
@@ +15,5 @@
>      <link rel="stylesheet" href="style/browser.css" type="text/css">
> +    <!-- Shared responsive -->
> +    <link rel="stylesheet" href="shared/style/responsive.css" type="text/css">
> +    <!-- Shared responsive -->
> +    <link rel="stylesheet" href="shared/style/responsive.css" type="text/css">

duplicate line here.

::: apps/browser/manifest.webapp
@@ +42,2 @@
>      "120": "/shared/resources/branding/Browser.png",
> +    "320": "/style/icons/splash/splash.png"

where did you get the splash.png? is that the correct usage of 'icons' property?

::: apps/browser/style/browser.css
@@ -2,4 @@
>    height: 100%;
>    padding: 0;
>    margin: 0;
> -  background-color: #575E66;

I am not sure if removing the background color is a good idea -- you would ended up having app content stack on top of the code launch splash right? For the screenshot it is not that noticeably, but if we are moving away from that we should keep the background color.

@@ -158,4 @@
>    text-overflow: ellipsis;
>  }
>  
> -#tabs-list .close:hover, #tabs-list .close:active {

I wonder why this patch touches browser.css other than removing the background color?

::: apps/communications/dialer/style/keypad.css
@@ +70,4 @@
>  #keyboard-container {
>    width: 100%;
>    height: 30rem;
> +  background-color: white;

Interesting... instead of removing the background color, you /added/ it here. Was it moved from somewhere?

::: apps/system/js/window_manager.js
@@ +242,4 @@
>      var objectURL = URL.createObjectURL(blob);
>      frame.dataset.bgObjectURL = objectURL;
>      var backgroundCSS =
> +      '-moz-linear-gradient(top, rgba(0,0,0,0) 0%, rgba(0,0,0,0) 100%),' +

Remove this if we are not going to darken the screenshot.

@@ +246,2 @@
>        'url(' + objectURL + '),' +
> +      ((transparent) ? 'transparent' : '#ccc');

Gray background underneath the iframe?

Also, given the fact you removed comment on L726, you might as well as remove the 3rd parameter (|transparent|) and always use a solid color here.

@@ +526,5 @@
>    function setFrameBackground(frame, callback, transparent) {
> +    try {
> +      var splash = frame.firstChild.splash;
> +    } catch(e) {
> +      debug("manifest: can't find splash\n");

Intentional or unintentional debug()?

@@ +1233,5 @@
> +    var sizes = Object.keys(icons).map(function parse(str) {
> +      return parseInt(str, 10);
> +    });
> +
> +    sizes.sort(function(x, y) { return y - x; });

There are better ways to find the largest number in a given array (instead of sort() the entire array), but let's not be picky here :P

@@ +1289,5 @@
> +      splash = a.protocol + '//' + a.hostname + ':' + (a.port || 80) + splash;
> +
> +      // Start to load the image in background to avoid flickering if possible.
> +      var img = new Image();
> +      img.src = splash;

Interesting. Will this actually prevent flickering?

Comment 7

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #6)
> Comment on attachment 742443 [details] [diff] [review]
> Patch
> 
> Review of attachment 742443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if the following concerns are addressed.
> 
> 1. #ccc background for app iframe. web pages are traditionally loaded
> against a white background (not in any spec AFAIK but a de-facto standard).
> There should be a better argument here before we change the background color.

I add back a white background.

> 2. Removing the background-color of the app content will sometimes make
> active elements "floating" on the screenshot, and splash in this case. This
> might result a noticeable visual defect during code launch. I am not sure if
> you really want it.

That's intentional in our case. I want to avoid a flicker during app load when there is a background-color. I'm opened to fix it an other way in a followup. The only goal is to avoid flickering here.

> 3. the use of the largest image in the manifest.icons object as splash. I
> cannot see the actual image on Bugzilla, but if they are simply large icons,
> naming icons as 'splash.png' is misleading. Second of all, establish a
> convention in Gaia that the largest icon == image used in splash is more
> worse than, say, manifest.spalsh_image. I cannot say I agree the approach
> here. At least (A) the file name should be changed, and (B) instead of go
> for the largest icon in getIconForSplash(), the function should consider the
> screen size (and the actual icon placement on the splash). (B) can be
> addressed in a following bug.

I create a icons/320 folder for each app.  I can't say I'm really happy of using the icons field but Mounir is really against adding a new splash_screen field in the manifest.

In any case it will make a splash for all apps since all of them are required to have an icon. A nice default splash with the icon of the app will be good to have. (hint UX!)
 

> @@ -158,4 @@
> >    text-overflow: ellipsis;
> >  }
> >  
> > -#tabs-list .close:hover, #tabs-list .close:active {
> 
> I wonder why this patch touches browser.css other than removing the
> background color?
>

Mostly because :hover is really weird on a touchscreen device. But yeah that's not 100% part of this patch.
 
> ::: apps/communications/dialer/style/keypad.css
> @@ +70,4 @@
> >  #keyboard-container {
> >    width: 100%;
> >    height: 30rem;
> > +  background-color: white;
> 
> Interesting... instead of removing the background color, you /added/ it
> here. Was it moved from somewhere?

Yeah that avoid a translucent keypad on the dialer app...

> 
> ::: apps/system/js/window_manager.js
> @@ +242,4 @@
> >      var objectURL = URL.createObjectURL(blob);
> >      frame.dataset.bgObjectURL = objectURL;
> >      var backgroundCSS =
> > +      '-moz-linear-gradient(top, rgba(0,0,0,0) 0%, rgba(0,0,0,0) 100%),' +
> 
> Remove this if we are not going to darken the screenshot.
> 

Removed.

> @@ +246,2 @@
> >        'url(' + objectURL + '),' +
> > +      ((transparent) ? 'transparent' : '#ccc');
> 
> Gray background underneath the iframe?
> 
> Also, given the fact you removed comment on L726, you might as well as
> remove the 3rd parameter (|transparent|) and always use a solid color here.
>

Removed.
 
> @@ +526,5 @@
> >    function setFrameBackground(frame, callback, transparent) {
> > +    try {
> > +      var splash = frame.firstChild.splash;
> > +    } catch(e) {
> > +      debug("manifest: can't find splash\n");
> 
> Intentional or unintentional debug()?
> 

Removed.

> @@ +1233,5 @@
> > +    var sizes = Object.keys(icons).map(function parse(str) {
> > +      return parseInt(str, 10);
> > +    });
> > +
> > +    sizes.sort(function(x, y) { return y - x; });
> 
> There are better ways to find the largest number in a given array (instead
> of sort() the entire array), but let's not be picky here :P
>

I agree with you :)
 
> @@ +1289,5 @@
> > +      splash = a.protocol + '//' + a.hostname + ':' + (a.port || 80) + splash;
> > +
> > +      // Start to load the image in background to avoid flickering if possible.
> > +      var img = new Image();
> > +      img.src = splash;
> 
> Interesting. Will this actually prevent flickering?

I feel like it helps. I can't tell it prevent all cases but once the first load has been done it seems helpful (could be only in my mind).

Comment 8

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

https://github.com/mozilla-b2g/gaia/commit/9e3d1e7193c0e6b5007eb036fd0fa671dcc41812

Comment 9

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

Eric you will be able to test in the tree. Splash are just placeholder for now.

Comment 10

[David Flanagan [:djf]]

This patch introduces a console warning: [JavaScript Error: "TypeError: callback is not a function" {file: "app://system.gaiamobile.org/js/window_manager.js" line: 854}]

Comment 11

[Anthony Ricaud (:rik)]

This patch broke the linter tests so I pushed a quick fix.

https://github.com/mozilla-b2g/gaia/commit/012a752598c6f53a95e1d7ea5b068f9db9f2d54a

Comment 12

[Cristian Rodriguez (:crdlc)]

I've pulled master and installed in my device and currently (after this patch) I can see how apps that are running, when I go to the grid and open them again, the animations are not synchronized. When the homescreen' s animation finishes the open-app animation is performed. Can you see the same behavior? Thanks

Comment 13

[Cristian Rodriguez (:crdlc)]

Follow up bug 871940

Comment 14

[Cristian Rodriguez (:crdlc)]

In order to avoid extra work to Vivien I've fixed the bug comment 10 in my follow-up

Comment 15

[Chris Lord [:cwiiis]]

Is there a bug tracking making this look correct for hidpi devices?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Chris Lord [:cwiiis] from comment #15)
> Is there a bug tracking making this look correct for hidpi devices?

Please file a bug if this doesn't look correct on non-hidpi device.

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 9e3d1e7193c0e6b5007eb036fd0fa671dcc41812
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1-train
  git cherry-pick -x  012a752598c6f53a95e1d7ea5b068f9db9f2d54a
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think this bug is uplift-able ... but Vivien and Leo could take the risk if you want it.

Comment 19

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

Landing this bug without some fixes (that have not landed yet) is at risk. It has landed mostly to experiment on a new system for splash screen. If it needs to land on leo then some followups needs to be opened and we need to find some people to work on it...

Comment 20

[Wayne Chang [:wchang]]

Triage - agreement with partners to track and not release blocking.

Comment 21

[Eric Pang [:epang] UX]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> Landing this bug without some fixes (that have not landed yet) is at risk.
> It has landed mostly to experiment on a new system for splash screen. If it
> needs to land on leo then some followups needs to be opened and we need to
> find some people to work on it...

Hi Vivien, I had trouble testing this on my device.  Asset wise, is there anything I can help with?