Just because we can.

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?

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

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

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

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.

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

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

OK, I removed class="viewSelector".

Comment on attachment 365639 [details] [diff] [review] v3 requesting review from Mossop on the extensions.js/.xul changes

Comment on attachment 365639 [details] [diff] [review] v3 requesting review from Enn on the WindowDraggingUtils.jsm change

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!

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

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.

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 on attachment 365639 [details] [diff] [review] v3 The WindowDraggingUtils.jsm change is OK. I didn't look at the rest of the patch.

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

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

Comment on attachment 369782 [details] [diff] [review] v4 I think this is ok, though I had to unbitrot it to get it to apply.

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