Last Comment Bug 695440 - Create sidebar for Highlighter tools
: Create sidebar for Highlighter tools
Status: VERIFIED FIXED
[fixed-in-fx-team][testday-20111125]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Rob Campbell [:rc] (:robcee)
:
:
Mentors:
: 663834 672806 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 12:21 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-11-25 07:46 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch 1 (24.41 KB, patch)
2011-10-25 16:36 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
WIP 2 (29.00 KB, patch)
2011-10-26 14:18 PDT, Rob Campbell [:rc] (:robcee)
dcamp: feedback+
Details | Diff | Splinter Review
WIP 3 (29.75 KB, patch)
2011-10-31 12:18 PDT, Rob Campbell [:rc] (:robcee)
paul: feedback+
Details | Diff | Splinter Review
Sidebar (30.98 KB, patch)
2011-11-01 13:22 PDT, Rob Campbell [:rc] (:robcee)
dao+bmo: review-
dcamp: review+
Details | Diff | Splinter Review
screenshot windows 7 (515.76 KB, image/png)
2011-11-02 10:53 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
screenshot linux ubuntu 10.04 (230.07 KB, image/png)
2011-11-02 11:15 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
Sidebar v2 (32.03 KB, patch)
2011-11-02 11:17 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Sidebar v2.1 (32.01 KB, patch)
2011-11-02 11:27 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Sidebar v2.2 (32.06 KB, patch)
2011-11-02 12:24 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Sidebar v2.5 (31.97 KB, patch)
2011-11-03 05:35 PDT, Rob Campbell [:rc] (:robcee)
dao+bmo: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-10-18 12:21:32 PDT
We need a sidebar to host the various tools comprising the highlighter. That work goes here.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-10-25 16:36:48 PDT
Created attachment 569542 [details] [diff] [review]
WIP patch 1
Comment 2 Paul Rouget [:paul] 2011-10-26 04:46:52 PDT
Comment on attachment 569542 [details] [diff] [review]
WIP patch 1

+#devtools-sidebar-box > toolbarbutton {
+  -moz-appearance: none;
+  min-width: 78px;
+  min-height: 22px;
+  color: hsl(210,30%,85%);
+  text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
+  border: 1px solid hsla(210,8%,5%,.45);
+  border-radius: @toolbarbuttonCornerRadius@;
+  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%)), -moz-linear-gradient(hsla(212,7%,57%,.35), hsla(212,7%,57%,.1)) padding-box;

This is the gradient for the [checked] state (but without the corresponding shadow).
It should be: background: -moz-linear-gradient(hsla(212,7%,57%,.35), hsla(212,7%,57%,.1)) padding-box;

+  box-shadow: 0 1px 0 hsla(210,16%,76%,.15) inset, 0 0 0 1px hsla(210,16%,76%,.15) inset, 0 1px 0 hsla(210,16%,76%,.15);
+}
+
+#devtools-sidebar-box > toolbarbutton:not([checked]):hover:active {
+  border-color: hsla(210,8%,5%,.6);
+  background: -moz-linear-gradient(hsla(220,6%,10%,.3), hsla(212,7%,57%,.15) 65%, hsla(212,7%,57%,.3));
+  box-shadow: 0 0 3px hsla(210,8%,5%,.25) inset, 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 0 hsla(210,16%,76%,.15);
+}
+
+#devtools-sidebar-box > toolbarbutton[checked] {
+  color: hsl(208,100%,60%) !important;
+  border-color: hsla(210,8%,5%,.6);
+  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%)), -moz-linear-gradient(hsla(220,6%,10%,.6), hsla(210,11%,18%,.45) 75%, hsla(210,11%,30%,.4));
+  box-shadow: 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 0 hsla(210,16%,76%,.15);
+}
+
+#devtools-sidebar-box > toolbarbutton[checked]:hover:active {
+  background-color: hsla(210,8%,5%,.2);
+}

There are already definitions for this kind of buttons. Why don't you add these selectors to the existing code?
Comment 3 Rob Campbell [:rc] (:robcee) 2011-10-26 14:18:37 PDT
Created attachment 569791 [details] [diff] [review]
WIP 2

Updated WIP patch.

