994669 - Integrate gaia responsive mode tweaks into Firefox Mulet

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Alexandre Poirot [:ochameau]]

A bunch of tweaks are being made from an addon hosted in gaia repo.
We need to uplift those to m-c so that the mulet can have the same cool features. (phone design, hardware buttons, rotation, ...)

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Integrate gaia responsive mode tweaks

paul, any feedback on these hacks?

For example, we may apply the "device" skin to the responsive mode itself.
It would make this code way less hacky!

Also, we may tweak the responsive mode code to make it easier to integrate it with shell.html...

Comment 2

[Paul Rouget [:paul]]

Comment on attachment 8404698 [details] [diff] [review]
Integrate gaia responsive mode tweaks

The CSS part will need a good clean up. Any reason to not move the b2g/browser.css code into responsivedesign.inc.css?

And to insert a stylesheet, you might want to use nsIDOMWindowUtils.loadSheet().

The JS code is basically monkeypatching the responsive mode.

Can we merge this code inside the responsive mode code? Basically, make the responsive mode aware of the gaia-debugging mode?

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Integrate gaia responsive mode tweaks r=paul

Merged the code into responsive mode instead of monkey patching.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8469615 [details] [diff] [review]
Integrate gaia responsive mode tweaks r=paul

This patch doesn't necessary help the simulator scenario,
we may keep shell.html as top level window and the existing hardware buttons for the first mulet-based releases.
But it allows to drop these hacks from gaia repo!

Comment 5

[Paul Rouget [:paul]]

Alex, what's the process to test that?

Comment 6

[Alexandre Poirot [:ochameau]]

Build mulet (ac_add_options --enable-application=b2g/dev)
Build a regular gaia profile ( ~gaia $ make )
Launch mulet with -profile path/to/gaia/profile
Open chrome://b2g/content/shell.html (Mulet used to open that automatically, but doesn't do it anymore)

The responsive mode should automatically be opened, with FxOS tweaks. A toolbox should also be opened.
The responsive mode shouldn't be special when opened against any other webpage.

Comment 7

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=a5785b1fafed

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 8469615 [details] [diff] [review]
Integrate gaia responsive mode tweaks r=paul

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

The home button is always present (with any page).
Also, after loading b2g, starting the responsive mode adds all the fxos controls.

::: b2g/chrome/content/browser.css
@@ +1,1 @@
> +

Where is this file used?

@@ +16,5 @@
> +  border: 1px solid #FFFFFF;
> +  border-bottom-width: 0;
> +  
> +  background-color: #353535;
> +  

Trailing whitespaces. In #os-hardware-buttons too.

@@ +19,5 @@
> +  background-color: #353535;
> +  
> +  box-shadow: 0 3px 0.7px 1px #777777, 0 5px rgba(0, 0, 0, 0.4) inset !important;
> +
> +  background-image: -moz-linear-gradient(right, #111 11%, #333 56%);

Don't use -moz-. More below.

::: b2g/chrome/content/desktop.js
@@ +108,5 @@
> +  mgr.handleGcliCommand(browserWindow,
> +                        browserWindow.gBrowser.selectedTab,
> +                        'resize to',
> +                        args);
> +}

Don't use handeGcliCommand. Use setSize().

@@ +113,5 @@
> +
> +function openDevtools() {
> +  // Open devtool panel while maximizing its size according to screen size
> +  Services.prefs.setIntPref('devtools.toolbox.sidebar.width',
> +                            browserWindow.outerWidth - 550);

You forgot to force sidebar mode.

::: b2g/chrome/content/screen.js
@@ +5,5 @@
>  // TODO: support multiple device pixels per CSS pixel
>  // 
>  
> +let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +let isMulet = "ResponsiveUI" in browserWindow;

Don't you have a more solid way to know if we're in "mulet mode"?

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +461,5 @@
>      this.stack.appendChild(this.resizer);
>      this.stack.appendChild(this.resizeBarV);
>      this.stack.appendChild(this.resizeBarH);
> +
> +    // FxOS custom controls

Not sure to understand. Aren't you supposed to build these only if we're in FxOS mode?

::: browser/themes/shared/devtools/responsivedesign.inc.css
@@ +224,5 @@
> +}
> +
> +/* FxOS custom mode with additional buttons and phone look'n feel */
> +
> +/* Hide devtools manual resizer */

Don't you also want to hide the dropdown menu?

