Closed Bug 818151 Opened 8 years ago Closed 8 years ago

[toolbox] adapt the toolbox UI to make it fit on the side of the browser

Categories

(DevTools :: Framework, defect)

x86
All
defect
Not set
normal

Tracking

(firefox22 verified)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox22 --- verified

People

(Reporter: paul, Assigned: miker)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 7 obsolete files)

We have disabled the "dock in the sidebar" feature because our tools and the toolbox look "squashed".

We should implement a responsive UI and then re-enable the pref.
Depends on: 818288
One generic thing that can be done , which will help a lot in this bug and bug 818288 will be to make the sidebar responsive. As soon as the width to height ratio drop a threshold, make the sidebar come below the rest of the content (instead of to the right of the rest of the content). Just like style editor's source editor behave.

Similar thing can be done for the two side panels of debugger. When the panels are hidden, no issues. But when you show them, instead of showing to the left and right of the source editor, show them stacked together, below the editor.
Component: Developer Tools → Developer Tools: Framework
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
No longer depends on: 818288
Duplicate of this bug: 842515
Attached patch Patch (obsolete) — Splinter Review
Ideally we would have used the overflow and underflow events but they just don't work here (they do not get triggered at the right time). Using a resize listener works far better.
Attachment #715981 - Flags: review?(jwalker)
Comment on attachment 715981 [details] [diff] [review]
Patch

I'll take a look too.

Don't turn on the pref in the bug though.
Attachment #715981 - Flags: review?(paul)
Comment on attachment 715981 [details] [diff] [review]
Patch

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

