1033364 - Support mozbrowsermetachange color in the Window Manager

Status: RESOLVED FIXED

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

Description

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

Those patches are here in order to implement a basic support of mozbrowsermetachange for brand color in the Window Manager.

I will likely fix the platform patch in order to let this meta goes throught only for certified apps for now, until the spec is finalized.

But this should not prevent us to start using it in order to prototype and ship something asap, as most of the spec will likely not change too much what's going on.

Comment 1

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

Attachment: bug1033364.part1.patch

Alive I believe you will be the right fit to review those changes.

Comment 2

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

Attachment: bug1033364.part2.patch

Nothing should be visible for now until the class brandcolor-active is set on #screen. I plan to add a pref in the settings app to toggle it.

Comment 3

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

Attachment: bug10333364.part3.patch

Add a preference to turn brand color support on/off

Comment 4

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

With the part2 of the patch I broke the size of windows when the attention screen is minimize. Likely because we were using the StatusBar.height to calculate the window size before and now we don't.

I'm looking into that.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8449401 [details] [diff] [review]
bug1033364.part2.patch

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

basically ok, could you try:

1. AppWindow has a PopupWindow
2. AppWindow has a ActivityWindow
3. ActivityWindow has a PopupWindow
4. PopupWindow has a ActivityWindow
5. Landscape mode

You could use dev_apps/sheet-app-123 to try.

::: apps/system/index.html
@@ +307,5 @@
>  
>      <!-- Windows -->
>      <!-- Any module that wants to intercept the Home button and prevent the -->
>      <!-- homescreen from being displayed must come before this script. -->
> +    <link rel="stylesheet" type="text/css" href="style/window_layout.css">

Do you steal it from https://github.com/alivedise/gaia/commit/1ede288ba7deba09ec88e25d7928f1d9221825c0 ? :P