@@ +233,5 @@
> +}
> +
> +/* Gives responsive mode a phone look'n feel */
> +.browserStack.fxos-mode {
> +  padding: 60px 15px 0;

To these 4 selectors, add [responsivemode]. Otherwise, you'll run into specificity issues.

@@ +243,5 @@
> +  border-bottom-width: 0;
> +
> +  background-color: #353535;
> +
> +  box-shadow: 0 3px 0.7px 1px #777777, 0 5px rgba(0, 0, 0, 0.4) inset !important;

Like here. Remove !important.

@@ +245,5 @@
> +  background-color: #353535;
> +
> +  box-shadow: 0 3px 0.7px 1px #777777, 0 5px rgba(0, 0, 0, 0.4) inset !important;
> +
> +  background-image: -moz-linear-gradient(right, #111 11%, #333 56%);

Remove -moz-. Use "to right". For all the below linear gradients too.

::: browser/themes/windows/jar.mn
@@ +371,5 @@
>          skin/classic/browser/devtools/font-inspector.css            (../shared/devtools/font-inspector.css)
>          skin/classic/browser/devtools/computedview.css              (../shared/devtools/computedview.css)
>          skin/classic/browser/devtools/arrow-e.png                   (../shared/devtools/images/arrow-e.png)
>          skin/classic/browser/devtools/[e-mail]                (../shared/devtools/images/[e-mail])
> +        skin/classic/browser/devtools/responsiveui-home.png         (../shared/devtools/responsiveui-home.png)

You missed the aero part.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #8)
> Comment on attachment 8469615 [details] [diff] [review]
> ::: b2g/chrome/content/browser.css
> @@ +1,1 @@
> > +
> 
> Where is this file used?

Nowhere, it was a left over.

> 
> ::: b2g/chrome/content/desktop.js
> @@ +108,5 @@
> > +  mgr.handleGcliCommand(browserWindow,
> > +                        browserWindow.gBrowser.selectedTab,
> > +                        'resize to',
> > +                        args);
> > +}
> 
> Don't use handeGcliCommand. Use setSize().

handleGcLiCommand has the advantage to be on the global ResponsiveUIManager.
But I tweaked that to call mgr.toggle() then responsive.setSize().

> 
> @@ +113,5 @@
> > +
> > +function openDevtools() {
> > +  // Open devtool panel while maximizing its size according to screen size
> > +  Services.prefs.setIntPref('devtools.toolbox.sidebar.width',
> > +                            browserWindow.outerWidth - 550);
> 
> You forgot to force sidebar mode.

That was intentional, but I think it is better to force it.
My issue was that it will reset the toolbox to sidebar every time your launch firefox.

> 
> ::: b2g/chrome/content/screen.js
> @@ +5,5 @@
> >  // TODO: support multiple device pixels per CSS pixel
> >  // 
> >  
> > +let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> > +let isMulet = "ResponsiveUI" in browserWindow;
> 
> Don't you have a more solid way to know if we're in "mulet mode"?

In JS, not really. I can check for some pref like the systemapp url one,
but it doesn't sound much more solid either.
There is nothing in appinfo that can help identifying the mulet.
I talked with jgriffin about renaming the whole product to Mulet,
that would change the app name in appinfo, but would also
modify the binary name and require a bunch of coordination to tweak releng scripts :/

> 
> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +461,5 @@
> >      this.stack.appendChild(this.resizer);
> >      this.stack.appendChild(this.resizeBarV);
> >      this.stack.appendChild(this.resizeBarH);
> > +
> > +    // FxOS custom controls
> 
> Not sure to understand. Aren't you supposed to build these only if we're in
> FxOS mode?

I was thinking about always build the DOM and hiding them via a CSS class...
But you are right it sounds better to only add it on mulet, I tweaked the code to do that.

> 
> ::: browser/themes/shared/devtools/responsivedesign.inc.css
> @@ +224,5 @@
> > +}
> > +
> > +/* FxOS custom mode with additional buttons and phone look'n feel */
> > +
> > +/* Hide devtools manual resizer */
> 
> Don't you also want to hide the dropdown menu?
> 

It may help people using mulet to run TV/tablet builds.

Comment 10

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=4f761de89790

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 8470939 [details] [diff] [review]
patch

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

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +295,5 @@
>  
>      // Removed elements.
>      this.container.removeChild(this.toolbar);
> +    if (this.bottomToolbar) {
> +      this.container.removeChild(this.bottomToolbar);

this.bottomToolbar.remove();

::: browser/themes/shared/devtools/responsivedesign.inc.css
@@ +219,5 @@
>    cursor: se-resize;
>    transform: translate(12px, 12px);
>    background-size: cover;
>    background-image: url("chrome://browser/skin/devtools/responsive-se-resizer.png");
> +  display:none;

Why?

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch

> ::: browser/themes/shared/devtools/responsivedesign.inc.css
> @@ +219,5 @@
> >    cursor: se-resize;
> >    transform: translate(12px, 12px);
> >    background-size: cover;
> >    background-image: url("chrome://browser/skin/devtools/responsive-se-resizer.png");
> > +  display:none;
> 
> Why?

I don't know! That scares me to submit patches with such things...

https://tbpl.mozilla.org/?tree=Try&rev=1d72bca75983

I also removed some other leftovers in b2g/ ...

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cc73a5e460b0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc73a5e460b0

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8471465 [details] [diff] [review]
patch

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

I know it's probably too late do things here, but I've noticed some problems in the CSS. (You can probably file a follow up)

::: browser/themes/shared/devtools/responsivedesign.inc.css
@@ +244,5 @@
> +  background-color: #353535;
> +
> +  box-shadow: 0 3px 0.7px 1px #777777, 0 5px rgba(0, 0, 0, 0.4) inset;
> +
> +  background-image: linear-gradient(to right, #111 11%, #333 56%);

In you previous patch, it was -moz-linear-gradient(right, #111 11%, #33 56%);

If you convert it to the W3C syntax, it should be to left, not to right. So right now, your background is actually backwards. This applies for all the other gradients I guess.

::: browser/themes/windows/jar.mn
@@ +375,5 @@
>          skin/classic/browser/devtools/font-inspector.css            (../shared/devtools/font-inspector.css)
>          skin/classic/browser/devtools/computedview.css              (../shared/devtools/computedview.css)
>          skin/classic/browser/devtools/arrow-e.png                   (../shared/devtools/images/arrow-e.png)
>          skin/classic/browser/devtools/[e-mail]                (../shared/devtools/images/[e-mail])
> +        skin/classic/browser/devtools/responsiveui-home.png         (../shared/devtools/responsiveui-home.png)

All the responsiveui files are inside browser/themes/shared/devtools/images/responsivemode/ folder. Right now, the file feels a bit lonely here.