1215077 - Fix icon flicker and AppChrome races and add support for the background_color property in the manifest

Status: RESOLVED WONTFIX

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

Description

[vnicolas]

Attachment: system.diff

This patch add support for the background_color property in the manifest. The property is disabled by default beeing a flag and it can be enable from the developer section in the Settings app.

The patch change a few behavior of the window management code such as:
  - No more lazy AppChrome for apps. It is built when the AppWindow is built, similarly to regular Web Content.
 - Metachanges handling are now delegated to the AppChrome. AppWindow has nothing to do with that.
 - AppWindow.themeColor does not exists anymore. It is replace by AppChrome.themeColor (and now there is also AppChrome.themeGroup and AppChrome.backgroundColor).
 - AppWindow.setFrameBackground is now synchronous to not repaint while the app is animating.
 - The AppWindow is hidden until AppChrome is fully ready to avoid visual janks.
 - PopupWindow has an extra property isPopupWindow. That sounds way cleaner than checking the CLASS_NAME honestly. So this is similar to isHomescreen.
 - PopupWindow is not trying to manage the theme color itself anymore. This is delegated to AppChrome.
 - Theme Color and Theme Group can be set from the manifest, which allow starting the app with the right color during a cold launch and avoid a lot of extra repaints in the system app when an app is launched.
 - Standalone app does not listen anymore for navigation events (ex: locationchange). This prevent changing the layout and doing extra frames during app life. This is especially useful for nga apps.

Happy review.

Comment 1

[vnicolas]

Attachment: background_color support

I realized that the patch ends up regressing bug 869903. I change a bit how the background color hierarchy of the system app changes.
Now all the background-color of sub-elements are derived from the root container.

TBH I think background_color support will deserve its own CSS files as it is pretty important, similarly to window_layout.css. I didn't make that in that patch though.

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 8674232 [details] [diff] [review]
background_color support

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

If there's one thing we'd want a a settings for it's the hacky "theme-group" (which is my bad).

I'd vote to remove the setting here (and default on).
The gaia apps will basically opt in the feature by defining the field.
And we're just honoring what third party apps are asking.

Of course, we can easily add the setting back if there's debate, but I don't think there will be.
And this way we're saving ourselves from one more soon-forgotten-setting (remember app suspending? :)). 


PS: this patch is now obsolete, so I'm dumping this "first pass" for comments and I'll take a look at the new patch.

::: apps/system/js/app_chrome.js
@@ +75,5 @@
>  
>      this.reConfig();
> +    this.ready = new Promise((resolve) => {
> +      this.setSiteIcon(resolve);
> +    });

+1

::: apps/system/js/app_window.js
@@ +681,5 @@
>      if (this.config.stayBackground || this.isHomescreen) {
>        this.setVisible(false);
>      }
>  
> +    this.setFrameBackground();

I think there's a few |this.sinon.clock.tick()| in the |setFrameBackground| test suite that we want to remove because of this

@@ +875,5 @@
>  
> +      this.appChrome = new AppChrome(this);
> +      this.appChrome.ready.then(() => {
> +        this.element.hidden = false;
> +      });

we need a test for this!
(the element being hidden until the appChrome's promise resolve)
+ the inputWindow special case

::: apps/system/style/window.css
@@ +642,5 @@
>    background: none;
>    pointer-events: none;
>  }
> +
> +/* Root background-color hierarchy */

feel free to over-comment here, those selectors are so hard to grasp

