927375 - UI should be responsive when docked to the side

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: dbg-responsive.patch

Comment 2

[Victor Porof [:vporof][:vp]]

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

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 817802 [details] [diff] [review]
dbg-responsive.patch

You know what, I'm going to experiment with different arrangements a bit more. Like having the sources and instruments panes together, with only the editor on top.

Comment 4

[Girish Sharma [:Optimizer]]

(Can we please have some screensohts :) )

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: dbg-responsive.patch

Ok, this is much better. In side hosts, the layout is 1 + 2, meaning one editor on top, and the two panes underneath laid out horizontally. Also comes with a test!

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 818473 [details] [diff] [review]
dbg-responsive.patch

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

LGTM w/ nits addressed!

::: browser/devtools/debugger/debugger-controller.js
@@ +1905,5 @@
>    "gTarget": {
>      get: function() DebuggerController._target
>    },
> +  "gHostType": {
> +    get: function() DebuggerView._hostType

Perhaps _hostType should be public if we're referencing it outside of DebuggerView? Or maybe DebuggerView should expose a getter, if you aren't comfortable with that?

::: browser/devtools/debugger/debugger-panel.js
@@ +90,5 @@
>      return this._controller.Breakpoints.removeBreakpoint(aLocation);
>    },
>  
> +  handleHostChanged: function() {
> +    this._view._handleHostChanged(this._toolbox.hostType);

Again, if an instance's private methods are going to be called from outside of that instance's methods, those private methods should probably just be public.

::: browser/devtools/debugger/debugger-view.js
@@ -117,5 @@
>      this.showEditor = this.showEditor.bind(this);
>      this.showBlackBoxMessage = this.showBlackBoxMessage.bind(this);
>      this.showProgressBar = this.showProgressBar.bind(this);
>      this.maybeShowBlackBoxMessage = this.maybeShowBlackBoxMessage.bind(this);
> -    this._editorDeck = document.getElementById("editor-deck");

Woops, sorry about putting this in the wrong place.

@@ +534,5 @@
> +    let normContainer = document.getElementById("debugger-widgets");
> +    let vertContainer = document.getElementById("vertical-layout-panes-container");
> +    let newLayout = "";
> +
> +    if (aType == "side") {

I wouldn't mind splitting out the contents of each of these branches into `_enterVerticalLayout` and `_enterHorizontalLayour` helper methods. The method is just getting a little long, and I think it would be nice to keep things smaller.

::: browser/devtools/debugger/test/browser_dbg_host-layout.js
@@ +18,5 @@
> +    finish();
> +  });
> +}
> +
> +function testHosts(aHostTypes, aLayoutTypes) {

If aHostTypes is always going to be of length 3, we should just destructure it to [firstType, secondType, thirdType] so that it is self documenting, instead of just having this implicit. Ditto for aLayoutTypes.

@@ +47,5 @@
> +
> +    yield testHost(aTab, aDebuggee, aPanel, aHostType, aLayoutType);
> +  });
> +
> +  function once(aTarget, aEvent) {

Don't we already have a "once" function in head.js? Can we reuse or extend that? If not, cool, but it would be nice if we could.

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +1905,5 @@
> >    "gTarget": {
> >      get: function() DebuggerController._target
> >    },
> > +  "gHostType": {
> > +    get: function() DebuggerView._hostType
> 
> Perhaps _hostType should be public if we're referencing it outside of
> DebuggerView? Or maybe DebuggerView should expose a getter, if you aren't
> comfortable with that?
> 

One shouldn't get the feeling that assigning a new value to _hostType does something, thus the getter. I added the gHostType getter on the window instead of DebuggerView, because it's something assigned outside the tools's iframe window context, added by the panel on initialization, and it's nice to have such things clearly distinguished. We do the same thing for _target, so I went for symmetry.

> ::: browser/devtools/debugger/debugger-panel.js
> @@ +90,5 @@
> >      return this._controller.Breakpoints.removeBreakpoint(aLocation);
> >    },
> >  
> > +  handleHostChanged: function() {
> > +    this._view._handleHostChanged(this._toolbox.hostType);
> 
> Again, if an instance's private methods are going to be called from outside
> of that instance's methods, those private methods should probably just be
> public.
> 

In this case, the underscore is more of a "you probably shouldn't call this manually, it's invoked from the outside when needed" than marking privacy, but I don't feel strongly about it.

> @@ +534,5 @@
> > +    let normContainer = document.getElementById("debugger-widgets");
> > +    let vertContainer = document.getElementById("vertical-layout-panes-container");
> > +    let newLayout = "";
> > +
> > +    if (aType == "side") {
> 
> I wouldn't mind splitting out the contents of each of these branches into
> `_enterVerticalLayout` and `_enterHorizontalLayour` helper methods. The
> method is just getting a little long, and I think it would be nice to keep
> things smaller.
> 

Yup, agreed.

> ::: browser/devtools/debugger/test/browser_dbg_host-layout.js
> @@ +18,5 @@
> > +    finish();
> > +  });
> > +}
> > +
> > +function testHosts(aHostTypes, aLayoutTypes) {
> 
> If aHostTypes is always going to be of length 3, we should just destructure
> it to [firstType, secondType, thirdType] so that it is self documenting,
> instead of just having this implicit. Ditto for aLayoutTypes.
> 

Sure.

> @@ +47,5 @@
> > +
> > +    yield testHost(aTab, aDebuggee, aPanel, aHostType, aLayoutType);
> > +  });
> > +
> > +  function once(aTarget, aEvent) {
> 
> Don't we already have a "once" function in head.js? Can we reuse or extend
> that? If not, cool, but it would be nice if we could.

Yeah, the problem is that the panel window is both an EventEmitter and a DOM node, thus both on/off and addEventListener and removeEventListener are valid for "once". Since the function in head.js checks for the DOM methods first, and defaults to them, I added this small overwrite here.

Comment 8

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/b106a15a50ee

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[mcdavis941 (sporadically reading bugmail)]

Horizontal layout:

  +------------------------+
  |                        |
  |           1            |    1 = inspected page
  |                        |
  +------------------------+
  |           2            |    2 = devtools toolbar + debugger toolbar
  +-----+------------+-----+
  |     |            |     |    3 = debugger sources list
  |  3  |     4      |  5  |    4 = debugger source pane
  |     |            |     |    5 = debugger instruments pane
  +-----+------------+-----+

Vertical layout, i.e., when docked to the side:

  +--------+---------------+
  |        |       2       |    1 = inspected page
  |        +---------------+    2 = devtools toolbar + debugger toolbar
  |        |               |    3 = debugger sources list
  |        |       4       |    4 = debugger source pane
  |   1    |               |    5 = debugger instruments pane
  |        +---------+-----+
  |        |         |     |
  |        |   3     |  5  |
  |        |         |     |
  +--------+---------+-----+

(For the historical record...)