Closed Bug 733292 Opened 8 years ago Closed 7 years ago

The sidebar should have a close button

Categories

(DevTools :: Inspector, defect, P2)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: paul, Unassigned)

Details

(Whiteboard: [good first bug][mentor=paul][lang=js][lang=xul])

Attachments

(3 files, 5 obsolete files)

No description provided.
triage, filter on centaur
Priority: -- → P2
Whiteboard: [good first bug][mentor=paul][lang=js][lang=xul]
Hi, am Aishwarya and am new here! I would like to fix this bug. Could you please help me out?
Sure thing!

In the sidebar, there are 2 buttons. Rule and Computed. We want a close button on the right.

These buttons live in a toolbar.

You can see the toolbar here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1065

As you can see, the toolbar is empty. We add the buttons dynamically here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1656

What you need to do is to add a toolbarbutton in the browser.xul:

<toolbarbutton id="close"
   tooltiptext="&inspectorSidebarClosebutton.tooltip;"
   class="devtools-closebutton"
   oncommand="FIXME"/>

inspectorSidebarClosebutton.tooltip doesn't exist, you'll have to create it in the locales/ folder.

You will have to make sure the rule and computed buttons are added on the left of the close button, not after.

You will also need to find how to close the sidebar (today, to close it, we uncheck the "style" button, just look at how it works).

