u428464 Reporter
	Description • 11 years ago

The large widgets should be borderless like the other button. See http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html to see the styling. Zoom controls and edit buttons are concerned for now.

	u428464 Reporter
	Comment 1 • 11 years ago

I don't know about inverting the place of plus-minus buttons on the zoom controls. We had the other way around before, but as I pointed out in previous bugs since we read left to right and other browsers show it like our current implementation I think it should stay that way (-/100%/+).

	Mike de Boer [:mikedeboer] Assignee
	Comment 2 • 11 years ago

This patch implements the flat panel-button style, as seen in all recent mockups by shorlander.
As per http://people.mozilla.org/~shorlander/files/australis-design-specs/australis-design-specs-osx.html I also moved away from the darker panel style to the light one.

	Mike de Boer [:mikedeboer] Assignee
	Comment 3 • 11 years ago

(The lighter panel style on OSX is required for the flat button style, because the darker panel made the flat buttons very hard to discern).

	Mike de Boer [:mikedeboer] Assignee
	Comment 4 • 11 years ago

This patch moves the styles for the wide panel items to a separate CSS include file.

It also makes sure that the zoom and edit controls look and behave just like in the interactive mockup when in the menupanel, overflow panel and toolbar.

	Mike de Boer [:mikedeboer] Assignee
	Comment 6 • 11 years ago

Comment on attachment 825248 [details] [diff] [review]
Patch 2: update the panel styling for the zoom and edit controls

Per discussion on IRC, proxying review to Jared.

Dão, please feel free to comment/ review if you have time!

	:Gijs (he/him)
	Comment 7 • 11 years ago

Comment on attachment 825245 [details] [diff] [review]
Patch 1: introduce the final, flat panel UI style

And stealing per IRC.

	:Gijs (he/him)
	Comment 8 • 11 years ago

Comment on attachment 825245 [details] [diff] [review]
Patch 1: introduce the final, flat panel UI style

Review of attachment 825245 [details] [diff] [review]:
-----------------------------------------------------------------