::: apps/system/test/unit/app_chrome_test.js
@@ +381,5 @@
> +      this.sinon.stub(chrome, 'handleEvent');
> +      events.forEach(function(type) {
> +        var evt = new CustomEvent(type);
> +        app.element.dispatchEvent(evt);
> +        assert.isFalse(chrome.handleEvent.called);

nit: isTrue(....notCalled) to be consistent with the previous one.
or better: sinon.assert.notCalled(spy)

@@ +385,5 @@
> +        assert.isFalse(chrome.handleEvent.called);
> +      });
> +    });
> +
> +    test('Ensure metachange event is dispatched', function() {

'Ensure we are listening to metachange events'

@@ +452,5 @@
> +      this.sinon.stub(chrome, 'handleEvent');
> +      events.forEach(function(type) {
> +        var evt = new CustomEvent(type);
> +        app.element.dispatchEvent(evt);
> +        assert.isTrue(chrome.handleEvent.called);

I think that if 'mozbrowserloadstart' is being listened on, all other assertion will automatically pass.
You can
|assert.equal(chrome.handleEvent.callCount, events.length)| after the loop maybe

@@ +660,5 @@
> +        detail: {
> +          name: 'application-name',
> +          content: 'Phone2'
> +        },
> +      });

nit: I like to use a real dispatchEvent everywhere to make the test more "real"

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 8674232 [details] [diff] [review]
background_color support

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

Clearing for now, I'll have a second quick look on the rebased version with the first set of comments addressed and we should be good to go.
And a try build would help too, since we never know which part of the WindowManager internals are used by some integration tests.

Comment 4

[vnicolas]

Attachment: Add background_color support. v2.

Review comments addressed. Except the one relative to using real Element.dispatchEvent.
I would love to do it too but the event is dispatched on the mock_app_window. This one does not really nest the DOM as it is in reality. I can fix mock_app_window but I feel like this is out of the scope of this bug.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] vingtetun:background_color > mozilla-b2g:master

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Autolander from comment #5)
> Created attachment 8675714 [details] [review]
> [gaia] vingtetun:background_color > mozilla-b2g:master

Some interesting failures :)
https://treeherder.mozilla.org/#/jobs?repo=gaia&author=[e-mail]

Comment 7

[Etienne Segonzac (:etienne)]