* Cleaned up the registration and deregistration code for sidebar tools.
* Updated styling for Mac, Windows and Linux (see http://cl.ly/BJD9 for windows screenshot)
* Single-pixel splitter

TODO:

* Enable first-registered tool by default.
* Restore toolstate on tabswitch
* Register a second tool, write a test
* Verify and fix unittests

Further TODOs in separate bugs:

* Differentiate between large and small tools
* Add pretty resizers to the toolbar

And one last note, in my discussion about this being an accordion design, I'm leaning back towards having a toolbar with a deck for placing the tool iframes. Given the height of the toolbars, I think having n-registered toolbars will take up too much vertical space in the sidebar. There is still a fair bit of space in the single toolbar for multiple buttons. I'll be updating the patch accordingly.
Comment 4 Dave Camp (:dcamp) 2011-10-26 16:46:36 PDT
Comment on attachment 569791 [details] [diff] [review]
WIP 2

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

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ -237,0 +254,20 @@
> > +   * Dim or undim a panel by setting or removing a dimmed attribute.
> > +   * @param aState
> > +   *        true = dim, false = undim
> > +   */
NaN more ...

It seems kinda weird to enforce that every sidebar is an iframe, but still force the tool to deal with all this boilerplate.

Can we have tools provide the URL they want to load in tool registration and deal with loading the iframe content for them?
Comment 5 Dave Camp (:dcamp) 2011-10-26 16:47:49 PDT
Comment on attachment 569791 [details] [diff] [review]
WIP 2

Splinter chewed my comment up a bit - I was referring to: 

>+  open: function SI_open(aSelection)
>+  {
>+    if (this.openDocked) {
>+      if (!this.iframeReady) {
>+        this.iframe.setAttribute("src", "chrome://browser/content/csshtmltree.xhtml");
>+        let boundIframeOnLoad = function loadedInitializeIframe() {
Comment 6 Dave Camp (:dcamp) 2011-10-26 17:01:12 PDT
Don't want to block this patch, but I'll say it here so I don't forget:

Even (or especially) after the recent refactoring, StyleInspector.jsm is all boilerplate.  There's almost no style-inspector-specific stuff, it's just 300 lines of registerTool visibility garbage.  Much of that is to support its double life as a popup panel and (now) a sidebar.

After this patch we only use styleinspector in a popup from the web console, right?  Maybe we should:
a) put a simple sidebar-based registerTool implementation into CssHtmlTree.jsm
b) have the web console manually create a panel to contain a CssHtmlTree (like we're doing with the rule view)
c) Ditch StyleInspector.jsm.
d) (Rename CssHtmlTree to CssPropertyView or something)
Comment 7 Dave Camp (:dcamp) 2011-10-26 17:01:57 PDT
And if we don't think panels are good UI, maybe we should ditch panel support in registerTools, and let consumers that want a panel built it themselves.
Comment 8 Dave Camp (:dcamp) 2011-10-27 09:37:49 PDT
*** Bug 672806 has been marked as a duplicate of this bug. ***
Comment 9 Dave Camp (:dcamp) 2011-10-27 09:50:56 PDT
*** Bug 663834 has been marked as a duplicate of this bug. ***
Comment 10 Rob Campbell [:rc] (:robcee) 2011-10-30 11:21:28 PDT
Your suggestions in Comment 6 make a lot of sense. I've been struggling with getting all of the setup boilerplate working in order to get the Style Inspector running in this mode and... it's been a struggle.

Maybe passing the chrome URI for the tool's document makes more sense and let the sidebar handling code deal with all of that setup in a more well-defined way is the way to go.

I was really hoping I could just bolt on about 20 lines of code the way I did with the TreePanel but it's proving to be more difficult than that. I'll talk with you tomorrow about best approaches going forward. Maybe we can cook up a simple example using the Rule View rather than tackling the Style Inspector refactoring all at once.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-10-31 12:18:33 PDT
Created attachment 570789 [details] [diff] [review]
WIP 3

f? for dao on styles, zpao for browser.xul bits.

Still very much a work in progress, but wanted some feedback on styling and browser contact.

There are issues with this. The button.check behavior on the toolbar isn't applying, might need some selector adjustment. Maybe the type=radio is conflicting? I'm not sure but haven't investigated fully.

Activating the Style Inspector in the side bar currently requires pressing the Style button in that toolbar.

Also seeing some very curious sidebar width adjustment when the Inspect button is enabled and sidebar (Styling) is visible.

with all that in mind...
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-31 14:27:05 PDT
Comment on attachment 570789 [details] [diff] [review]
WIP 3

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

It's looking good to me. I ran it and it's much better than that broken popup, quirks and all :)

::: browser/base/content/browser-sets.inc
@@ +145,4 @@
>    <commandset id="inspectorCommands">
>      <command id="Inspector:Inspect"
>               oncommand="InspectorUI.toggleInspection();"/>
> +    <command id="Inspector:ShowStylebar"

Nit: seems like InspectorUI:ToggleStylebar or just InspectorUI:Sidebar would be a better name since it is a toggle
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-01 05:36:41 PDT
Thanks Paul. I'll add your Sidebar command nit to my list. That was bugging me too. :)

Also figured out why the buttons weren't being checked. toolbarbuttons with type="radio" don't seem to like having the .checked property manually fiddled with. Using setAttribute worked well.

New patch forthcoming today.
Comment 14 Dão Gottwald [:dao] 2011-11-01 11:50:42 PDT
Comment on attachment 570789 [details] [diff] [review]
WIP 3

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -956,7 +956,7 @@
>                 style="min-width: 14em; width: 18em; max-width: 36em;"/>
>     </vbox>
> 
>-    <splitter id="sidebar-splitter" class="chromeclass-extrachrome" hidden="true"/>
>+    <splitter id="sidebar-splitter" hidden="true"/>
>     <vbox id="appcontent" flex="1">
>       <tabbrowser id="content" disablehistory="true"
>                   flex="1" contenttooltip="aHTMLTooltip"

What is this change about?

>@@ -967,6 +967,11 @@
>       <statuspanel id="statusbar-display" inactive="true"/>
>     </vbox>
>     <vbox id="browser-border-end" hidden="true" layer="true"/>
>+    <splitter id="devtools-side-splitter" class="chromeclass-extrachrome" hidden="true"/>
>+    <vbox id="devtools-sidebar-box" hidden="true" flex="1">
>+      <toolbar id="devtools-sidebar-toolbar" nowindowdrag="true" />
>+      <deck id="devtools-sidebar-deck" flex="1" />
>+    </vbox>

browser-border-end needs to be at the end.

nit: remove space before />

>+#devtools-sidebar-box > toolbar {

Looks like you can use #devtools-sidebar-toolbar here (and elsewhere).
Comment 15 Rob Campbell [:rc] (:robcee) 2011-11-01 12:13:52 PDT
(In reply to Dão Gottwald [:dao] from comment #14)
> Comment on attachment 570789 [details] [diff] [review] [diff] [details] [review]
> WIP 3
> 
> >--- a/browser/base/content/browser.xul
> >+++ b/browser/base/content/browser.xul
> >@@ -956,7 +956,7 @@
> >                 style="min-width: 14em; width: 18em; max-width: 36em;"/>
> >     </vbox>
> > 
> >-    <splitter id="sidebar-splitter" class="chromeclass-extrachrome" hidden="true"/>
> >+    <splitter id="sidebar-splitter" hidden="true"/>
> >     <vbox id="appcontent" flex="1">
> >       <tabbrowser id="content" disablehistory="true"
> >                   flex="1" contenttooltip="aHTMLTooltip"
> 
> What is this change about?

whoops. That wasn't supposed to be there. Was trying to see what "chromeclass-extrachrome" did.

> 
> >@@ -967,6 +967,11 @@
> >       <statuspanel id="statusbar-display" inactive="true"/>
> >     </vbox>
> >     <vbox id="browser-border-end" hidden="true" layer="true"/>
> >+    <splitter id="devtools-side-splitter" class="chromeclass-extrachrome" hidden="true"/>
> >+    <vbox id="devtools-sidebar-box" hidden="true" flex="1">
> >+      <toolbar id="devtools-sidebar-toolbar" nowindowdrag="true" />
> >+      <deck id="devtools-sidebar-deck" flex="1" />
> >+    </vbox>
> 
> browser-border-end needs to be at the end.

moved.

> nit: remove space before />

Done and done.

> >+#devtools-sidebar-box > toolbar {
> 
> Looks like you can use #devtools-sidebar-toolbar here (and elsewhere).

excellent. Thanks!
Comment 16 Rob Campbell [:rc] (:robcee) 2011-11-01 13:22:52 PDT
Created attachment 571127 [details] [diff] [review]
Sidebar

Passing all tests, added some tests to the initialization test file, addressed Dão's comments.
Comment 17 Dave Camp (:dcamp) 2011-11-01 15:16:32 PDT
Comment on attachment 571127 [details] [diff] [review]
Sidebar

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

IRC conversation indicates that the leftover cleanup XXX comments are just leftover comments, so r+ for the devtools code.

We discussed some label changes on irc: Style Inspector's button should be labeled "Computed" or "Properties" (it will be opposite "Rules" for the rule view), and the sidebar button should probably be labeled "Style".

::: browser/devtools/highlighter/inspector.jsm
@@ +768,5 @@
> +    this.stylingButton.checked = true;
> +
> +    // activate the first tool in the sidebar
> +    let firstButtonId = this.getToolbarButtonId(this.sidebarTools[0].id);
> +    this.chromeDoc.getElementById(firstButtonId).click();

Can you do this only if a tool isn't already selected (if this isn't the first time opening the tool)?

Should probably file a followup to persist the selected tool...

@@ +1025,5 @@
>      this.closing = false;
>      this.isDirty = false;
>  
> +    // XXX todo sidepanel teardown
> +

Is this still todo, or is this a stale comment?

@@ +1769,4 @@
>      if (this.isInspectorOpen) {
>        this.closeInspectorUI();
>      }
> +    // XXX todo tear-down of sidebar tabpanels

Same here?

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ -76,5 @@
>  
>      // Were we invoked from the Highlighter?
>      if (this.IUI) {
> -      this.createPanel(true);
> -

(I was confused about when createPanel() is called if openDocked == false - it's called when HUDService.jsm creates a style inspector, in case any other reviewers are curious).

@@ -237,0 +267,33 @@
> > +   * Dim or undim a panel by setting or removing a dimmed attribute.
> > +   * @param aState
> > +   *        true = dim, false = undim
> > +   */
NaN more ...

Leftover commented-out line.
Comment 18 Dão Gottwald [:dao] 2011-11-02 07:23:38 PDT
Comment on attachment 571127 [details] [diff] [review]
Sidebar

>+++ b/browser/base/content/browser.xul
>@@ -966,6 +966,11 @@
>                   onclick="return contentAreaClick(event, false);"/>
>       <statuspanel id="statusbar-display" inactive="true"/>
>     </vbox>
>+    <splitter id="devtools-side-splitter" class="chromeclass-extrachrome" hidden="true"/>
>+    <vbox id="devtools-sidebar-box" hidden="true" flex="1">
>+      <toolbar id="devtools-sidebar-toolbar" nowindowdrag="true"/>
>+      <deck id="devtools-sidebar-deck" flex="1"/>
>+    </vbox>

I don't think class="chromeclass-extrachrome" makes sense here, as you wouldn't automatically open the side bar in a new window. If it did make sense, you would have to set it on the splitter *and* devtools-sidebar-box.

>+++ b/browser/themes/winstripe/browser/browser.css
>@@ -29,6 +29,7 @@
>  *   Jim Mathies (jmathies@mozilla.com)
>  *   Drew Willcoxon (adw@mozilla.com)
>  *   Paul Rouget (paul@mozilla.com)
>+*    Rob Campbell (rcampbell@mozilla.com)

Indentation is off.

>+#devtools-sidebar-box {
>+  min-width: 212px;
>+}

This should probably use an em value, like the standard sidebar does:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#955

>+#devtools-sidebar-toolbar {
>+  -moz-appearance: none;
>+  padding: 4px 3px 4px 3px;;

padding: 4px 3px; (duplicate semicolon removed)

>+#devtools-side-splitter {
>+  -moz-appearance: none;
>+  border-left: 1px solid hsla(210, 8%,5%,.45);
>+  max-width: 1px;
>+  min-width: 1px;
>+}

This is too thin to interact with. See #sidebar-splitter in browser-aero.css for how to fix this:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser-aero.css#69
Comment 19 Rob Campbell [:rc] (:robcee) 2011-11-02 10:53:22 PDT
Created attachment 571377 [details]
screenshot windows 7

screenshot showing updating splitter styling.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-11-02 11:15:24 PDT
Created attachment 571383 [details]
screenshot linux ubuntu 10.04

linux screenshot. For some reason the styling doesn't seem to be taking effect here. Grippy image is visible despite the background-image: none !important;.

Any suggestions?
Comment 21 Rob Campbell [:rc] (:robcee) 2011-11-02 11:17:41 PDT
Created attachment 571385 [details] [diff] [review]
Sidebar v2

addressed Dão's comments. Still need to figure out how to hide the grippy image on linux. suggestions welcome. Mac and Windows look good.
Comment 22 Rob Campbell [:rc] (:robcee) 2011-11-02 11:27:23 PDT
Created attachment 571388 [details] [diff] [review]
Sidebar v2.1

fixed it with a -moz-appearance: none.
Comment 23 Rob Campbell [:rc] (:robcee) 2011-11-02 12:24:18 PDT
Created attachment 571409 [details] [diff] [review]
Sidebar v2.2

added max-width of 42em for the sidebar. Without it, you can fill the screen.

Why 42? Because the max-width of the regular sidebar is 36, or 18 (min-width) x 2. 42 is 18 * 2 + 6. Or 6 * 7. It seemed like a nice proportion.
Comment 24 Dão Gottwald [:dao] 2011-11-03 01:49:48 PDT
Comment on attachment 571409 [details] [diff] [review]
Sidebar v2.2

>+#devtools-sidebar-box {
>+  min-width: 18em;
>+  max-width: 42em;
>+}