I have a lot of questions, and you need a toolkit peer for the toolkit part, so clearing review and redirecting to Blair for the toolkit bits.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +11,5 @@
>  %include ../browser.inc
>  
>  .panel-subviews {
>    background-image: linear-gradient(to bottom, white 1px, rgba(255, 255, 255, 0) 15px);
> +  background-color: Menu;

We're hardcoding a background for the main panel. I think we should do the same here. Using the menu background will regress the appearance of the toolbar buttons in e.g. the bookmarks menu popup subview against the now different background.

@@ +302,2 @@
>    border-radius: 2px;
> +  border: 1px solid !important;

!important proliferation! Why is this necessary? Can we just make the panelview selector be more specific? (I'm guessing that's the one that's "problematic" here) Now all these rules are using !important. They weren't before... :-(

@@ +305,1 @@
>    transition-property: background-color, border-color, box-shadow;

You removed the box-shadow here, but still transition it. I don't think the transition is necessary, especially not as this is for :active - the mousedown effect should be immediate.

@@ +318,4 @@
>  }
>  
>  panelview toolbarbutton@buttonStateActive@,
> +.customizationmode-button@buttonStateActive@,

Why get rid of the disabled check here?

::: toolkit/themes/osx/global/popup.css
@@ +67,5 @@
>  }
>  
>  .panel-arrowcontent {
>    -moz-appearance: none;
> +  background: #f5f5f5;

I don't think Jared and I realized you were changing toolkit. You need a toolkit peer's review for this.

If this part does get review, please remove:

http://mxr.mozilla.org/projects-central/source/ux/browser/themes/osx/browser.css#4037

and any other associated styles for the plugin doorhanger that override the toolkit style.

(separate note: are similar changes not necessary for Windows/Linux?)

	:Gijs (he/him)
	Comment 9 • 11 years ago

Comment on attachment 825248 [details] [diff] [review]
Patch 2: update the panel styling for the zoom and edit controls

Review of attachment 825248 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +324,5 @@
>          if (inPanel)
>            btnNode.setAttribute("tabindex", "0");
>          node.appendChild(btnNode);
> +        // The middle node is the 'Reset Zoom' button.
> +        if (aIndex == 1)

But you are hardcoding what the middle is... Just leave this where it was and use [2] rather than [1].

@@ +326,5 @@
>          node.appendChild(btnNode);
> +        // The middle node is the 'Reset Zoom' button.
> +        if (aIndex == 1)
> +          zoomResetButton = btnNode;
> +        if (aIndex != buttonCount - 1) {

Just insert the separator if index != 0, and insert it *before* you insert the node itself. Then you don't need the count.

@@ +329,5 @@
> +          zoomResetButton = btnNode;
> +        if (aIndex != buttonCount - 1) {
> +          node.appendChild(
> +            aDocument.createElementNS(kNSXUL, "separator")
> +          );

Please don't split this like this. Either use a variable, or just make it one line. It's not *that* long.

@@ +480,5 @@
>          setAttributes(btnNode, aButton);
>          if (inPanel)
>            btnNode.setAttribute("tabindex", "0");
>          node.appendChild(btnNode);
> +        if (aIndex != buttonCount - 1) {

Ditto.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +47,2 @@
>    margin-left: 0;
> +  margin-bottom: 0;

Why not just margin: 0 like before?

By the way, this now selects all buttons which aren't in a toolbar. That's unfortunate because it includes the menu button, which now creeps up to the side of the window in an unfortunate fashion, and probably the overflow button too. r- because of that.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +64,5 @@
>  #PanelUI-contents {
>    padding: .5em 0;
>  }
>  
> +:-moz-any(#PanelUI-contents,#widget-overflow-list) > toolbarpaletteitem > toolbaritem > toolbarbutton > .toolbarbutton-text,

I'm racing you here with bug 921732.

@@ +258,5 @@
>    transition-duration: 150ms;
>  }
>  
> +panelview toolbarbutton,
> +panelview .toolbarbutton-1,

Having both these selectors is weird. Why is 1 not good enough?

@@ +260,5 @@
>  
> +panelview toolbarbutton,
> +panelview .toolbarbutton-1,
> +#widget-overflow-list > toolbarbutton {
> +  margin-top: 6px;

As I understand it, this creates empty gaps between the panel buttons. Is that correct? That's going to break a lot of our drag and drop logic (because you can hover somewhere in the middle between buttons, which will mean you're hovering over the panel which will mean it gets appended at the end). Is this in the design? Can we still change this?

@@ +275,5 @@
>    border-color: hsla(210,4%,10%,.1) !important;
>  }
>  
>  panelview toolbarbutton@buttonStateActive@,
> +#widget-overflow-list > toolbarbutton@buttonStateActive@,

Please don't reorder (it breaks blame and makes the diff hard to read)

@@ -386,5 @@
> -}
> -
> -#edit-controls@inAnyPanel@ > toolbarbutton,
> -#zoom-controls@inAnyPanel@ > toolbarbutton {
> -  -moz-appearance: none;

This is gone. Why?

@@ -452,5 @@
>  }
>  
>  #widget-overflow-list > .overflowedItem {
>    width: 100%;
> -  max-width: @menuPanelWidth@;

Why remove this?

::: browser/themes/shared/customizableui/widePanelWidgets.inc.css
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Can you explain why you split this out? There's still a lot of specialcasing in the panelUIOverlay.inc.css file to deal with these widgets, might as well have everything in there. This also makes this a lot harder to review, because it's not easy to see what you changed and what you just copied from the other file... So in future, if possible please split up the moving/refactoring from the fixing.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#search-container@inAnyPanel@ {
> +  width: @menuPanelWidth@;
> +}

I'm changing this in bug 933262, which will land as soon as I've finished reviewing this. Please adopt. :-)

