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

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Markus Stange [:mstange]]

Attachment: fix v1

Just because we can.

Comment 1

[Dão Gottwald [: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?

Comment 2

[Markus Stange [:mstange]]

(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?

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Markus Stange [:mstange]]

(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.

Comment 5

[Markus Stange [:mstange]]

(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?

Comment 6

[Dão Gottwald [:dao]]

Wrapping the radio group in a box seems fine, I wouldn't use a binding for that.

Comment 7

[Markus Stange [:mstange]]

Attachment: v2

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 364895 [details] [diff] [review]
v2

Mossop, could you review the extensions.js/.xul changes?

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 364895 [details] [diff] [review]
v2

Also, it would be nice if Enn could review the change in WindowDraggingUtils.jsm.

Comment 10

[Dão Gottwald [:dao]]

I think viewGroup should be a class, and would you mind fixing winstripe and gnomestripe to use topBar instead of viewSelector?

Comment 11

[Markus Stange [:mstange]]

(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?

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Dão Gottwald [:dao]]

Oh, the viewGroup id is used in extensions.js and pageInfo.js, so you can't just make it a class.

Comment 14

[Markus Stange [:mstange]]

Attachment: v3

OK, I removed class="viewSelector".

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 365639 [details] [diff] [review]
v3

requesting review from Mossop on the extensions.js/.xul changes

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 365639 [details] [diff] [review]
v3

requesting review from Enn on the WindowDraggingUtils.jsm change

Comment 17

[Dão Gottwald [:dao]]

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!

Comment 18

[Markus Stange [:mstange]]

(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.

Comment 19

[Dão Gottwald [:dao]]

(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.

Comment 20

[Markus Stange [:mstange]]

OK, that works when I also add flex="1" to the deck. What advantages does this approach have over orient="vertical"?

Comment 21

[Dão Gottwald [:dao]]

It's more straightforward.

Comment 22

[Dave Townsend [:mossop]]

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.

Comment 23

[Neil Deakin]

Comment on attachment 365639 [details] [diff] [review]
v3

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

Comment 24

[Markus Stange [:mstange]]

(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.

Comment 25

[Dave Townsend [:mossop]]

(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.

Comment 26

[Markus Stange [:mstange]]

Attachment: v4

moved class="chromeclass-toolbar" to the windowdragbox + addressed comment 17

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 369782 [details] [diff] [review]
v4

I think this is ok, though I had to unbitrot it to get it to apply.

Comment 28

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/6565c955be7b

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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