I think this can go straight into browser/base/content/browser.css or browser.xul, à la http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#955

>+#devtools-side-splitter {
>+  border: 0;
>+  -moz-border-end: 1px solid #242b33;
>+  min-width: 0;
>+  width: 3px;
>+  background-color: transparent;
>+  -moz-margin-start: -3px;
>+  position: relative;
>+}

Apparently you copied -moz-border-end and -moz-margin-start from browser-aero.css. However, that sidebar is on the opposite side. The way you're doing this means that the splitter overlaps the content area. I think it's better to overlap the sidebar, i.e. use -moz-border-start and -moz-margin-end.
Comment 25 Rob Campbell [:rc] (:robcee) 2011-11-03 05:22:19 PDT
(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 571409 [details] [diff] [review] [diff] [details] [review]
> Sidebar v2.2
> 
> >+#devtools-sidebar-box {
> >+  min-width: 18em;
> >+  max-width: 42em;
> >+}
> 
> I think this can go straight into browser/base/content/browser.css or
> browser.xul, à la
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> xul#955

wfm! browser.xul it is.

> >+#devtools-side-splitter {
> >+  border: 0;
> >+  -moz-border-end: 1px solid #242b33;
> >+  min-width: 0;
> >+  width: 3px;
> >+  background-color: transparent;
> >+  -moz-margin-start: -3px;
> >+  position: relative;
> >+}
> 
> Apparently you copied -moz-border-end and -moz-margin-start from
> browser-aero.css. However, that sidebar is on the opposite side. The way
> you're doing this means that the splitter overlaps the content area. I think
> it's better to overlap the sidebar, i.e. use -moz-border-start and
> -moz-margin-end.

