877684 - Implement Customize and Help button styling, and disable Help button in customize mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) Enter customization mode
2) Click on "Help" in the panel
3) Watch helplessly as the menu contents slide to the left

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I can't reproduce this on the 5-30 UX Nightly Win7 or on a local build with b9228ba05437 at the tip.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I still can. :/

http://screencast.com/t/6N30eSks

Comment 3

[Mike Conley (:mconley) (:⚙️)]

That was with last night's UX Nightly - Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130531 Firefox/24.0

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #2)
> I still can. :/
> 
> http://screencast.com/t/6N30eSks

Thank you for the screencast. I didn't see this because I access Customize mode from pressing "Alt, V, T, C" to get to customize mode, which apparently doesn't run some of the panel initialization code paths.

Entering customization mode this way, then clicking on Help, then exiting customization mode and clicking on the menu panel shows a very short panel with the Help subview selected.

I was able to reproduce the comment #2 described-bug by following the same steps in the screencast.

Comment 5

[:Gijs (he/him)]

What is the desired end-state here? Should it just not do anything? We disable all the menus, and when you enter customization mode and then see an option called "help", you might think it helps you with customization mode, which it doesn't.

So, personally, I think we should just hide the help item when in customization mode, which happily should also be trivial to implement.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #5)
> when you enter customization mode and then see an
> option called "help", you might think it helps you with customization mode,
> which it doesn't.
> 
> So, personally, I think we should just hide the help item when in
> customization mode, which happily should also be trivial to implement.

Hm, that's a good point which I hadn't thought about. Let's go with the simple solution of just hiding the Help button while in customization mode.

Comment 7

[:Gijs (he/him)]

Attachment: Patch

Alright, hidden it is.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 758199 [details] [diff] [review]
Patch

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

Sorry, about misleading you. http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html shows that the Help button should look disabled when in customization mode.

Comment 9

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #8)
> Comment on attachment 758199 [details] [diff] [review]
> Patch
> 
> Review of attachment 758199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, about misleading you.
> http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/
> windows7-customizationMode-i02.html shows that the Help button should look
> disabled when in customization mode.

OK. Is the button supposed to look like that on OS X / Linux, too? shorlander's people dir doesn't have an index so I couldn't quickly find an existing customization mockup for OS X / Linux.

Comment 10

[u428464]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Jared Wein [:jaws] from comment #8)
> > Comment on attachment 758199 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 758199 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry, about misleading you.
> > http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/
> > windows7-customizationMode-i02.html shows that the Help button should look
> > disabled when in customization mode.
> 
> OK. Is the button supposed to look like that on OS X / Linux, too?
> shorlander's people dir doesn't have an index so I couldn't quickly find an
> existing customization mockup for OS X / Linux.

Haven't found a linux mockup, but here's the OSX one : http://cl.ly/image/1a0p1G1g3f0g .

Comment 11

[u428464]

Sorry but in fact I've found a Linux mockup : http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/australis-liveDemo-linux.html

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Yes, we want this for M7.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 15

[Justin Dolske [:Dolske]]

P1 based on new UI + prominent "Help" link... Breaking the UI when a user is trying to get help is bad times. :(

Comment 16

[:Gijs (he/him)]

Attachment: WIP

This is a tentative go at how this would work. I'm using red placeholders for the images, which should be added in bug 875488. Jared, does this look more or less OK to you?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 767234 [details] [diff] [review]
WIP

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

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +15,5 @@
>        <vbox id="PanelUI-mainView-spring" flex="1"/>
>  
> +      <footer class="PanelUI-footer">
> +        <toolbarbutton id="PanelUI-customize-btn" label="&appMenuCustomize.label;"
> +                       oncommand="gCustomizeMode.toggle()" flex="1000"/>

Why flex=1000, why not flex=1?

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +55,5 @@
>  #PanelUI-menu-button {
>    list-style-image: none;
>  }
>  
> +#PanelUI-contents .toolbarbutton-text {

Please use child selector here.

@@ +208,5 @@
>  }
>  
> +#PanelUI-help-btn,
> +#PanelUI-customize-btn {
> +  margin: -1px 0 0;

Why is the -1px top margin here necessary?

@@ +212,5 @@
> +  margin: -1px 0 0;
> +  padding: 12px;
> +  -moz-appearance: none;
> +  box-shadow: none;
> +  background: transparent;

Please don't use the shorthand property for background, as it is easy to set values that aren't intended and hard to find what rules are being set.

@@ +213,5 @@
> +  padding: 12px;
> +  -moz-appearance: none;
> +  box-shadow: none;
> +  background: transparent;
> +  border: 0 none;

border: none;

@@ +222,5 @@
> +#PanelUI-help-btn {
> +  border-left: 1px solid transparent;
> +}
> +#PanelUI-customize-btn {
> +  border-right: 1px solid transparent;

These won't work for RTL. You should use -moz-border-start and -moz-border-end.

@@ +245,5 @@
>    display: none;
>  }
>  
> +#PanelUI-customize-btn > .toolbarbutton-text {
> +  text-align: left;

text-align: start;

@@ +252,5 @@
> +#PanelUI-customize-btn > .toolbarbutton-icon,
> +#PanelUI-help-btn > .toolbarbutton-icon {
> +  height: 16px;
> +  width: 16px !important;
> +  background-color: red;

This is just for testing, right? :)

I think that exact red, rgb(255,0,0), is probably not what we want to eventually ship here :)

Comment 18

[:Gijs (he/him)]

Attachment: WIP v2

Excellent review, would definitely ask again (wait...).