::: apps/system/js/layout_manager.js
@@ -54,5 @@
>       */
>      get height() {
>        return window.innerHeight -
>          (this.keyboardEnabled ? KeyboardManager.getHeight() : 0) -
> -        StatusBar.height -

This may be incorrect. You still need to consider attention bar height which is inside statusbar before we move attention window into app Window.

::: apps/system/style/window_layout.css
@@ +86,5 @@
> +\*****************************************************************************/
> +
> +/* Regular windows */
> +
> +.appWindow > div,

I didn't try your code but we have div.chrome under .appWindow, please remember to check if this override the rule of .chrome.
refer my code at https://github.com/alivedise/gaia/blob/1ede288ba7deba09ec88e25d7928f1d9221825c0/apps/system/style/window_layout.css

@@ +144,5 @@
> +  bottom: 1rem;
> +}
> +
> +/*****************************************************************************\
> +| Search Window Positioning

Positioning and Layout(ing)

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> Comment on attachment 8449401 [details] [diff] [review]
> bug1033364.part2.patch
> 
> Review of attachment 8449401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> basically ok, could you try:
> 
> 1. AppWindow has a PopupWindow
> 2. AppWindow has a ActivityWindow
> 3. ActivityWindow has a PopupWindow
> 4. PopupWindow has a ActivityWindow
> 5. Landscape mode

6. BrowserWindow(with navigation bar) has a PopupWindow
7. BrowserWindow has a ActivityWindow

> 
> You could use dev_apps/sheet-app-123 to try.
>

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8449447 [details] [diff] [review]
bug10333364.part3.patch

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

::: apps/system/js/app_window_manager.js
@@ +273,5 @@
> +
> +        'app-brandcolor.enabled': {
> +          defaultValue: false,
> +          callback: function(value) {
> +            screenElement.classList.toggle('brandcolor-active', value);

Where is CSS rule about this one? Not in part.1 either.

Comment 8

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> Comment on attachment 8449447 [details] [diff] [review]
> bug10333364.part3.patch
> 
> Review of attachment 8449447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/js/app_window_manager.js
> @@ +273,5 @@
> > +
> > +        'app-brandcolor.enabled': {
> > +          defaultValue: false,
> > +          callback: function(value) {
> > +            screenElement.classList.toggle('brandcolor-active', value);
> 
> Where is CSS rule about this one? Not in part.1 either.

I may have left an outdated version of part2. I have some local changes on top of it in order to land disabled by default.

I will address your comments, fix the issue with the attention screen and post an updated patch.

About window_layout.css I have not seen your patch from the other bug! I'm glad that we came up with the same idea and the same name :)

Comment 9

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

Attachment: bug1033364.part2.patch

Thanks for pointing me to the .chrome part. Yes, the rule was not overidden correctly and it was obvious when I tried to do what you suggest using the sheet apps.

Comment 10

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

Attachment: bug1033364.part3.patch

Last part of the patch, that turns on/off the brand color support.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450125 [details] [diff] [review]
bug1033364.part2.patch

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

r+ with nit.
BTW don't we want statusbar overlay for popup window?

::: apps/system/js/activity_window.js
@@ +166,5 @@
>      return '<div class="appWindow activityWindow inline-activity' +
>              '" id="activity-window-' + _id++ + '">' +
>              '<div class="screenshot-overlay"></div>' +
>              '<div class="fade-overlay"></div>' +
> +            '<div class="statusbar-overlay"></div>' +

Question is do we want to render the statusbar with the activity color

::: apps/system/js/attention_screen.js
@@ +28,5 @@
>    },
>  
> +  get statusHeight() {
> +    if (this.isVisible() && !this.isFullyVisible()) {
> +      return this.attentionScreen.getBoundingClientRect().height;

We had better to record it down to avoid reflow every time access it!

::: apps/system/js/lockscreen_window.js
@@ +91,5 @@
>    LockScreenWindow.prototype._resize = function aw__resize() {
>      var height, width;
>  
>      // We want the lockscreen to go below the StatusBar
> +    height = self.layoutManager.height;

Nice decoupling.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450128 [details] [diff] [review]
bug1033364.part3.patch

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

::: apps/system/js/app_window_manager.js
@@ +270,5 @@
>              }
>            }.bind(this)
> +        },
> +
> +        'app-brandcolor.enabled': {

Please have a test in app_window_manager_test.js

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450125 [details] [diff] [review]
bug1033364.part2.patch

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

forgetten r+

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

BTW pay attention to follow regressions if any :)

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8449399 [details] [diff] [review]
bug1033364.part1.patch

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

I just noticed the event handler is only in AppWindow.
If you want ActivityWindow/LockScreenWindow/HomescreenWindow to be able to trigger _handle_mozbrowsermetachange,
you should change corresponding classes.

I know that is a nice pattern, but lint disallow me to have array under prototype.
And think about different window cares about different events I think it makes sense to have different event type array for each window.

Comment 16

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

https://github.com/mozilla-b2g/gaia/commit/1101626df2f8ea02816aacdd23509e58885735aa

Comment 17

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> Comment on attachment 8449399 [details] [diff] [review]
> bug1033364.part1.patch
> 
> Review of attachment 8449399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just noticed the event handler is only in AppWindow.
> If you want ActivityWindow/LockScreenWindow/HomescreenWindow to be able to
> trigger _handle_mozbrowsermetachange,
> you should change corresponding classes.
> 

Will do that in a followup. I want to land that asap in order to catch possible regressions - I suspect there will be some even if try is green.

Comment 18

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #12)
> Comment on attachment 8450128 [details] [diff] [review]
> bug1033364.part3.patch
> 
> Review of attachment 8450128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/js/app_window_manager.js
> @@ +270,5 @@
> >              }
> >            }.bind(this)
> > +        },
> > +
> > +        'app-brandcolor.enabled': {
> 
> Please have a test in app_window_manager_test.js

Done.

Comment 19

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> Comment on attachment 8450125 [details] [diff] [review]
> bug1033364.part2.patch
> 
> Review of attachment 8450125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nit.
> BTW don't we want statusbar overlay for popup window?
> 
> ::: apps/system/js/activity_window.js
> @@ +166,5 @@
> >      return '<div class="appWindow activityWindow inline-activity' +
> >              '" id="activity-window-' + _id++ + '">' +
> >              '<div class="screenshot-overlay"></div>' +
> >              '<div class="fade-overlay"></div>' +
> > +            '<div class="statusbar-overlay"></div>' +
> 
> Question is do we want to render the statusbar with the activity color
> 

I think so.

> ::: apps/system/js/attention_screen.js
> @@ +28,5 @@
> >    },
> >  
> > +  get statusHeight() {
> > +    if (this.isVisible() && !this.isFullyVisible()) {
> > +      return this.attentionScreen.getBoundingClientRect().height;
> 
> We had better to record it down to avoid reflow every time access it!
> 

I didn't fix it yet as it only concerns the case where there is an attention screen. I wish we have a new spec for attention screen in 2.1, especially since we need AttentionWindow in order to have the statusbar color correct with attention screen.