> @@ +452,5 @@
> > +      this.sinon.stub(chrome, 'handleEvent');
> > +      events.forEach(function(type) {
> > +        var evt = new CustomEvent(type);
> > +        app.element.dispatchEvent(evt);
> > +        assert.isTrue(chrome.handleEvent.called);
> 
> I think that if 'mozbrowserloadstart' is being listened on, all other
> assertion will automatically pass.
> You can
> |assert.equal(chrome.handleEvent.callCount, events.length)| after the loop
> maybe
> 

I don't think this comment was addressed, but we can talk about it tomorrow if it's unclear :)

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8675651 [details] [diff] [review]
Add background_color support. v2.

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

Almost there, let's sync up and land this tomorrow!

::: apps/system/js/app_window.js
@@ +1071,5 @@
>        // class on browser-container until the proper patch on the external
>        // repo.
>        this.browserContainer.classList.add('render');
>        // Force removing background image.
> +      this.browserContainer.style.backgroundImage = 'none';

we don't have a test covering this cleanup on loadend

::: apps/system/js/popup_window.js
@@ +40,5 @@
>        // We have to apply the style on the title bar element because the
>        // popup appChrome element doesn't overlap. See http://bugzil.la/1132418
> +      if (this.statusbar) {
> +        this.statusbar.titleBar.style.backgroundColor =
> +          this.rearWindow.appChrome.containerElement.style.backgroundColor;

the statusbar looks pretty weird when opening an "import" page in the contacts app, we might be missing a case here.
same for the "find duplicate" window.

white icons on a gray background :/

::: apps/system/style/window.css
@@ +648,5 @@
>  
>  .appWindow.lockScreenInputWindow {
>    background: none;
>    pointer-events: none;
>  }

<3 it, minus the trailing spaces

::: apps/system/test/unit/app_window_test.js
@@ +1147,5 @@
> +
> +    test('Not hidden if InputMethod (no AppChrome)', function() {
> +      app1 = new AppWindow(fakeInputAppConfig);
> +      assert.isFalse(app1.element.hidden);
> +    });

:+1:

Comment 9

[vnicolas]

(In reply to Etienne Segonzac (:etienne) AFK on October 21 and 26 from comment #7)
> > @@ +452,5 @@
> > > +      this.sinon.stub(chrome, 'handleEvent');
> > > +      events.forEach(function(type) {
> > > +        var evt = new CustomEvent(type);
> > > +        app.element.dispatchEvent(evt);
> > > +        assert.isTrue(chrome.handleEvent.called);
> > 
> > I think that if 'mozbrowserloadstart' is being listened on, all other
> > assertion will automatically pass.
> > You can
> > |assert.equal(chrome.handleEvent.callCount, events.length)| after the loop
> > maybe
> > 
> 
> I don't think this comment was addressed, but we can talk about it tomorrow
> if it's unclear :)

It's clear. I'm sure to (In reply to Etienne Segonzac (:etienne) AFK on October 21 and 26 from comment #7)
> > @@ +452,5 @@
> > > +      this.sinon.stub(chrome, 'handleEvent');
> > > +      events.forEach(function(type) {
> > > +        var evt = new CustomEvent(type);
> > > +        app.element.dispatchEvent(evt);
> > > +        assert.isTrue(chrome.handleEvent.called);
> > 
> > I think that if 'mozbrowserloadstart' is being listened on, all other
> > assertion will automatically pass.
> > You can
> > |assert.equal(chrome.handleEvent.callCount, events.length)| after the loop
> > maybe
> > 

It does. Some other places are doing something similar but with assert.isFalse(chrome.handleEvent.notCalled);

If you think this is unclear I can replace that by some checks to:
|assert.equal(chrome.handleEvent.callCount, 0)| after the loop if you want ?

Comment 10

[vnicolas]

(In reply to Etienne Segonzac (:etienne) AFK on October 21 and 26 from comment #8)
> Comment on attachment 8675651 [details] [diff] [review]
> Add background_color support. v2.
> 
> Review of attachment 8675651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there, let's sync up and land this tomorrow!
> 
> ::: apps/system/js/app_window.js
> @@ +1071,5 @@
> >        // class on browser-container until the proper patch on the external
> >        // repo.
> >        this.browserContainer.classList.add('render');
> >        // Force removing background image.
> > +      this.browserContainer.style.backgroundImage = 'none';
> 
> we don't have a test covering this cleanup on loadend
> 
> ::: apps/system/js/popup_window.js
> @@ +40,5 @@
> >        // We have to apply the style on the title bar element because the
> >        // popup appChrome element doesn't overlap. See http://bugzil.la/1132418
> > +      if (this.statusbar) {
> > +        this.statusbar.titleBar.style.backgroundColor =
> > +          this.rearWindow.appChrome.containerElement.style.backgroundColor;
> 
> the statusbar looks pretty weird when opening an "import" page in the
> contacts app, we might be missing a case here.
> same for the "find duplicate" window.
> 
> white icons on a gray background :/

I looked at how popup colors are derived from the parent and decided to make a different system to what is currently done.
This should fix the issue you are seeing as well as a small bug where the system ends up generating 2 * rgba(0,0,0,0.15) overlay on top of the app titlebar instead of 1.

Comment 11

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

Vivien, are you planning on landing this in 2.5? What is the risks vs. reward ratio for this feature? As far as I know, we don't have anything specific in 2.5 that takes advantage of background_color. If that is true, would you consider delaying landing this until after 2.5?

Comment 12

[Etienne Segonzac (:etienne)]

To be clear, I'm fine with splitting this :)
I just really want the race/flicker fixes because, you know, quality.

Comment 13

[Etienne Segonzac (:etienne)]

That said, the raptor results are worrisome (base is master, ran them on a flame):

sms.gaiamobile.org     base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
navigationLoaded              996     1097       101      * 0.00
willRenderThreads            1043     1150       106      * 0.00
navigationInteractive        1048     1154       106      * 0.00
visuallyLoaded               1344     1498       154      * 0.00
fullyLoaded                  1418     1572       154      * 0.00
rss                        36.273   35.683    -0.590        0.20
uss                        17.209   16.579    -0.630        0.20
pss                        21.275   20.619    -0.657        0.21

Comment 14

[vnicolas]

(In reply to Etienne Segonzac (:etienne) AFK on October 21 and 26 from comment #13)
> That said, the raptor results are worrisome (base is master, ran them on a
> flame):
> 
> sms.gaiamobile.org     base: mean  1: mean  1: delta  1: p-value
> ---------------------  ----------  -------  --------  ----------
> navigationLoaded              996     1097       101      * 0.00
> willRenderThreads            1043     1150       106      * 0.00
> navigationInteractive        1048     1154       106      * 0.00
> visuallyLoaded               1344     1498       154      * 0.00
> fullyLoaded                  1418     1572       154      * 0.00
> rss                        36.273   35.683    -0.590        0.20
> uss                        17.209   16.579    -0.630        0.20
> pss                        21.275   20.619    -0.657        0.21

Currently investigating it. Probably related to some part of AppChrome. Not sure which one yet.

Comment 15

[vnicolas]

(In reply to Michael Henretty [:mhenretty] from comment #11)
> Vivien, are you planning on landing this in 2.5? What is the risks vs.
> reward ratio for this feature? As far as I know, we don't have anything
> specific in 2.5 that takes advantage of background_color. If that is true,
> would you consider delaying landing this until after 2.5?

I will land that once it will be ready. Not sure it will be ready by 2.5.
In order to take advantages of background_color once this bug has landed it should be a matter of putting the right fields in the manifest. But tbh it looks even better once something like bug 1211853 will be available too.

Comment 16

[BMO Automation]

Firefox OS is not being worked on