862294 - Sidebar should be responsive in docked Toolbox.

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

This should be an inbuilt feature of the sidebar. When toolbox is docked to side, sidebar should magically stop being sidebar and should become bottombar. (parity chrome)

I was able to do it locally, but it required the whole Inspector's xul change.

Investigating JS options now.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch 0.1

Simple way to achieve the goal.
I think it would even work without the xul change.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Some other cursor fixes for the splitter.

The patch makes inspector and web console responsive.

Comment 3

[Girish Sharma [:Optimizer]]

Comment on attachment 737953 [details] [diff] [review]
patch v0.2

I tried the same approach on Netmonitor, but it was not working out due to following reasons:

1) The sidebar is hidden using translate-x, which makes the sidebar appear when it is just shifted to bottom.
2) The netmonitor hides anything that overflows horizontally, thus the sidebar also gets hidden when it is at bottom.

As for profiler and debugger, they are using sidemenuwidget (or will use), and iirc Victor had plans to make it responsive. Also, debugger has 3 areas. 3 vertically stacked panes do not fit well in vertical spaces that current devices have (on an average 900 px)

Screencast showing Inspector : http://www.youtube.com/watch?v=WLzkEJ9c0d4

(Sorry Joe, no one came forward to review this when I asked on the channel ;) )

Comment 4

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

(In reply to Girish Sharma [:Optimizer] from comment #3)
> Comment on attachment 737953 [details] [diff] [review]
> patch v0.2
> 
> I tried the same approach on Netmonitor, but it was not working out due to
> following reasons:
> 
> 1) The sidebar is hidden using translate-x, which makes the sidebar appear
> when it is just shifted to bottom.

It's done with margins (not important).
Maybe on a @media (max-width: 650px) the sidebar should just always be visible? I can't imagine any reason to hide it. You can add a resize listener on the network monitor window and remove all the magic (read: margins) associated with the sidebar when the width is small enough. I can't image why would anyone want to hide the sidebar when the UI is laid vertically. So just ViewHelpers.togglePane({ visible: true }) (see DV__toggleDetailsPane) and then nothing would be special with the panel; it should behave just like what the inspector and webconsole have.


> 2) The netmonitor hides anything that overflows horizontally, thus the
> sidebar also gets hidden when it is at bottom.
> 

I can't follow you here.

The netmonitor doesn't have a global overflow:hidden. In fact, I looked around for overflow rules in the css and nothing is hidden when stuff overflows. The timeline is also in a different box (and under different parents) than the sidebar, so I can't imagine how it would interfere.

> As for profiler and debugger, they are using sidemenuwidget (or will use),
> and iirc Victor had plans to make it responsive. Also, debugger has 3 areas.
> 3 vertically stacked panes do not fit well in vertical spaces that current
> devices have (on an average 900 px)
> 

Debugger is already using the SideMenuWidget, profiler isn't yet. I don't think I ever mentioned that the widget should be in/by itself responsive. The responsiveness of any kind of sidebar shouldn't be implemented differently from what you have here, from what I can tell, so what were the issues you encountered?

In any case, I'm not sure if you should follow the style editor here and put the sources list on the top. It sounds like a good idea to me, but somehow it feels like it'd waste a ton of space. You decide (maybe do what's easier).

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3 including netmonitor

Shifting to Victor for review.
Added netmonitor changes requested by Victor. Makes it responsive too.

Maybe Debugger and Profiler can be done in a followup?

Comment 6

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

Comment on attachment 738117 [details] [diff] [review]
patch v0.3 including netmonitor

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

Nice. r+ with comments addressed.

::: browser/themes/linux/devtools/netmonitor.css
@@ -214,5 @@
>  /* Network request details */
>  
>  #details-pane {
>    background: hsl(208,11%,27%);
> -  max-width: 500px;

Don't remove this. Instead, do max-width: none; inside the media query below.

@@ +299,5 @@
> +
> +/* Responsive sidebar */
> +@media (max-width: 650px) {
> +  #details-pane {
> +     margin: 0 !important;

One space too many indenting.

@@ +301,5 @@
> +@media (max-width: 650px) {
> +  #details-pane {
> +     margin: 0 !important;
> +     /* To prevent all the margin hacks to hide the sidebar */
> +  }

Add a max-height: 85vh; here to avoid having the details pane cover the whole requests menu.

On second thought, you should add this max-height inside common.inc.css to avoid this problem across all tools. Resizing the side panel to fill out all the available vertical space shouldn't be possible.

@@ +304,5 @@
> +     /* To prevent all the margin hacks to hide the sidebar */
> +  }
> +
> +  .requests-menu-waterfall,
> +  #details-pane-toggle {

Nit: id first, class second.

@@ +305,5 @@
> +  }
> +
> +  .requests-menu-waterfall,
> +  #details-pane-toggle {
> +    display: none;