Let me know if you need anymore information (and come to IRC (#devtools, I'm "paul")).
Attached image Screenshot
Attached patch Patch File (obsolete) — Splinter Review
Attachment #656901 - Flags: review?
Attachment #656901 - Flags: review? → review?(paul)
Comment on attachment 656901 [details] [diff] [review]
Patch File

Please make sure the indentation is correct. And there's an extra new line in the browser.xul. And remove the comment.

Between the closebutton and the property button, we need a <spacer flex="1"> to force the close button to be right-aligned.

Beside that, it looks good :)

Thank you!
Attachment #656901 - Flags: review?(paul)
Attached image Sreen Shot
Attached patch Patch File- Updated alignment (obsolete) — Splinter Review
Attachment #657241 - Flags: review?(paul)
Comment on attachment 657241 [details] [diff] [review]
Patch File- Updated alignment

>+                 class="devtools-toolbar"
>+                 nowindowdrag="true">
>+            <spacebar id="devtools-side-toolbar-spacer" flex="1"/>

Not spacebar, spacer. No need to give it an id.

>+            <toolbarbutton id="close"

No need to give it an id.

>diff -r e874475efe15 browser/devtools/highlighter/inspector.jsm
>--- a/browser/devtools/highlighter/inspector.jsm	Sat Aug 25 11:42:21 2012 -0400
>+++ b/browser/devtools/highlighter/inspector.jsm	Fri Aug 31 20:59:38 2012 +0800
>@@ -1678,8 +1678,9 @@
>     btn.setAttribute("image", aRegObj.icon || "");
>     btn.setAttribute("type", "radio");
>     btn.setAttribute("group", "sidebar-tools");
>-    this._toolbar.appendChild(btn);
> 
>+    let spacer = this._toolbar.querySelector("#devtools-side-toolbar-spacer");

Better to do:  let spacer = this._toolbar.querySelector("spacer")

>+    this._toolbar.insertBefore(btn,spacer);

Add a space before "spacer".

-----


Thank you! Please address the above comments and I will then request a browser peer to review this (and then we commit your code).
Attachment #657241 - Flags: review?(paul) → review+
Attachment #657247 - Flags: review?(paul)
Comment on attachment 657247 [details] [diff] [review]
Patch File - Updated Spacer Changes

>--- a/browser/base/content/browser.xul	Sat Aug 25 11:42:21 2012 -0400
>+++ b/browser/base/content/browser.xul	Fri Aug 31 21:25:35 2012 +0800
>@@ -1062,10 +1062,16 @@
>     <splitter id="devtools-side-splitter" hidden="true"/>
>     <vbox id="devtools-sidebar-box" hidden="true"
>           style="min-width: 18em; width: 22em; max-width: 42em;" persist="width">
>-      <toolbar id="devtools-sidebar-toolbar"
>-               class="devtools-toolbar"
>-               nowindowdrag="true"/>
>-      <deck id="devtools-sidebar-deck" flex="1"/>
>+        <toolbar id="devtools-sidebar-toolbar"
>+                 class="devtools-toolbar"
>+                 nowindowdrag="true">
>+            <spacer flex="1"/>
>+            <toolbarbutton
>+                    tooltiptext="&inspectorSidebarClosebutton.tooltip;"
>+                    class="devtools-closebutton"
>+                    command="Inspector:Sidebar" />
>+        </toolbar>
>+        <deck id="devtools-sidebar-deck" flex="1"/>
>     </vbox>

Indentation is messed up. Please indent using two spaces.

>@@ -1081,7 +1087,6 @@
>     </vbox>
>     <vbox id="browser-border-end" hidden="true" layer="true"/>
>   </hbox>
>-
>   <hbox id="full-screen-warning-container" hidden="true" fadeout="true">
>     <hbox style="width: 100%;" pack="center"> <!-- Inner hbox needed due to bug 579776. -->
>       <vbox id="full-screen-warning-message" align="center">

Please drop this change.

>--- a/browser/locales/en-US/chrome/browser/browser.dtd	Sat Aug 25 11:42:21 2012 -0400
>+++ b/browser/locales/en-US/chrome/browser/browser.dtd	Fri Aug 31 21:25:35 2012 +0800

>+<!ENTITY inspectorSidebarClosebutton.tooltip "Close Side Bar Inspector">

What's a "Side Bar Inspector"?
Attachment #656901 - Attachment is obsolete: true
Attachment #656901 - Attachment is patch: true
Attachment #657247 - Attachment is patch: true
Attachment #657241 - Attachment is obsolete: true
Attachment #657241 - Attachment is patch: true
(I was planning to cleanup the indentation before asking for a browser review, it's Aishwarya's first patch)

For the tooltip, we should use "Close Sidebar". Would that work for you Dao?
Aishwarya, can you update the patch with the new tooltip? Also, try to fix the indentation and extra lines problem (if you can't, I'll fix it for you).
Attached patch v1 (obsolete) — Splinter Review
Fixed indentation. Updated tooltip text.
Attachment #657247 - Attachment is obsolete: true
Attachment #657247 - Flags: review?(paul)
Attachment #658039 - Flags: review?(dao)
Aishwarya, I tweaked your patch a little for the browser review.
Is the long term plan to add close buttons to all side bars and panels that can be toggled from the inspector toolbar? The markup panel currently doesn't have a close button, which seems inconsistent.
(In reply to Dão Gottwald [:dao] from comment #16)
> Is the long term plan to add close buttons to all side bars and panels that
> can be toggled from the inspector toolbar? The markup panel currently
> doesn't have a close button, which seems inconsistent.

This inconsistency will be fixed with the Toolbox project (stacking all the tools into a box below the browser, and this box will have a close button).

So yes, we want a close button for the sidebar (there's only one sidebar).
Comment on attachment 658039 [details] [diff] [review]
v1

>+        <toolbarbutton tooltiptext="&inspectSidebarCloseButton.tooltiptext;"
>+                       class="devtools-closebutton"
>+                       command="Inspector:Sidebar" />

nit: drop the space before />

>+<!ENTITY inspectSidebarCloseButton.tooltiptext "Close Sidebar">

Since this is a tooltip, "sidebar" shouldn't be capitalized.
Attachment #658039 - Flags: review?(dao) → review+
Aishwarya, can you update the latest patch to address Dao's comments?
Thanks.
Attached patch Patch - Updated (obsolete) — Splinter Review
I have removed the capitalisation of Sidebar and removed the space.
Attachment #662425 - Flags: review?(paul)
Attachment #662425 - Flags: review?(paul)
Attached patch Patch - UpdatedSplinter Review
Sorry, that was a wrong patch. This is the latest patch.
Attachment #662425 - Attachment is obsolete: true
Attachment #662427 - Flags: review?(paul)
Attachment #658039 - Attachment is obsolete: true
Attachment #662427 - Flags: review?(paul) → review+
Thank you Aishwarya. We will land that soon in Firefox.
Whiteboard: [good first bug][mentor=paul][lang=js][lang=xul] → [good first bug][mentor=paul][lang=js][lang=xul][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/43f5c824aa3d
Whiteboard: [good first bug][mentor=paul][lang=js][lang=xul][land-in-fx-team] → [good first bug][mentor=paul][lang=js][lang=xul][fixed-in-fx-team]
Thanks Paul :)
https://hg.mozilla.org/mozilla-central/rev/43f5c824aa3d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=paul][lang=js][lang=xul][fixed-in-fx-team] → [good first bug][mentor=paul][lang=js][lang=xul]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.