(In reply to Jared Wein [:jaws] from comment #17)
> > +                       oncommand="gCustomizeMode.toggle()" flex="1000"/>
> 
> Why flex=1000, why not flex=1?

Because I'm an idiot and forgot to take that out after debugging (it was initially not working and I was breaking my head over why, and tried various things - I don't even remember what the real issue turned out to be, but it wasn't this).

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +55,5 @@
> >  #PanelUI-menu-button {
> >    list-style-image: none;
> >  }
> >  
> > +#PanelUI-contents .toolbarbutton-text {
> 
> Please use child selector here.

Done, but this selector is now pretty hideous. Not sure you really want this instead:

#PanelUI-contents > toolbarpaletteitem > toolbaritem > toolbarbutton > .toolbarbutton-text,
#PanelUI-contents > toolbaritem > toolbarbutton > .toolbarbutton-text,
#PanelUI-contents > toolbarpaletteitem > toolbarbutton > .toolbarbutton-text,
#PanelUI-contents > toolbarbutton > .toolbarbutton-text {


> @@ +208,5 @@
> >  }
> >  
> > +#PanelUI-help-btn,
> > +#PanelUI-customize-btn {
> > +  margin: -1px 0 0;
> 
> Why is the -1px top margin here necessary?

Because their top borders on hover should overlap the border of the footer, not be below it. I realize it's a little suboptimal, but I couldn't see a way to satisfy all of these conditions without having the overlap:

1. Have a continuous border on the footer with no hover.
2. Have contents not shift when hovering one of the elements, or hovering from one to the other element

Because you could just use a top border on the elements themselves, but then to get the vertical border on hover to not shift the elements without, you need a transparent border there when they're not hovered. But adding a transparent border in the middle breaks up the top border (which AFAICT is cut off 'diagonally' if the vertical border is transparent, on retina screens).

So I left the top border on the footer, and made its contents overlap the border. An alternative solution *might* be using an inset 1px non-blurred box-shadow for the footer? I haven't tried that, though.

> @@ +212,5 @@
> > +  margin: -1px 0 0;
> > +  padding: 12px;
> > +  -moz-appearance: none;
> > +  box-shadow: none;
> > +  background: transparent;
> 
> Please don't use the shorthand property for background, as it is easy to set
> values that aren't intended and hard to find what rules are being set.

Fixed, and this was a good point because it turns out I'm *actually* overriding a background-image: linear-gradient here (at least on Mac). Oops.

> 
> @@ +213,5 @@
> > +  padding: 12px;
> > +  -moz-appearance: none;
> > +  box-shadow: none;
> > +  background: transparent;
> > +  border: 0 none;
> 
> border: none;
>

Done.

> You should use -moz-border-start and -moz-border-end.
>
> text-align: start;

Fixed.

> 
> @@ +252,5 @@
> > +#PanelUI-customize-btn > .toolbarbutton-icon,
> > +#PanelUI-help-btn > .toolbarbutton-icon {
> > +  height: 16px;
> > +  width: 16px !important;
> > +  background-color: red;
> 
> This is just for testing, right? :)
> 
> I think that exact red, rgb(255,0,0), is probably not what we want to
> eventually ship here :)

Yup. Still no shorlander, so no icons, so left this for now.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 772015 [details] [diff] [review]
WIP v2

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +238,5 @@
> +#PanelUI-customize-btn > .toolbarbutton-icon,
> +#PanelUI-help-btn > .toolbarbutton-icon {
> +  height: 16px;
> +  width: 16px !important;
> +  background-color: red;

The customize-button can use the new dummy icon that is in the top-left corner of http://hg.mozilla.org/projects/ux/raw-file/04a9719e7057/browser/themes/windows/Toolbar.png.

The help button can use chrome://global/skin/icons/question-16.png.

We don't need to wait for a proper icon from Stephen before landing this. That can be handled in a follow-up.

Comment 20

[:Gijs (he/him)]

Attachment: Patch v1

OK, done, but I used menuPanel-small.png instead because its size fits better.

Another thing is that I noticed we had a Windows-only selector which added end-margin to labels in toolbarbutons - so I removed the label from the help button, seeing as we never display it anyway, and only left it as an aria-label for accessibility reasons.

Also, I realized I had to add a disabled style to the help button, because we override that styling for buttons in customization mode...

For these 2.5 things, quickly re-requesting review. :-)

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 773256 [details] [diff] [review]
Patch v1

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +206,5 @@
> +  transition: background-color;
> +}
> +#PanelUI-help-btn {
> +  -moz-border-start: 1px solid transparent;
> +  list-style-image: url(chrome://global/skin/icons/question-16.png);

I should have said something in the previous review, but we'll need to do something here for HiDPI.

@@ +245,5 @@
> +#PanelUI-customize-btn > .toolbarbutton-icon,
> +#PanelUI-help-btn > .toolbarbutton-icon {
> +  height: 16px;
> +  width: 16px;
> +}*/

Why is this commented out? Can it just be deleted?

Comment 22

[:Gijs (he/him)]

Attachment: Patch v1.1

This time with hidpi support + the commented out block removed.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 773501 [details] [diff] [review]
Patch v1.1

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

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +226,5 @@
> +  box-shadow: none;
> +}
> +
> +#PanelUI-help-btn:not([disabled]):hover {
> +  -moz-border-start: 1px solid rgba(8, 25, 42, 0.2);

nit, no spaces after commas inside of a color macro. rgba(8,25,42,0.2);

this makes it easier to see when color stops are used in gradients, and so as a convention it is better to use this style in all places.

Comment 24

[:Gijs (he/him)]

Attachment: Patch v1.2

Fixed the colors, but also properly fixed the width: 16px bit this time. :|

Comment 25

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/a39e19167f5e

Filed bug 892076 for the icon update.

Comment 26

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/a39e19167f5e