@@ +27,5 @@
> +}
> +
> +#edit-controls@inAnyPanel@ > toolbarbutton,
> +#zoom-controls@inAnyPanel@ > toolbarbutton {
> +  border: 0 !important;

Still more important! Is this really necessary? I'd rather have 6 selectors for the individual buttons, if that helped. :-(

@@ +52,5 @@
> +  min-width: 7em;
> +}
> +
> +#edit-controls@inAnyPanel@ > toolbarbutton,
> +#wrapper-edit-controls > #edit-controls@inAnyPanel@ > toolbarbutton,

The wrapper versions are unnecessary, they will still match the other selector.

@@ +66,5 @@
> +  -moz-box-orient: horizontal;
> +  -moz-box-align: start;
> +  max-height: none;
> +  height: auto;
> +  padding-left: .5em;

You've set padding 30 lines above here, so this is now redundant.

@@ +100,5 @@
> +  min-width: 7ch;
> +%endif
> +}
> +
> +#zoom-controls@inAnyPanel@ > #zoom-reset-button {

The toolbarbutton-icon is already display: none. Is this actually necessary?

@@ +125,5 @@
> +#zoom-controls@inAnyPanel@ > separator {
> +  -moz-appearance: none;
> +  width: 3px;
> +  min-width: 3px;
> +  height: calc(1em + 6px);

This is weird. Did you test this with different font settings on Windows? Presumably the toolbarbuttons and the separators could just -moz-box-align: stretch, possibly with a bit of margin?

@@ +130,5 @@
> +  background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0)),
> +                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.15) 40%, hsla(210,54%,20%,.15) 60%, hsla(210,54%,20%,0)),
> +                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0));
> +  background-size: 1px, 1px, 1px;
> +  background-position: top left, top center, top right;

Is this necessary?

	Mike de Boer [:mikedeboer] Assignee
	Comment 10 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #8)
> @@ +318,4 @@
> >  }
> >  
> >  panelview toolbarbutton@buttonStateActive@,
> > +.customizationmode-button@buttonStateActive@,
> 
> Why get rid of the disabled check here?

Well, I didn't really. Currently the selector says:

```css
.customizationmode-button:not([disabled]):hover:active
```

and `@buttonStateActive@` says

```css
:not([disabled]):-moz-any([open],[checked],:hover:active)
```

...which includes the same disabled check. I thought it'd be good to unify this one remaining selector.

> (separate note: are similar changes - for the panel (ed.) - not necessary for Windows/Linux?)

No, I checked this on Windows and Linux. The contrast of the panels there is good enough and already match Shorlanders' mockups.

	Mike de Boer [:mikedeboer] Assignee
	Comment 11 • 11 years ago

Patch with Gijs' comments addressed.

	Mike de Boer [:mikedeboer] Assignee
	Comment 13 • 11 years ago

Gijs, some stuff you commented on in the previous review is still here. I'll explain why in another comment.

	Mike de Boer [:mikedeboer] Assignee
	Comment 14 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #9)
> ::: browser/themes/osx/customizableui/panelUIOverlay.css
> @@ +47,2 @@
> >    margin-left: 0;
> > +  margin-bottom: 0;
> 
> Why not just margin: 0 like before?

Because of the newly introduced margin-top: 6px in the panel, it's not really wanted to reset that back to 0. Hence the unfolded notation.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +260,5 @@
> >  
> > +panelview toolbarbutton,
> > +panelview .toolbarbutton-1,
> > +#widget-overflow-list > toolbarbutton {
> > +  margin-top: 6px;
> 
> As I understand it, this creates empty gaps between the panel buttons. Is
> that correct? That's going to break a lot of our drag and drop logic
> (because you can hover somewhere in the middle between buttons, which will
> mean you're hovering over the panel which will mean it gets appended at the
> end). Is this in the design? Can we still change this?

