Use inset box shadows instead of border images for the toolbar buttons in the Add-ons Manager, Page Info dialog and Error Console

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Themes
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

({memory-footprint})

Trunk
mozilla1.9.2a1
All
Mac OS X
memory-footprint
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 361541 [details] [diff] [review]
fix v1

Just because we can.
Attachment #361541 - Flags: review?(dao)
Comment on attachment 361541 [details] [diff] [review]
fix v1

>+#topStackBar {
>+  display: -moz-box;

This doesn't seem right for the Add-ons manager.
I don't see why that stack even exists in Page Info...

> #viewGroup > * {
[...]
>+  border: none;
>+  border-left: 1px solid rgba(0, 0, 0, 0.8);

> #viewGroup > radio[selected=true],
> #viewGroup > toolbarbutton[checked=true] {
[...]
>+  border: none;
>+  border-left: 1px solid rgba(0, 0, 0, 0.8);

Isn't that redundant?
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> (From update of attachment 361541 [details] [diff] [review])
> >+#topStackBar {
> >+  display: -moz-box;
> 
> This doesn't seem right for the Add-ons manager.
> I don't see why that stack even exists in Page Info...

I don't see why it exists in the Add-ons manager either. From reading extensions.js it seems that at all times only one of #viewGroup or #progressBox is visible, so #topStackBar could just be a vbox there.

Or even better, we could just remove the stacks and style the windowdragbox instead. Do you want me to do that?

> > #viewGroup > * {
> [...]
> >+  border: none;
> >+  border-left: 1px solid rgba(0, 0, 0, 0.8);
> 
> > #viewGroup > radio[selected=true],
> > #viewGroup > toolbarbutton[checked=true] {
> [...]
> >+  border: none;
> >+  border-left: 1px solid rgba(0, 0, 0, 0.8);
> 
> Isn't that redundant?

