Last Comment Bug 666650 - [highlighter] create a global toolbar for the highlighter
: [highlighter] create a global toolbar for the highlighter
Status: VERIFIED FIXED
[minotaur]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 8
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
Depends on: 660606
Blocks: 650794 663778 663830 672006
  Show dependency treegraph
 
Reported: 2011-06-23 10:58 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-10-07 02:47 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
toolbar (5.90 KB, patch)
2011-06-29 07:38 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
toolbar 2 (5.33 KB, patch)
2011-07-28 12:19 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
[in-fx-team] toolbar2.1 (7.01 KB, patch)
2011-07-28 13:07 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-06-23 10:58:27 PDT
The highlighter should have a toolbar for the handling of global functions related to it. For example, an inspect button, a button to toggle the html panel on or off, and a place for other tools to add themselves, e.g., style panel.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-06-29 07:38:10 PDT
Created attachment 542816 [details] [diff] [review]
toolbar

moving main toolbar out of the HTML panel.
Comment 2 Mihai Sucan [:msucan] 2011-06-29 12:10:51 PDT
Comment on attachment 542816 [details] [diff] [review]
toolbar

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

Patch looks good. I like we will have a global Inspector toolbar.

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

I am not sure why you add this here?

::: browser/base/content/browser.xul
@@ +742,5 @@
> +#  <toolbarbutton id="inspector-html-toolbutton"
> +#                 label="HTML"
> +#                 accesskey="H"
> +#                 class="toolbarbutton-text"
> +#                 command="Inspector:HTML"/>

Why do you add this here if it's not used?

::: browser/base/content/inspector.js
@@ +911,5 @@
>        return;
>      }
>  
>      this.closing = true;
> +    this.toolbar.setAttribute("collapsed", true);

Please add:

delete this.toolbar;
Comment 3 Rob Campbell [:rc] (:robcee) 2011-06-30 08:26:24 PDT
(In reply to comment #2)
> Comment on attachment 542816 [details] [diff] [review] [review]
> toolbar
> 
> Review of attachment 542816 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. I like we will have a global Inspector toolbar.
> 
> ::: browser/base/content/browser-sets.inc
> @@ +145,5 @@
> >    <commandset id="inspectorCommands">
> >      <command id="Inspector:Inspect"
> >               oncommand="InspectorUI.toggleInspection();"/>
> > +#    <command id="Inspector:HTML"
> > +#             oncommand="InspectorUI.toggleHTMLPanel();"/>
> 
> I am not sure why you add this here?
> 
> ::: browser/base/content/browser.xul
> @@ +742,5 @@
> > +#  <toolbarbutton id="inspector-html-toolbutton"
> > +#                 label="HTML"
> > +#                 accesskey="H"
> > +#                 class="toolbarbutton-text"
> > +#                 command="Inspector:HTML"/>
> 
> Why do you add this here if it's not used?

mostly as reminders for my next piece of work. I'll remove them.

> ::: browser/base/content/inspector.js
> @@ +911,5 @@
> >        return;
> >      }
> >  
> >      this.closing = true;
> > +    this.toolbar.setAttribute("collapsed", true);
> 
> Please add:
> 
> delete this.toolbar;

ah, good catch. Thank you!
Comment 4 Rob Campbell [:rc] (:robcee) 2011-07-28 12:19:43 PDT
Created attachment 549192 [details] [diff] [review]
toolbar 2

updated based on mihai's comments.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-07-28 12:46:43 PDT
Comment on attachment 549192 [details] [diff] [review]
toolbar 2

canceling review for a second. I want to add a bit to integrate better with the register tools patch.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-07-28 13:07:06 PDT
Created attachment 549205 [details] [diff] [review]
[in-fx-team] toolbar2.1

updated. Repositioned toolbar to bottom of browser and updated registerTool to use the new location.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-02 12:42:30 PDT
Comment on attachment 549205 [details] [diff] [review]
[in-fx-team] toolbar2.1

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>   <vbox id="browser-bottombox" layer="true">
>+    <toolbar id="inspector-toolbar"
>+             collapsed="true">

Can you use hidden instead? That'll remove any possible Txul impact from having the toolbar there (though it should be minimal). And you should be able to use .hidden rather than toggling attributes directly.

Should the toolbar keep the nowindowdrag attribute? I'm not sure what behavior makes the most sense.

Does this interact with the findbar and/or sync notification bars decently? Those get added dynamically to the top of the browser-bottombox when they're added, so they'll show up on top of this toolbar. I guess that's OK?

r=me with those addressed.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-08-02 13:37:24 PDT
(In reply to comment #7)
> Comment on attachment 549205 [details] [diff] [review] [diff] [details] [review]
> toolbar2.1
> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >   <vbox id="browser-bottombox" layer="true">
> >+    <toolbar id="inspector-toolbar"
> >+             collapsed="true">
> 
> Can you use hidden instead? That'll remove any possible Txul impact from
> having the toolbar there (though it should be minimal). And you should be
> able to use .hidden rather than toggling attributes directly.

Should be able to. I'll try it and see.

> Should the toolbar keep the nowindowdrag attribute? I'm not sure what
> behavior makes the most sense.

I think so. The idea is that this is soon going to be the top toolbar with a docked HTML panel underneath it (between this toolbar and the addon bar). I think window dragging behavior would be weird there, and I'm toying with the idea of having drag resizing on the toolbar itself or in a splitter like the web console has.

Separate issue, but yes. No window drag. :)

> Does this interact with the findbar and/or sync notification bars decently?
> Those get added dynamically to the top of the browser-bottombox when they're
> added, so they'll show up on top of this toolbar. I guess that's OK?

that's what I think we want here. Would be strange to bring up the findbar with the highlighter active and have it appear below the highlighter toolbar and eventual HTML panel.

> r=me with those addressed.

Thanks, Gavin!

(just verified that hidden works well for the above, so I think this is good to go)
Comment 9 Rob Campbell [:rc] (:robcee) 2011-08-02 14:35:38 PDT
Comment on attachment 549205 [details] [diff] [review]
[in-fx-team] toolbar2.1

http://hg.mozilla.org/integration/fx-team/rev/3d5ba47b14f2
Comment 10 Rob Campbell [:rc] (:robcee) 2011-08-02 16:48:41 PDT
testfix:

http://hg.mozilla.org/integration/fx-team/rev/56a12277818e
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-03 18:06:49 PDT
> http://hg.mozilla.org/mozilla-central/rev/3d5ba47b14f2

nits:
- looks like you forgot to add nowindowdrag="true"
- this.toolbar.hidden = null; <- should be false
Comment 13 Rob Campbell [:rc] (:robcee) 2011-08-04 10:01:38 PDT
augh. I'll push fixes.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-08-04 10:07:41 PDT
http://hg.mozilla.org/integration/fx-team/rev/b268f3e3b307
Comment 15 Tim Taubert [:ttaubert] 2011-08-07 11:28:36 PDT
http://hg.mozilla.org/mozilla-central/rev/b268f3e3b307
Comment 16 Teodosia Pop 2011-10-07 02:47:55 PDT
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.

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