This is in the design and we cannot change that. I'm aware that this will require adjustments in the DnD code for the panel, but that's something to be dealt with in a follow-up IMHO. 

> @@ -386,5 @@
> > -}
> > -
> > -#edit-controls@inAnyPanel@ > toolbarbutton,
> > -#zoom-controls@inAnyPanel@ > toolbarbutton {
> > -  -moz-appearance: none;
> 
> This is gone. Why?

This is already defined in the generic buttons in the panel rules. (The zoom and edit controls now inherit these too, so no need to duplicate here.)

> @@ -452,5 @@
> >  }
> >  
> >  #widget-overflow-list > .overflowedItem {
> >    width: 100%;
> > -  max-width: @menuPanelWidth@;
> 
> Why remove this?

I made it apply on the parent container, so that the zoom and edit controls are allowed to overflow. Both #edit-control and #zoom-control now each contain two separators of 3px wide, which makes them 6px wider than before. I set a negative margin of 3px to center it in the panel.
If you consider this a no-go, please help me in figuring out another method of sizing the controls.

> @@ +130,5 @@
> > +  background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0)),
> > +                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.15) 40%, hsla(210,54%,20%,.15) 60%, hsla(210,54%,20%,0)),
> > +                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0));
> > +  background-size: 1px, 1px, 1px;
> > +  background-position: top left, top center, top right;
> 
> Is this necessary?

I tried omitting all-to-some of the rules, to no avail. They are all needed to end up with the desired effect.

	:Gijs (he/him)
	Comment 15 • 11 years ago

Comment on attachment 826752 [details] [diff] [review]
Patch 2.2: update the panel styling for the zoom and edit controls

Review of attachment 826752 [details] [diff] [review]:
-----------------------------------------------------------------

r- because you changed the search-in-panel stuff, and I have too many questions still.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +278,5 @@
>      onBuild: function(aDocument) {
>        const kPanelId = "PanelUI-popup";
>        let inPanel = (this.currentArea == CustomizableUI.AREA_PANEL);
>        let noautoclose = inPanel ? "true" : null;
> +      let cls = inPanel ? "panel-combined-button" : null;

Err, no more toolbarbutton-1 in the toolbar? Why not?

@@ +359,3 @@
>          };
>          for (let i = 0, l = node.childNodes.length; i < l; ++i) {
> +          if (node.childNodes[i].localName == "separator")

Note that these changes are racing with bug 882353.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +315,5 @@
>  }
>  
> +panelview .toolbarbutton-1,
> +#widget-overflow-list > toolbarbutton {
> +  margin-top: 6px;

I really don't like this. This also means there is dead space inbetween two buttons, and clicking on that space will close the panel without hitting any of the buttons. Can we get confirmation from UX that this is what they want, rather than an artifact of the mockup? Can we come up with another way of doing this, like using the padding trick we use in the navbar, and setting the background/border on the image instead of the text? I suppose that's hard to do with variable-height text... :-\

@@ +391,5 @@
>  #bookmarks-menu-button[cui-areatype="menu-panel"] > .toolbarbutton-menubutton-dropmarker {
>    display: none;
>  }
>  
> +#search-container@inAnyPanel@ {

This is wrong. Please don't change this selector.

@@ +408,5 @@
> +  border: 1px solid;
> +  border-color: hsla(210,4%,10%,0);
> +  border-bottom-color: hsla(210,4%,10%,.1);
> +  padding: 0;
> +  -moz-margin-start: -3px;

Huh, yeah, I missed this. Not a fan. I have to confess I don't understand your explanation for it, either. Why not update the styles of the buttons to not be so wide that they don't fit? How is this solved in the design/mockup?

@@ +415,5 @@
> +  transition-duration: 150ms;
> +}
> +
> +#edit-controls@inAnyPanel@@buttonStateHover@,
> +#zoom-controls@inAnyPanel@@buttonStateHover@ {

Please add these selectors to the block above which sets exactly the same hover styles.