yeah, I wondered about that. I will flip it.
Comment 26 Rob Campbell [:rc] (:robcee) 2011-11-03 05:35:14 PDT
Created attachment 571610 [details] [diff] [review]
Sidebar v2.5

Updated, moved *widths to browser.xul, flipped start and end in splitter to move grip into sidebar area.

Also added persist="width" to the sidebar-box to store the widget's size.
Comment 27 Dão Gottwald [:dao] 2011-11-03 06:13:47 PDT
Comment on attachment 571610 [details] [diff] [review]
Sidebar v2.5

>+    <vbox id="devtools-sidebar-box" hidden="true" flex="1" 
>+      style="min-width: 18em; width: 22em; max-width: 42em;" persist="width"> 

Add more spaces so that id and style line up.
Comment 28 Rob Campbell [:rc] (:robcee) 2011-11-03 06:40:08 PDT
https://hg.mozilla.org/integration/fx-team/rev/79cb1f3bf7bb
Comment 29 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-03 18:49:11 PDT
https://hg.mozilla.org/mozilla-central/rev/79cb1f3bf7bb
Comment 30 Teodosia Pop 2011-11-25 07:46:01 PST
Verified fixed using
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111124 Firefox/10.0a2
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111123 Firefox/10.0a2

Note You need to log in before you can comment on or make changes to this bug.