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.

I still can. :/ http://screencast.com/t/6N30eSks

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.

Alright, hidden it is.

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.

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

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

Yes, we want this for M7.

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

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

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 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 :)

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

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

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

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.

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

https://hg.mozilla.org/projects/ux/rev/a39e19167f5e Filed bug 892076 for the icon update.