@@ +450,5 @@
>  #edit-controls@inAnyPanel@ > #paste-button:-moz-locale-dir(rtl),
>  #zoom-controls@inAnyPanel@ > #zoom-out-button:-moz-locale-dir(ltr),
>  #zoom-controls@inAnyPanel@ > #zoom-in-button:-moz-locale-dir(rtl) {
> +  border-top-right-radius: 0;
> +  border-bottom-right-radius: 0;

Wat. These used to be explicitly radiused, now they're explicitly non-radiused? Why? If none of these are meant to have a radius (which I think is the case), then please use much simpler selectors to turn it off specifically for these nodes, or don't include the edit/zoom controls in with the other stuff which gets a radius.

@@ +458,5 @@
>  #edit-controls@inAnyPanel@ > #paste-button:-moz-locale-dir(ltr),
>  #zoom-controls@inAnyPanel@ > #zoom-out-button:-moz-locale-dir(rtl),
>  #zoom-controls@inAnyPanel@ > #zoom-in-button:-moz-locale-dir(ltr) {
> +  border-top-left-radius: 0;
> +  border-bottom-left-radius: 0;

Ditto.

@@ +471,5 @@
> +  background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0)),
> +                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.15) 40%, hsla(210,54%,20%,.15) 60%, hsla(210,54%,20%,0)),
> +                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0));
> +  background-size: 1px, 1px, 1px;
> +  background-position: top left, top center, top right;

I think what I meant was, if this is really just: 0 0, 1px 0, 2px 0, is it necessary? Don't the background automatically get ordered side-by-side-horizontally, and/or is there a simpler way to do that? If not, can we at least use the px values?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 16 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #15)
> I really don't like this. This also means there is dead space inbetween two
> buttons, and clicking on that space will close the panel without hitting any
> of the buttons.

This is bug 934532.

	Mike de Boer [:mikedeboer] Assignee
	Comment 17 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #15)
> ::: browser/components/customizableui/src/CustomizableWidgets.jsm
> @@ +278,5 @@
> >      onBuild: function(aDocument) {
> >        const kPanelId = "PanelUI-popup";
> >        let inPanel = (this.currentArea == CustomizableUI.AREA_PANEL);
> >        let noautoclose = inPanel ? "true" : null;
> > +      let cls = inPanel ? "panel-combined-button" : null;
> 
> Err, no more toolbarbutton-1 in the toolbar? Why not?

Because it
1) other toolbarbutton-1 rules interfered/ took precedence over rules specifically meant for the zoom & edit controls.
2) wasn't necessary in the first place.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +315,5 @@
> >  }
> >  
> > +panelview .toolbarbutton-1,
> > +#widget-overflow-list > toolbarbutton {
> > +  margin-top: 6px;
> 
> I really don't like this. This also means there is dead space inbetween two
> buttons, and clicking on that space will close the panel without hitting any
> of the buttons. Can we get confirmation from UX that this is what they want,
> rather than an artifact of the mockup? Can we come up with another way of
> doing this, like using the padding trick we use in the navbar, and setting
> the background/border on the image instead of the text? I suppose that's
> hard to do with variable-height text... :-\

Apart from bug 934532, I'll needinfo shorlander with this specific question. I'm afraid there'll be no way around this. I'm sure we can come up with something smart that can make this margin possible.

> @@ +408,5 @@
> > +  border: 1px solid;
> > +  border-color: hsla(210,4%,10%,0);
> > +  border-bottom-color: hsla(210,4%,10%,.1);
> > +  padding: 0;
> > +  -moz-margin-start: -3px;
> 
> Huh, yeah, I missed this. Not a fan. I have to confess I don't understand
> your explanation for it, either. Why not update the styles of the buttons to
> not be so wide that they don't fit? How is this solved in the design/mockup?

