Closed Bug 877684 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M8][Australis:P1])

Attachments

(1 file, 5 obsolete files)

STR:

1) Enter customization mode
2) Click on "Help" in the panel
3) Watch helplessly as the menu contents slide to the left
I can't reproduce this on the 5-30 UX Nightly Win7 or on a local build with b9228ba05437 at the tip.
That was with last night's UX Nightly - Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130531 Firefox/24.0
(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.
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.
(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.
Attached patch Patch (obsolete) — Splinter Review
Alright, hidden it is.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #758199 - Flags: review?(jaws)
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.
Attachment #758199 - Flags: review?(jaws)
(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.
(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 .
Yes, we want this for M7.
Whiteboard: [Australis:M?] → [Australis:M7]
Summary: Clicking on menu panel's "Help" in customization mode causes the panel contents to shift → Implement Customize and Help button styling, and disable both in customize mode
Duplicate of this bug: 878575
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
P1 based on new UI + prominent "Help" link... Breaking the UI when a user is trying to get help is bad times. :(
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #758199 - Attachment is obsolete: true
Attachment #767234 - Flags: feedback?(jaws)
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 :)
Attachment #767234 - Flags: feedback?(jaws) → feedback+
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #767234 - Attachment is obsolete: true
Attachment #772015 - Flags: feedback?(jaws)
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.
Attachment #772015 - Flags: feedback?(jaws) → review+
Attached patch Patch v1 (obsolete) — Splinter Review
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. :-)
Attachment #772015 - Attachment is obsolete: true
Attachment #773256 - Flags: review?(jaws)
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?
Attachment #773256 - Flags: review?(jaws) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
This time with hidpi support + the commented out block removed.
Attachment #773256 - Attachment is obsolete: true
Attachment #773501 - Flags: review?(jaws)
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.
Attachment #773501 - Flags: review?(jaws) → review+
Attached patch Patch v1.2Splinter Review
Fixed the colors, but also properly fixed the width: 16px bit this time. :|
Attachment #773501 - Attachment is obsolete: true
Attachment #773507 - Flags: review?(jaws)
Attachment #773507 - Flags: review?(jaws) → review+
Depends on: 892076
https://hg.mozilla.org/projects/ux/rev/a39e19167f5e

Filed bug 892076 for the icon update.
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M8][Australis:P1][fixed-in-ux]
Depends on: 893032
Summary: Implement Customize and Help button styling, and disable both in customize mode → Implement Customize and Help button styling, and disable Help button in customize mode
Blocks: 892483
https://hg.mozilla.org/mozilla-central/rev/a39e19167f5e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.