868636 - Implement Help button in the panel menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Matthew N. [:MattN]]

Attachment: Zhenshuo's 20121212 exploration

Figure out where to put the items in the help item of the menubar and Firefox button.

Comment 1

[u428464]

FWIW :

Zhenshuo Fang (:fang) - Firefox UX Team comment #6

> Thank you for bringing up the issue, I almost forgot I need to follow up
> with the SUMO team on this! I talked to them a while ago about "Help" in
> Australis and here's the thought process: 
> 
> In Windows Help menu I have: 
> - Help (SUMO Page)
> - Getting Started
> - Troubleshooting
> - Submit Feedback
> - Restart w/ add-on disabled
> - About Firefox
> On Mac there's no "Getting started" but there's "Report web forgery". And in
> nightly there's "Firefox Health Report".
> 
> Originally the plan was: 
> 1. "Help" button just open the SUMO page
> 2. A separate info icon for "About Firefox" in the panel
> 3. Remove "Getting Started" and "report web forgery" 
> 4. Remove "submit feedback" for now and maybe make them in-context in the
> future (see shorlander's mock-up http://cl.ly/image/3q2k2x1D0y0a0N0e3A0Q/o)
> OR put the smiley/sad face in the panel if it's not a good idea to remove
> "submit feedback".
> 5.Remove "troubleshooting" and "restart without add-ons" (because they are
> <2% features and most users access them through SUMO page)
> 
> The SUMO team agree with 1-4 but has some concerns on 5 and don't think
> removing troubleshooting funcitons is a good idea. So in the meantime we can
> either make this a terribly looking drop-down, or move Help up into the
> Panel and use the subview structure.
> 
> Example: http://cl.ly/image/1m3L092f0C2j The left one is the original plan,
> the right one is the terribly looking dropdown.
> Note: Unfocused brought up a good point that we need "exit" button in
> Windows, that's why in the mock-up "Customize" is up in the main panel. But
> moving Help up instead might make more sense.

Comment 2

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

I'll get the ball rolling tomorrow/tonight on this.

Comment 3

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

The Help button here is supposed to function like a subview widget. Clicking on it will show a subview that has the same Help items that are in the Help submenu in the Firefox App menu.

The location of the Help button shouldn't change though, it should still be at the bottom of the panel.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: New Help subview panel

Jared, the panel and its items show up properly, but what is the desired behavior when you click an item? And how to implement that on top of the already defined action for those items? Event listeners in JS?

Comment 5

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

Comment on attachment 748913 [details] [diff] [review]
New Help subview panel

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

Everything seems to be working fine except that the panel isn't dismissed when clicking on one of these. Also, the "This isn't a web forgery..." menuitem is shown when it shouldn't be (I believe it is only shown on pages that were deemed web forgeries, but it shows up on about:home for me).

We also need to tweak the CSS rules so that .panel-multiview-achor has more precedence than #PanelUI-help-btn.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: New Help subview panel

Comment 7

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

Comment on attachment 749211 [details] [diff] [review]
New Help subview panel

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

::: browser/components/customizableui/content/panelUI.js
@@ +59,5 @@
> +      let NSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +      let _slice = Array.prototype.slice;
> +
> +      // Help menuitems change every so often, which means we need to observe that
> +      let obs = new MutationObserver(function(mutations) {

This seems overly complex to just get the right menuitems in this list. How does the Alt+H help menu on Windows/Linux work? Can the current menuitems from that menu be copied when the user clicks on the Help button?

You can use http://www.mozilla.org/firefox/its-a-trap.html to test a web forgery site.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +225,5 @@
>  #PanelUI-historyItems > toolbarbutton {
>    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
>  }
>  
> +#PanelUI-help-btn.panel-multiview-anchor,

I think it looks better when the #footer and #PanelUI-help-btn get the panel-multiview-anchor class, here's a screenshot: http://screencast.com/t/temO3kCYXpE

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: New Help subview panel

Comment 9

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

Comment on attachment 749812 [details] [diff] [review]
New Help subview panel

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

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +45,5 @@
>        <vbox id="PanelUI-mainView-spring" flex="1"/>
>  
>        <footer class="PanelUI-footer" align="center">
>          <toolbarbutton id="PanelUI-help-btn" label="&helpMenu.label;"
> +                       onclick="PanelUI.showHelpView(this.parentNode);"/>

Might be good to include a comment here stating that the parentNode is used so that the footer is presented as the anchor instead of just the button being the anchor.

::: browser/components/customizableui/content/panelUI.js
@@ +40,5 @@
>      for (let event of this.kEvents) {
>        this.panel.addEventListener(event, this);
>      }
> +
> +    this.helpView.addEventListener("ViewShowing", function PUI___ViewShowing(aEvent) {

This event listener doesn't appear to be removed.

@@ +50,5 @@
> +      let attrs = ["oncommand", "onclick", "label", "key", "disabled"];
> +      let NSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +      // Button onclick handler which hides the whole PanelUI
> +      function onButtonClick(e) {

You should be able to move this event listener to this.helpView and check that (e.button == 0 && !e.target.hasAttribute("disabled")).

I think you can use 'target' here instead of 'originalTarget'. Also would be nice to remove the event listener when the view is hidden.

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: New Help subview panel

Jared, I'm asking you for another review, because I moved quite a substantial amount of code around

Comment 12

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

Comment on attachment 749935 [details] [diff] [review]
New Help subview panel

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

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +43,5 @@
>  
>        <vbox id="PanelUI-mainView-spring" flex="1"/>
>  
>        <footer class="PanelUI-footer" align="center">
> +        <!-- The parentNode is used so that the footer is presented as the anchor 

nit, trailing whitespace

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +219,5 @@
> +panelmainview toolbarbutton[disabled],
> +panelsubview toolbarbutton[disabled] {
> +  text-shadow: 0 1px #fff;
> +  opacity: .5;
> +}

I think we can do without these rules. toolbarbutton[disabled] already gives us color:graytext, and lowering the opacity on the toolbarbutton further may make it too hard to read. Is this about the icon? If so, we could add a special rule that is for the icon to have a lower opacity.

Comment 13

[Mike de Boer [:mikedeboer]]

Jared: (In reply to Jared Wein [:jaws] from comment #12)
> 
> I think we can do without these rules. toolbarbutton[disabled] already gives
> us color:graytext, and lowering the opacity on the toolbarbutton further may
> make it too hard to read. Is this about the icon? If so, we could add a
> special rule that is for the icon to have a lower opacity.

I added these rules specifically, because the visual difference wasn't there. color:graytext is also the main button text color, so this doesn't show the button as 'disabled'.

These rules have the added effect that the copy/paste buttons look disabled when they actually are.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> I added these rules specifically, because the visual difference wasn't
> there. color:graytext is also the main button text color,

Is it? On what OS? This sounds like a bug, specifically for what you're observing now: it's faint (usually used to bring across that something is disabled) and reducing the opacity further for the actual disabled state hinders legibility.

Comment 15

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

Mike, please move your disabled CSS changes to another bug as more investigation will be needed. I was not able to see any issues on my local machine when I had a disabled toolbarbutton in a subview (which is what your patch should be trying to address).

Fixing the disabled state of the cut/copy/paste buttons isn't part of this bug.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: New Help subview panel

removed disabled state CSS and trailing space in comment. Carrying over r=jaws

Comment 17

[:Gijs (he/him)]

Pushed: http://hg.mozilla.org/projects/ux/rev/51471a039200

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/51471a039200