In the mockup this is solved by using <div>s, `display: flex`, `flex: 1` for toolbarbuttons (cut, copy, paste) and negative margins to correct things. You can see this in action at http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows7.html.
I did not get this to work in the XUL universe though, which has been mighty frustrating for me. If I could think of any other solution that would grant me the effect to reduce a box' width while it has `flex: 1`, I would've put it here. Unfortunately I didn't.
While I keep on searching for a better solution, please feel free to ping me in the mean time if you come up with something that (might) work!

> @@ +471,5 @@
> > +  background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0)),
> > +                    linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.15) 40%, hsla(210,54%,20%,.15) 60%, hsla(210,54%,20%,0)),
> > +                    linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0));
> > +  background-size: 1px, 1px, 1px;
> > +  background-position: top left, top center, top right;
> 
> I think what I meant was, if this is really just: 0 0, 1px 0, 2px 0, is it
> necessary? Don't the background automatically get ordered
> side-by-side-horizontally, and/or is there a simpler way to do that? If not,
> can we at least use the px values?

It doesn't get centered automatically and I don't know of a better way.
Sure, I can use the px values, but is that really that much clearer? Or does it perform better?

	Mike de Boer [:mikedeboer] Assignee
	Comment 18 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #15)
> @@ +450,5 @@
> >  #edit-controls@inAnyPanel@ > #paste-button:-moz-locale-dir(rtl),
> >  #zoom-controls@inAnyPanel@ > #zoom-out-button:-moz-locale-dir(ltr),
> >  #zoom-controls@inAnyPanel@ > #zoom-in-button:-moz-locale-dir(rtl) {
> > +  border-top-right-radius: 0;
> > +  border-bottom-right-radius: 0;
> 
> Wat. These used to be explicitly radiused, now they're explicitly
> non-radiused? Why? If none of these are meant to have a radius (which I
> think is the case), then please use much simpler selectors to turn it off
> specifically for these nodes, or don't include the edit/zoom controls in
> with the other stuff which gets a radius.

The buttons inside the zoom and edit controls now inherit the styles for generic buttons in the panel rules, including the `border-radius: 2px`. Before this, the situation was the reverse (no border-radius), so the logic here needs to be reversed as well and still be correct for RTL. Hence this change and I strongly feel this ought to stay.

	Mike de Boer [:mikedeboer] Assignee
	Comment 20 • 11 years ago

I'll ask Stephen about the margin between the panel buttons on IRC directly.

	Mike de Boer [:mikedeboer] Assignee
	Comment 21 • 11 years ago

[15:39:59] <shorlander> mikedeboer: yes, the margin is intentional

	:Gijs (he/him)
	Comment 22 • 11 years ago

Comment on attachment 827360 [details] [diff] [review]
Patch 2.3: update the panel styling for the zoom and edit controls

Review of attachment 827360 [details] [diff] [review]:
-----------------------------------------------------------------

This LGTM, apart from the following two small nits. Please doublecheck the width/min-width thing for the separator, and adjust accordingly.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +594,5 @@
>        }
>  
>        this.notifyListeners("onWidgetBeforeDOMChange", widgetNode, null, container, true);
>  
> +      this.removeLocationAttributes(widgetNode);

Please add a comment explaining why this is necessary (because in customize mode, CustomizeMode.jsm may reappend this node to the palette even if the if condition is true and we removeChild the node here).

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +434,5 @@
> +#edit-controls > separator,
> +#zoom-controls > separator {
> +  -moz-appearance: none;
> +  width: 3px;
> +  min-width: 3px;

Do we need both of these?

	:Gijs (he/him)
	Comment 23 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #22)
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +594,5 @@
> >        }
> >  
> >        this.notifyListeners("onWidgetBeforeDOMChange", widgetNode, null, container, true);
> >  
> > +      this.removeLocationAttributes(widgetNode);
> 
> Please add a comment explaining why this is necessary (because in customize
> mode, CustomizeMode.jsm may reappend this node to the palette even if the if
> condition is true and we removeChild the node here).