I think this is a good step forward, some things I think we need to do before we can pref-on by default (I don't mind if we do them in this bug or another)
- The font for the menu items should be the same as the font for the normal buttons
- The menu items should have the icons of the normal buttons
- There should be more left padding, and less white border to the menu
- The tab overflow marker used by the Firefox tabbrowser is a single '>' i.e. (chrome://browser/skin/tabbrowser/tab-arrow-right.png) I think it would be good to have a graphic based on that
- When an item in the menu is selected then the overflow '>' should have the overline highlight, and really it would be good to have the current menuitem having a similar highlight (but probably on the RHS?)

::: browser/devtools/framework/Toolbox.jsm
@@ +257,5 @@
>          iframe.removeEventListener("DOMContentLoaded", domReady, true);
>  
>          this.isReady = true;
>  
> +        this.closeButton = this.doc.getElementById("toolbox-close");

this._closeButton maybe ?

@@ +419,4 @@
>        this.selectTool(id);
> +    }.bind(this, id);
> +
> +    radio.addEventListener("command", func);

Perhaps we can find a better function name than func?

@@ +735,5 @@
>  
>      gDevTools.off("tool-registered", this._toolRegistered);
>      gDevTools.off("tool-unregistered", this._toolUnregistered);
>  
> +    this.frame.removeEventListener("resize", this._tabBarRefresh, false);

I've not looked into the flow, but we're removing in 2 places. Is that right?

::: browser/devtools/framework/toolbox.css
@@ +21,5 @@
> +#toolbox-tab-dropdown,
> +#toolbox-tab-dropdown > .button-box,
> +#toolbox-tab-dropdown > menupopup,
> +#toolbox-tab-dropdown > menupopup menuitem {
> +  -moz-appearance: none;

I think this should be theme CSS?
https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS
Attachment #715981 - Flags: review?(jwalker) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #5)
> Comment on attachment 715981 [details] [diff] [review]
> Patch
> 
> Review of attachment 715981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is a good step forward, some things I think we need to do
> before we can pref-on by default (I don't mind if we do them in this bug or
> another)
> - The font for the menu items should be the same as the font for the normal
> buttons
> - The menu items should have the icons of the normal buttons
> - There should be more left padding, and less white border to the menu
> - The tab overflow marker used by the Firefox tabbrowser is a single '>'
> i.e. (chrome://browser/skin/tabbrowser/tab-arrow-right.png) I think it would
> be good to have a graphic based on that
> - When an item in the menu is selected then the overflow '>' should have the
> overline highlight, and really it would be good to have the current menuitem
> having a similar highlight (but probably on the RHS?)
> 

Most of the tools also need a narrow view via media queries.

> ::: browser/devtools/framework/Toolbox.jsm
> @@ +257,5 @@
> >          iframe.removeEventListener("DOMContentLoaded", domReady, true);
> >  
> >          this.isReady = true;
> >  
> > +        this.closeButton = this.doc.getElementById("toolbox-close");
> 
> this._closeButton maybe ?
> 

Done

> @@ +419,4 @@
> >        this.selectTool(id);
> > +    }.bind(this, id);
> > +
> > +    radio.addEventListener("command", func);
> 
> Perhaps we can find a better function name than func?
> 

Actually, we don't even need a new function there. Removed.

> @@ +735,5 @@
> >  
> >      gDevTools.off("tool-registered", this._toolRegistered);
> >      gDevTools.off("tool-unregistered", this._toolUnregistered);
> >  
> > +    this.frame.removeEventListener("resize", this._tabBarRefresh, false);
> 
> I've not looked into the flow, but we're removing in 2 places. Is that right?
> 

This is because the resize listener is added to this.frame. switchHost creates a new frame so the listener is first removed from the old frame and then added to the new one. I have added a comment to clarify this.

> ::: browser/devtools/framework/toolbox.css
> @@ +21,5 @@
> > +#toolbox-tab-dropdown,
> > +#toolbox-tab-dropdown > .button-box,
> > +#toolbox-tab-dropdown > menupopup,
> > +#toolbox-tab-dropdown > menupopup menuitem {
> > +  -moz-appearance: none;
> 
> I think this should be theme CSS?
> https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS

moz-appearance being theme css surprises me as for any styling to be possible it needs to be set to none. Moved.

So in a nutshell:
- The font for the menu items should be the same as the font for the normal buttons on all OSes.
- The menu items should have the icons of the normal buttons
- There should be more left padding, and less white border to the menu
- The tab overflow marker used by the Firefox tabbrowser is a single '>'
  i.e. (chrome://browser/skin/tabbrowser/tab-arrow-right.png) I think it would be good to have a graphic based on that
- When an item in the menu is selected then the overflow '>' should have the overline highlight, and really it would be good to have the current menuitem having a similar highlight (but probably on the RHS?)
- Most of the tools also need a narrow view via media queries.
Attachment #715981 - Attachment is obsolete: true
Attachment #715981 - Flags: review?(paul)
Attachment #716478 - Flags: review?(paul)
Attached image Sidebar Linux (obsolete) —
Attached image Sidebar Mac (obsolete) —
We should refresh the tabs onLoad instead of onDOMLoad as otherwise they are missed.

Shorlander, The current look of a narrower version of the toolbax (for docking on the right of the screen) is attached.

We figure that maybe this needs some UX love ... what do you think?
Whiteboard: [ux-feedback-needed]
Comment on attachment 716478 [details] [diff] [review]
Patch v2

(removing review until ux comes up with a design)
Attachment #716478 - Flags: review?(paul)
I believe we should either fold all the tabs into a single menu, or not fold them at all (but we're still waiting for UX feedback)
Shorlander suggested that we just hide the labels on overflow. About only having icons, he said:

"Labels are good, however I am not too worried about dropping them for space savings in this case.

People learn the meanings of ideograms either through prior experience/convention or by active association with a real world thing or label. In this case we do have labels in the default state where things are docked to the bottom. A user would have to start in that state and actively move the toolbar to the side.

I don't find this any different than all of the other places we use just icons and no label.
                 
I would shrink the tabs down to their smallest size (icon) and if there is still overflow use the chevron and list approach but also include the icon in the list with the tool name."
Whiteboard: [ux-feedback-needed]
Blocks: 846371
For the moment we have decided to show only icons when docked to the side and show the full tabs when docked to the bottom.
Attached patch Patch v3 (obsolete) — Splinter Review
Very simple.
Attachment #716478 - Attachment is obsolete: true
Attachment #716630 - Attachment is obsolete: true
Attachment #716631 - Attachment is obsolete: true
Attachment #722399 - Flags: review?(paul)
Attached patch Patch v4 (obsolete) — Splinter Review
Removed pointless assignment.
Attachment #722399 - Attachment is obsolete: true
Attachment #722399 - Flags: review?(paul)
Attachment #722711 - Flags: review?(paul)
Whiteboard: [has-patch]
Comment on attachment 722711 [details] [diff] [review]
Patch v4

On OSX:

> label is undefined
> Toolbox.jsm:431
Attachment #722711 - Flags: review?(paul)
Attached patch patch v5 (obsolete) — Splinter Review
This works well.

Mihai and Benvie need to test this on Windows and Linux before you review it.
Attachment #722711 - Attachment is obsolete: true
Attachment #724605 - Flags: review?(paul)
Attachment #724605 - Flags: feedback?(mihai.sucan)
Attachment #724605 - Flags: feedback?(bbenvie)
Comment on attachment 724605 [details] [diff] [review]
patch v5

>--- a/browser/themes/linux/devtools/toolbox.css
> .devtools-tab {
>   -moz-appearance: none;
>   min-width: 88px;
>-  min-height: 32px;
>+  min-height: 47px;
>+  max-width: 137px;
>   color: #b6babf;
>   margin: 0;
>   padding: 0;
>   background-image: linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
>                     linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1));
>   background-size: 1px 100%;
>   background-repeat: no-repeat;
>   background-position: left, right;
>   border-right: 1px solid hsla(206,37%,4%,.45);
>+  width: 1px;
> }

width:1px ?
Attachment #724605 - Flags: feedback?(bbenvie) → feedback+
Attachment #724605 - Flags: feedback?(mihai.sucan)
Attached patch patch v47Splinter Review
Added min-width to side panel
Attachment #724605 - Attachment is obsolete: true
Attachment #724605 - Flags: review?(paul)
Attachment #724660 - Flags: review?(paul)
Comment on attachment 724660 [details] [diff] [review]
patch v47

Let's land that!
Attachment #724660 - Flags: review?(paul) → review+
Whiteboard: [has-patch] → [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/10e454c16406
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Blocks: 856814
Verified as fixed on Firefox 22 beta 2: -the toolbox UI fits on the side of the browser.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0
Build ID: 20130521223249
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.