UI should be responsive when docked to the side

RESOLVED FIXED in Firefox 27

Status

RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 27

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 817802 [details] [diff] [review]
dbg-responsive.patch
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #817802 - Flags: review?(nfitzgerald)
(Assignee)

Comment 3

5 years ago
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.
Attachment #817802 - Flags: review?(nfitzgerald)
(Can we please have some screensohts :) )
(Assignee)

Comment 5

5 years ago
Created attachment 818473 [details] [diff] [review]
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!
Attachment #817802 - Attachment is obsolete: true
Attachment #818473 - Flags: review?(nfitzgerald)
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.
Attachment #818473 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 7

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/b106a15a50ee
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
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...)

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.