Am I right in thinking this fixes bug 930950 already? Nice. :-)

(also, did you mean to mark 2.2 obsolete or was there some reason not to mark it as such when uploading this version?)

	Mike de Boer [:mikedeboer] Assignee
	Comment 24 • 11 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> Am I right in thinking this fixes bug 930950 already? Nice. :-)

Yes it does :) I just HAD to fix this when I spotted the bug for these controls.

> (also, did you mean to mark 2.2 obsolete or was there some reason not to
> mark it as such when uploading this version?)

Done! I didn't notice it before :S

Thanks for the reviewing action!

	Dão Gottwald [:dao]
	Comment 25 • 11 years ago

Comment on attachment 825920 [details] [diff] [review]
Patch 1.1: introduce the final, flat panel UI style

> .panel-arrow[side="top"] {
>-  list-style-image: url("chrome://global/skin/arrow/panelarrow-vertical.png");
>+  list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical.png");
>   margin-left: 6px;
>   margin-right: 6px;
>   margin-bottom: -2px;
> }

You're making panelarrow-vertical.png and others unused. You should remove them from the tree, and probably rename panelarrow-light-vertical.png to panelarrow-vertical.png.

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (The lighter panel style on OSX is required for the flat button style,
> because the darker panel made the flat buttons very hard to discern).

I don't think this is reason enough to change all arrow panels. Was there a more conscious decision behind that?

	Mike de Boer [:mikedeboer] Assignee
	Comment 26 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #25)
> I don't think this is reason enough to change all arrow panels. Was there a
> more conscious decision behind that?

I think so - derived from the fact that the identity popup was recently implemented with the lighter style. But the exact reasoning/ decision I do not know; I'll ask Stephen for a ui-review.

	Mike de Boer [:mikedeboer] Assignee
	Comment 27 • 11 years ago

Comment on attachment 825920 [details] [diff] [review]
Patch 1.1: introduce the final, flat panel UI style

Stephen, is it your intention to make the lighter panel style the default style on OSX for all panels?

(I assumed so, because newer versions of OSX don't have these darker panels anymore).

	Mike de Boer [:mikedeboer] Assignee
	Comment 28 • 11 years ago

Comment on attachment 825920 [details] [diff] [review]
Patch 1.1: introduce the final, flat panel UI style

Clearing ui-r?shorlander, because I discussed this directly with him on IRC:

> <mikedeboer> shorlander: will all panels on OSX use the lighter style like with the identity popup and MenuPanel?
>> <shorlander> mikedeboer: all panels on OSX should already be light on OSX, they just aren't the right light color: i.e. http://cl.ly/image/0q1U182Q0W2V
> <mikedeboer> shorlander: that's what I mean... they should all be that color, right?
> <mikedeboer> shorlander: (currently only the identity popup uses that lighter color)
>> <shorlander> mikedeboer: they all look the same to me, currently
>> <shorlander> mikedeboer: but yes, they should be consistent
> <mikedeboer> shorlander: uh, oh. you're right! ok, I take that back then :) so, they should all be lighter.
>> <shorlander> yes :)
> <mikedeboer> consider it done, sir.

Green light.

	Mike de Boer [:mikedeboer] Assignee
	Comment 30 • 11 years ago

sorry, uploaded the wrong patch. THIS is 1.2.

	Mike de Boer [:mikedeboer] Assignee
	Comment 31 • 11 years ago

Dão, review ping?

	Dão Gottwald [:dao]
	Comment 32 • 11 years ago

Comment on attachment 831653 [details] [diff] [review]
Patch 1.2: introduce the final, flat panel UI style

> .panel-subviews {
>   background-image: linear-gradient(to bottom, white 1px, rgba(255, 255, 255, 0) 15px);
>+%ifdef XP_MACOSX
>+  background-color: #f5f5f5;
>+%else
>   background-color: -moz-dialog;
>+%endif
>   box-shadow: -1px 0px 0px rgba(0, 0, 0, 0.2), -1px 0px 2px rgba(0, 0, 0, 0.1), 1px 0px 0px rgba(255, 255, 255, 0.2) inset;
>   -moz-margin-start: @exitSubviewGutterWidth@;
> }

