The default bug view has changed. See this FAQ.

[highlighter] move the close button to the toolbar

VERIFIED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 10
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
because it takes too much space on the content.
Created attachment 557842 [details]
DevTools Toolbar Close - Pinstripe
Created attachment 557843 [details]
DevTools Toolbar Close - Winstripe

States are from left to right: Normal, Hover, Pressed
(Assignee)

Comment 3

6 years ago
Thank you Stephen.
(Assignee)

Comment 4

6 years ago
I think this button should be at the right-end of the tool bar (in LTR, and for all the OSes). I don't think it is a good idea to keep it next to the "Inspect" button (which is the most important on in this toolbar).
(Assignee)

Comment 5

6 years ago
Created attachment 560046 [details] [diff] [review]
patch v1
Assignee: nobody → paul
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #560046 - Flags: review?(juddhale)
(Assignee)

Updated

6 years ago
Attachment #560046 - Flags: review?(juddhale) → review?(ddahl)
(In reply to Stephen Horlander from comment #2)
> Created attachment 557843 [details]
> DevTools Toolbar Close - Winstripe
> 
> States are from left to right: Normal, Hover, Pressed

Is it just me or is that image blurry?

(In reply to Paul Rouget [:paul] from comment #4)
> I think this button should be at the right-end of the tool bar (in LTR, and
> for all the OSes). I don't think it is a good idea to keep it next to the
> "Inspect" button (which is the most important on in this toolbar).

This makes it inconsistent with the add-on bar and the find bar.
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 560046 [details] [diff] [review]
patch v1

>+      <spacer flex="1" />
>+      <toolbarbutton id="highlighter-close-button" oncommand="InspectorUI.closeInspectorUI(false);" tooltiptext="&inspectCloseButton.tooltiptext;" />

Superfluous spaces before /> and the second line is overly long.

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

> #highlighter-close-button {
>-  list-style-image: url("chrome://browser/skin/KUI-close.png");
>-  top: 12px;
>-  right: 12px;
>-  cursor: pointer;
>+  list-style-image: url("chrome://browser/skin/devtools/toolbarbutton-close.png");
>+  -moz-image-region: rect(0, 16px, 16px, 0);
>+  min-width: 16px;
>+  width: 16px;
>+}

For consistency with all other close buttons in gnomestrip, this should probably use the stock icon.

Comment 8

6 years ago
Comment on attachment 560046 [details] [diff] [review]
patch v1

Looks good. passing to dao.
Attachment #560046 - Flags: review?(ddahl)
Attachment #560046 - Flags: review?(dao)
Attachment #560046 - Flags: review+
Comment on attachment 560046 [details] [diff] [review]
patch v1

see comment 6 and comment 7
Attachment #560046 - Flags: review?(dao) → review-
(Assignee)

Updated

6 years ago
Blocks: 663830
(Assignee)

Updated

6 years ago
Depends on: 672006
(Assignee)

Comment 10

6 years ago
Created attachment 568636 [details] [diff] [review]
patch v1.1

Dao, I use `margin:-4px` for the gnomestrip button:
> /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
>   use evil CSS to give the impression of smaller content */

The same thing is used for `.tab-close-button > .toolbarbutton-icon`
(see http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/browser.css#1597)

Let me know if you think this could be a problem.
(Assignee)

Updated

6 years ago
Attachment #560046 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #568636 - Flags: review?(dao)
Comment on attachment 568636 [details] [diff] [review]
patch v1.1

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>+#inspector-toolbar[treepanel-open] {
>+  padding: 0 0 4px 3px;
>+}

Does this need to take RTL into account?

>+#inspector-toolbar[treepanel-open] .inspector-resizer {
>+  display: -moz-box;
> }

Can the descendant selector be avoided here?
This looks like it belongs in content CSS.

>diff --git a/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png b/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png

Gnomestripe doesn't need this image anymore.
Attachment #568636 - Flags: review?(dao) → review-
(Assignee)

Comment 12

6 years ago
Created attachment 568661 [details] [diff] [review]
patch v1.2