toolbarbutton.css sets left and right borders for checked toolbarbuttons. This reverses it. I agree that it looks redundant; should I use !important on the first border declaration? Or do you have a better idea?
(In reply to comment #2)
> I don't see why it exists in the Add-ons manager either. From reading
> extensions.js it seems that at all times only one of #viewGroup or #progressBox
> is visible, so #topStackBar could just be a vbox there.

Seems like it should be a deck, so that extensions.js can just do progressBox.parentNode.selectedPanel = progressBox; and viewGroup.parentNode.selectedPanel = viewGroup; instead of the current mess.

> Or even better, we could just remove the stacks and style the windowdragbox
> instead. Do you want me to do that?

Not sure if windowdragbox should be styled. Couldn't there be multiple windowdragboxes? In that case, you should probably use an id or class.

> toolbarbutton.css sets left and right borders for checked toolbarbuttons. This
> reverses it. I agree that it looks redundant; should I use !important on the
> first border declaration? Or do you have a better idea?

Hm, sucks that toolbarbutton.css takes precedence here. !important seems slightly better, I think, although it won't be obvious why that stuff is needed either way.
(Assignee)

Comment 4

9 years ago
(In reply to comment #3)
> > toolbarbutton.css sets left and right borders for checked toolbarbuttons. This
> > reverses it. I agree that it looks redundant; should I use !important on the
> > first border declaration? Or do you have a better idea?
> 
> Hm, sucks that toolbarbutton.css takes precedence here.

I was wrong. toolbarbutton.css doesn't take precedence here, the additional border rules are unnecessary. I don't know why I thought they were necessary.
(Assignee)

Comment 5

9 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > I don't see why it exists in the Add-ons manager either. From reading
> > extensions.js it seems that at all times only one of #viewGroup or #progressBox
> > is visible, so #topStackBar could just be a vbox there.
> 
> Seems like it should be a deck, so that extensions.js can just do
> progressBox.parentNode.selectedPanel = progressBox; and
> viewGroup.parentNode.selectedPanel = viewGroup; instead of the current mess.

That makes sense... but I can't find a way of centering the radiogroup in the deck while at the same time giving progressBox full width. Is it possible without introducing another wrapper box?
The code I removed in bug 456214 applied a "radiogroup-wrapper" binding to the radiogroup which provided another wrapper. Should I resurrect that binding?
Wrapping the radio group in a box seems fine, I wouldn't use a binding for that.

Updated

9 years ago
Attachment #361541 - Flags: review?(dao)
(Assignee)

Comment 7

9 years ago
Created attachment 364895 [details] [diff] [review]
v2
Attachment #361541 - Attachment is obsolete: true
Attachment #364895 - Flags: review?(dao)
(Assignee)

Updated

9 years ago
Attachment #364895 - Flags: review?(dtownsend)
(Assignee)

Comment 8

9 years ago
Comment on attachment 364895 [details] [diff] [review]
v2

Mossop, could you review the extensions.js/.xul changes?
Comment on attachment 364895 [details] [diff] [review]
v2

Also, it would be nice if Enn could review the change in WindowDraggingUtils.jsm.
Attachment #364895 - Flags: review?(enndeakin)
I think viewGroup should be a class, and would you mind fixing winstripe and gnomestripe to use topBar instead of viewSelector?
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> I think viewGroup should be a class, 

Why? Seems to me like lots of changes without obvious gain.

> and would you mind fixing winstripe and
> gnomestripe to use topBar instead of viewSelector?

I can do that, but would that change anything? Can I do it in a follow-up bug?
(In reply to comment #11)
> (In reply to comment #10)
> > I think viewGroup should be a class, 
> 
> Why?

Because you're adding viewGroupWrapper as a class, and it seems like they should be consistent.
Should be a simple search-and-replace, right?

> > and would you mind fixing winstripe and
> > gnomestripe to use topBar instead of viewSelector?
> 
> I can do that, but would that change anything? Can I do it in a follow-up bug?

Yeah, you could drop the viewSelector class instead of carrying it around. Could be done as a follow-up, but would also be quite easy to integrate here.
Oh, the viewGroup id is used in extensions.js and pageInfo.js, so you can't just make it a class.
(Assignee)

Comment 14

9 years ago
Created attachment 365639 [details] [diff] [review]
v3

OK, I removed class="viewSelector".
Attachment #364895 - Attachment is obsolete: true
Attachment #365639 - Flags: review?(dao)
Attachment #364895 - Flags: review?(enndeakin)
Attachment #364895 - Flags: review?(dtownsend)
Attachment #364895 - Flags: review?(dao)
(Assignee)

Comment 15

9 years ago
Comment on attachment 365639 [details] [diff] [review]
v3

requesting review from Mossop on the extensions.js/.xul changes
Attachment #365639 - Flags: review?(dtownsend)
(Assignee)

Updated

9 years ago
Attachment #365639 - Flags: review?(enndeakin)
(Assignee)

Comment 16

9 years ago
Comment on attachment 365639 [details] [diff] [review]
v3

requesting review from Enn on the WindowDraggingUtils.jsm change

Updated

9 years ago
Attachment #365639 - Flags: review?(dao) → review+
Comment on attachment 365639 [details] [diff] [review]
v3

>--- a/browser/base/content/pageinfo/pageInfo.xul
>+++ b/browser/base/content/pageinfo/pageInfo.xul

>+  <windowdragbox id="topBar" class="viewGroupWrapper" orient="vertical">

remove orient="vertical"

>--- a/toolkit/mozapps/extensions/content/extensions.xul
>+++ b/toolkit/mozapps/extensions/content/extensions.xul

>+  <windowdragbox orient="vertical" id="topBar">

here too

>--- a/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css

> /* View buttons */
>-.viewSelector {
>+#topBar {
>   -moz-appearance: listbox;
>-  margin: 8px 8px 0 8px;
>-  padding: 0;
>+  margin: 8px 8px 0;
> }

/* View buttons resp. Progress box */

>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

> /* View buttons */
>-.viewSelector {
>+#topBar {
>   border-bottom: 2px groove ThreeDFace;
>-  margin: 0px;
>   -moz-padding-start: 10px;
>   background-color: -moz-Field;
>   color: -moz-FieldText;
> }

same here

Thanks!
(Assignee)

Comment 18

9 years ago
(In reply to comment #17)
> remove orient="vertical"

> here too

That breaks the centering.

> /* View buttons resp. Progress box */

OK, I'll do that on checkin.
(In reply to comment #18)
> > remove orient="vertical"
> 
> That breaks the centering.

You probably want to use -moz-box-pack: center; and make <vbox class="viewGroupWrapper"> an hbox.
(Assignee)

Comment 20

9 years ago
OK, that works when I also add flex="1" to the deck. What advantages does this approach have over orient="vertical"?
It's more straightforward.
Attachment #365639 - Flags: review?(dtownsend) → review-
Comment on attachment 365639 [details] [diff] [review]
v3

This breaks when using the pill button. Currently it hides the entire pane selector area, with this patch it hides the selector but leaves a big gap still.

I'm part inclined to say that the pill button shouldn't actually do anything, but I think since it already does we should keep that behaviour.

Updated

9 years ago
Attachment #365639 - Flags: review?(enndeakin) → review+

Comment 23

9 years ago
Comment on attachment 365639 [details] [diff] [review]
v3

The WindowDraggingUtils.jsm change is OK. I didn't look at the rest of the patch.
(Assignee)

Comment 24

9 years ago
(In reply to comment #22)
> (From update of attachment 365639 [details] [diff] [review])
> This breaks when using the pill button.

Good catch!

> I'm part inclined to say that the pill button shouldn't actually do anything,
> but I think since it already does we should keep that behaviour.

Keeping the exact behaviour is a little tricky with the <deck> approach.
Would it be acceptable to move class="chromeclass-toolbar" to the deck or the windowdragbox, so that the progress box is hidden, too? That's the easiest solution I can think of.
(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 365639 [details] [diff] [review] [details])
> > This breaks when using the pill button.
> 
> Good catch!
> 
> > I'm part inclined to say that the pill button shouldn't actually do anything,
> > but I think since it already does we should keep that behaviour.
> 
> Keeping the exact behaviour is a little tricky with the <deck> approach.
> Would it be acceptable to move class="chromeclass-toolbar" to the deck or the
> windowdragbox, so that the progress box is hidden, too? That's the easiest
> solution I can think of.

That sounds fine I think. I'm mainly concerned with it not looking ugly when someone tries that.
(Assignee)

Comment 26

9 years ago
Created attachment 369782 [details] [diff] [review]
v4

moved class="chromeclass-toolbar" to the windowdragbox + addressed comment 17
Attachment #365639 - Attachment is obsolete: true
Attachment #369782 - Flags: review?(dtownsend)
Attachment #369782 - Flags: review?(dtownsend) → review+
Comment on attachment 369782 [details] [diff] [review]
v4

I think this is ok, though I had to unbitrot it to get it to apply.
Blocks: 414116
(Assignee)

Comment 28

9 years ago
http://hg.mozilla.org/mozilla-central/rev/6565c955be7b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 491058
(Assignee)

Updated

9 years ago
Depends on: 491329
Verified fixed by inspecting the elements with DOMi.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre ID:20090526031623
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.