Move this outside of shared/ instead of using ifdef.

> panelview toolbarbutton,
>+%ifdef XP_MACOSX
>+panelview .toolbarbutton-1:not([type="menu-button"]),
>+%endif
> #widget-overflow-list > toolbarbutton,
> .customizationmode-button,
> #BMB_bookmarksPopup > menu,
> #BMB_bookmarksPopup > menuitem {

'panelview .toolbarbutton-1:not([type="menu-button"])' seems redundant, given 'panelview toolbarbutton'. Also, why is this OS-X-specific?

	Mike de Boer [:mikedeboer] Assignee
	Comment 33 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #32)
> > panelview toolbarbutton,
> >+%ifdef XP_MACOSX
> >+panelview .toolbarbutton-1:not([type="menu-button"]),
> >+%endif
> > #widget-overflow-list > toolbarbutton,
> > .customizationmode-button,
> > #BMB_bookmarksPopup > menu,
> > #BMB_bookmarksPopup > menuitem {
> 
> 'panelview .toolbarbutton-1:not([type="menu-button"])' seems redundant,
> given 'panelview toolbarbutton'. Also, why is this OS-X-specific?

The style for buttons inside the panel have a border now (`border: 1px solid`), but on OSX this specific selector negates that border. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#395.

I have/ had three choices:
1) remove the `border: 0` from osx/browser.css and go regression hunting
2) add the 'offending' selector to the panelview toolbarbutton selectors inside an %ifdef
3) put an `!important` after the `border: 1px solid` rule.

What do you think? I there a choice I missed? Which is a good/ better option?

	Dão Gottwald [:dao]
	Comment 34 • 11 years ago

(1) sounds preferable.

	Dão Gottwald [:dao]
	Comment 36 • 11 years ago

Comment on attachment 8336039 [details] [diff] [review]
Patch 1.3: introduce the final, flat panel UI style

> .toolbarbutton-1:not([type="menu-button"]),
> .toolbarbutton-1 > .toolbarbutton-menubutton-button,
> .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
> #restore-button {
>   -moz-box-orient: vertical;
>   height: 22px;
>   padding: 0;
>+}
>+
>+toolbar .toolbarbutton-1:not([type="menu-button"]),
>+.toolbarbutton-1 > .toolbarbutton-menubutton-button,
>+.toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
>+#restore-button {
>   border: 0;
> }

Why apply the toolbar restriction only to .toolbarbutton-1:not([type="menu-button"]), and why only for border?

	Mike de Boer [:mikedeboer] Assignee
	Comment 37 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #36)
> Why apply the toolbar restriction only to
> .toolbarbutton-1:not([type="menu-button"]), and why only for border?

Good point, the toolbar restriction can apply to all rules here. The reason I didn't make them apply for `.toolbarbutton-1 > .toolbarbutton-menubutton-button` and `.toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker` is for (perhaps premature) optimization due to the descendant selector.

	Mike de Boer [:mikedeboer] Assignee
	Comment 38 • 11 years ago

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/2a99c5feb322
https://hg.mozilla.org/integration/fx-team/rev/01e7d117df29

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2a99c5feb322
https://hg.mozilla.org/mozilla-central/rev/01e7d117df29

	Cornel Ionce [:noni] [Hubs QA]
	Comment 40 • 10 years ago

Verified fixed on Windows 7 64bi, Ubuntu 12.04 32bit, Mac OS X 10.9 and Windows 8.1 64bit (MS Pro 2 - hiDPI):
- Fx 29 beta 8 (build ID: 20140414143035)
- Latest Aurora (build ID: 20140415004003)
- Latest Nightly (build ID: 20140415030203).