(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 568636 [details] [diff] [review] [diff] [details] [review]
> patch v1.1
> 
> >--- a/browser/themes/gnomestripe/browser/browser.css
> >+++ b/browser/themes/gnomestripe/browser/browser.css
> 
> >+#inspector-toolbar[treepanel-open] {
> >+  padding: 0 0 4px 3px;
> >+}
> 
> Does this need to take RTL into account?

Fixed.

> >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> >+  display: -moz-box;
> > }
> 
> Can the descendant selector be avoided here?

I could avoid this, but it would need more CSS (to match the 2 resizers),
or more JavaScript.

> This looks like it belongs in content CSS.

Fixed.

> >diff --git a/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png b/browser/themes/gnomestripe/browser/devtools/toolbarbutton-close.png
> 
> Gnomestripe doesn't need this image anymore.

Good catch. And I forgot to remove them from jar.mn as well.
Fixed.
(Assignee)

Updated

6 years ago
Attachment #568636 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
(sorry for the UTF8 bug in the previous comment. Sounds like bzexport doesn't like the "ã")
(Assignee)

Updated

6 years ago
Attachment #568661 - Flags: review?(dao)

Comment 14

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2
Comment on attachment 568661 [details] [diff] [review]
patch v1.2

(In reply to Paul Rouget [:paul] from comment #12)
> > >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> > >+  display: -moz-box;
> > > }
> > 
> > Can the descendant selector be avoided here?
> 
> I could avoid this, but it would need more CSS (to match the 2 resizers),
> or more JavaScript.

I wouldn't mind if this was a separate document that got created on demand, but since this is in browser.xul, the extra code seems like a price worth paying.
Attachment #568661 - Flags: review?(dao) → review-
(Assignee)

Comment 16

6 years ago
(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 568661 [details] [diff] [review] [diff] [details] [review]
> patch v1.2
> 
> (In reply to Paul Rouget [:paul] from comment #12)
> > > >+#inspector-toolbar[treepanel-open] .inspector-resizer {
> > > >+  display: -moz-box;
> > > > }
> > > 
> > > Can the descendant selector be avoided here?
> > 
> > I could avoid this, but it would need more CSS (to match the 2 resizers),
> > or more JavaScript.
> 
> I wouldn't mind if this was a separate document that got created on demand,
> but since this is in browser.xul, the extra code seems like a price worth
> paying.

What if I use:
#inspector-toolbar[treepanel-open] > vbox > .inspector-resizer,
#inspector-toolbar[treepanel-open] > vbox > hbox > .inspector-resizer {
 display: -moz-box;
}
Assignee: paul → nobody
Status: ASSIGNED → NEW
OS: All → Mac OS X
Priority: P2 → --
Hardware: All → x86
(Assignee)

Comment 17

6 years ago
(???, how did I manage to change all these fields?)
Assignee: nobody → paul
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P2
> What if I use:
> #inspector-toolbar[treepanel-open] > vbox > .inspector-resizer,
> #inspector-toolbar[treepanel-open] > vbox > hbox > .inspector-resizer {
>  display: -moz-box;
> }

Why would you use .inspector-resizer instead of the ids there?
(Assignee)

Comment 19

6 years ago
Created attachment 570951 [details] [diff] [review]
patch v1.3

(In reply to Dao Gottwald [:dao] from comment #18)
> Why would you use .inspector-resizer instead of the ids there?
No good reason.
(Assignee)

Updated

6 years ago
Attachment #570951 - Flags: review?(dao)
(Assignee)

Comment 20

6 years ago
Created attachment 570952 [details] [diff] [review]
patch v1.4

Rebase mistake.
(Assignee)

Updated

6 years ago
Attachment #570951 - Attachment is obsolete: true
Attachment #570951 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Attachment #570952 - Flags: review?(dao)

Updated

6 years ago
Attachment #570952 - Flags: review?(dao) → review+

Updated

6 years ago
Attachment #568661 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Thank you for the r+ Dao.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/5b1c3d9c342b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
added a fix for failing test:

https://hg.mozilla.org/integration/fx-team/rev/9f51745bdd6d
https://hg.mozilla.org/mozilla-central/rev/9f51745bdd6d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/5b1c3d9c342b

Comment 26

5 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.