Move this into content css.

@@ +310,5 @@
> +  }
> +
> +  .requests-menu-size {
> +    border-width: 0px !important;
> +    box-shadow: none !important;

Document the !important please if are they really necessary.

::: browser/themes/shared/devtools/common.inc.css
@@ +89,5 @@
> +.devtools-responsive-container {
> +  -moz-box-orient: horizontal;
> +}
> +
> +@media (max-width: 650px) {

Use %define and reuse the variable across all css files where necessary.
Nit: I think 700px would be better.

@@ +95,5 @@
> +    -moz-box-orient: vertical;
> +  }
> +
> +  .devtools-responsive-container > .devtools-side-splitter {
> +    border: 0;

I think the border is already 0, isn't it?

@@ +96,5 @@
> +  }
> +
> +  .devtools-responsive-container > .devtools-side-splitter {
> +    border: 0;
> +    margin: 0;

Does this margin really need to be 0? You probably don't need this.

@@ +97,5 @@
> +
> +  .devtools-responsive-container > .devtools-side-splitter {
> +    border: 0;
> +    margin: 0;
> +    border-top: 1px solid #242b33;

Use border-top: 1px solid black; for this to be the same as a .devtools-horizontal-splitter.

@@ +98,5 @@
> +  .devtools-responsive-container > .devtools-side-splitter {
> +    border: 0;
> +    margin: 0;
> +    border-top: 1px solid #242b33;
> +    min-height: 0;

This should probably be min-height 3px;, no?

@@ +106,5 @@
> +    cursor: n-resize;
> +  }
> +
> +  .devtools-responsive-container > .devtools-sidebar-tabs {
> +    min-height: 300px;

Use 35vh instead.

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #6)
> @@ +299,5 @@
> > +
> > +/* Responsive sidebar */
> > +@media (max-width: 650px) {
> > +  #details-pane {
> > +     margin: 0 !important;
> 
> One space too many indenting.

Whoops.

> @@ +301,5 @@
> > +@media (max-width: 650px) {
> > +  #details-pane {
> > +     margin: 0 !important;
> > +     /* To prevent all the margin hacks to hide the sidebar */
> > +  }
> 
> Add a max-height: 85vh; here to avoid having the details pane cover the
> whole requests menu.
> 
> On second thought, you should add this max-height inside common.inc.css to
> avoid this problem across all tools. Resizing the side panel to fill out all
> the available vertical space shouldn't be possible.

True. will do.

> @@ +304,5 @@
> > +     /* To prevent all the margin hacks to hide the sidebar */
> > +  }
> > +
> > +  .requests-menu-waterfall,
> > +  #details-pane-toggle {
> 
> Nit: id first, class second.

haha. okay.

> @@ +95,5 @@
> > +    -moz-box-orient: vertical;
> > +  }
> > +
> > +  .devtools-responsive-container > .devtools-side-splitter {
> > +    border: 0;
> 
> I think the border is already 0, isn't it?

no, in side-splitter, the border-start was 1px. This border: 0 clears that so as to apply the border-top in the below lines.

> @@ +96,5 @@
> > +  }
> > +
> > +  .devtools-responsive-container > .devtools-side-splitter {
> > +    border: 0;
> > +    margin: 0;
> 
> Does this margin really need to be 0? You probably don't need this.

Same as above. Side splitter had margin start and end, which needs to be cleared before applying margin top and bottom.

Agreeing to rest all comments.

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #6)
::: browser/themes/shared/devtools/common.inc.css
@@ +89,5 @@
> > +.devtools-responsive-container {
> > +  -moz-box-orient: horizontal;
> > +}
> > +
> > +@media (max-width: 650px) {
>
> Use %define and reuse the variable across all css files where necessary.
> Nit: I think 700px would be better.

On second thought, This would not be of any use as this 650px is used in 5 files, and the variables would have to be defined in all of them. So there is no reusing. I am just making the 650 to 700.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Addressed review comments.

I made the sidebar's max-height as 75vh instead of 85 as 85 was too much. For ex., in net monitor, only 3 request lines were visible.

Comment 10

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

Comment on attachment 738331 [details] [diff] [review]
patch v0.4

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

::: browser/themes/linux/devtools/netmonitor.css
@@ +308,5 @@
> +
> +  .requests-menu-size {
> +    border-width: 0px !important;
> +    box-shadow: none !important;
> +    /* !important are required here to overwrite the style applied in line 31 */

"line 31" will likely change in the future and I doubt anyone's going to keep this comment in sync, maybe say "to the last visible child in the toolbar" instead?

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4.1

Comment changed. to "!important are required here because Timeline is not visible and thus the right border and box-shadow of Size column should be hidden"

Comment 12

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

(In reply to Girish Sharma [:Optimizer] from comment #11)

Much better.

Comment 13

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

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

Comment 14

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/3af1161f4214