871203 - Add Exit button to panel menu

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We need an exit button somewhere. On OSX we might be able to get away without it, as it's in the system global menu. On Windows and Linux there is no such thing - convention is to add it in the appplication's menus. Linux at least has a convention for a shortcut (Ctrl-Q) that Windows lacks, but not everyone uses shortcuts.

At the very least, no exit button breaks session restore for anyone using multiple browser windows.

Comment 1

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #0)
> We need an exit button somewhere. On OSX we might be able to get away
> without it, as it's in the system global menu. On Windows and Linux there is
> no such thing - convention is to add it in the appplication's menus. Linux
> at least has a convention for a shortcut (Ctrl-Q) that Windows lacks, but
> not everyone uses shortcuts.

That's the Linux shortcut for quitting the entire application, right? You mean the a menu item in the 'File' menu to exit the app? (I've seen that in Windows, at least).

> 
> At the very least, no exit button breaks session restore for anyone using
> multiple browser windows.

I assume you mean a button which, once clicked, quits Firefox, no? Will it be draggable to the toolbars? Will it be removable?

Do you know of an example or mockup that shows how this might look like? I have a feeling that such a thing will provide me with the context I need...

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> That's the Linux shortcut for quitting the entire application, right? You
> mean the a menu item in the 'File' menu to exit the app? (I've seen that in
> Windows, at least).

Yep.

> I assume you mean a button which, once clicked, quits Firefox, no?

Yep.

> Will it
> be draggable to the toolbars? Will it be removable?
> 
> Do you know of an example or mockup that shows how this might look like? I
> have a feeling that such a thing will provide me with the context I need...

AFAIK, there is no mockup or spec. Personally, I'd like to have it as a static (non-customizable) button at the bottom of the panel. But Help and Customize are already there, so adding a third item might make that area a bit too busy.

Either way, it needs UX input.

Comment 3

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: A mockup

On Windows(at least on win7) user can quit an application by right click on the doc thing (on the right of Windows Start, not sure what to call it) "Exit" is not used very often(we don't have any data though), maybe we shouldn't surface it to the menu panel? 

If we *really* want to do it, I kind of have a mock-up. We can put three things(exit, help, customize) on the bottom, see mock-up on the left. But this makes the UI a bit cluttered. Or we can make "customize" a widget but make it not removable, which is not ideal either...

Comment 4

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

(In reply to Blair McBride [:Unfocused] from comment #0)
> At the very least, no exit button breaks session restore for anyone using
> multiple browser windows.

Is it possible that we could just fix our session restore so that windows can be closed individually and it will still work? I remember there being some code to do that already, maybe it just needs some TLC?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #4)
> 
> Is it possible that we could just fix our session restore so that windows
> can be closed individually and it will still work? I remember there being
> some code to do that already, maybe it just needs some TLC?

That sounds more compelling to me, but that is mainly because I don't see THE solution in fang's mockup.

Blair, what do you think?

Comment 6

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] from comment #4)
> (In reply to Blair McBride [:Unfocused] from comment #0)
> > At the very least, no exit button breaks session restore for anyone using
> > multiple browser windows.
> 
> Is it possible that we could just fix our session restore so that windows
> can be closed individually and it will still work? I remember there being
> some code to do that already, maybe it just needs some TLC?

I have also seen code code for something like this before. It's somewhat of a guessing game to know whether the user closed a window because they don't want it or because they are closing all of their windows one-by-one. I believe there are existing bugs on the topic so this probably isn't the best place for hashing out the problems but surprisingly I'm having trouble finding one.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #4)
> Is it possible that we could just fix our session restore so that windows
> can be closed individually and it will still work? I remember there being
> some code to do that already, maybe it just needs some TLC?

That sounds like a fantastic opportunity to get things horribly wrong :) As MattN said, we'd just be guessing - so we'd end up with magical behaviour that would be wrong some times, resulting in a much worse experience.

Still, I think it *is* something exploring eventually... just not for this. IMO its out of scope, and requires more time than we have available to get it right.

(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #3)
> On Windows(at least on win7) user can quit an application by right click on
> the doc thing (on the right of Windows Start, not sure what to call it)
> "Exit" is not used very often(we don't have any data though), maybe we
> shouldn't surface it to the menu panel? 

I don't see that on Windows 8. Even still, I doubt many people use that menu. The standard convention in Windows is to use the main menu in the application (or the close button on the titlebar, which is why I think the above is worth exploring eventually).

(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #3)
> If we *really* want to do it, I kind of have a mock-up. We can put three
> things(exit, help, customize) on the bottom, see mock-up on the left. But
> this makes the UI a bit cluttered. Or we can make "customize" a widget but
> make it not removable, which is not ideal either...

FWIW, my personal choice would be the 3rd mockup there. Move the customize to be just above the bottom area, but not styled like a normal widget (more contextual, at the very least), and add an "Exit button with a full label (to me, just the "X" makes me think it would only close the panel).

Comment 8

[Mike de Boer [:mikedeboer]]

Fang: I'd like to reach to a conclusion here... I saw on IRC that shorlander also worked out some ideas about the close button; is there a final solution/ mockup you can share with us?

Comment 9

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

Please attach a link to shorlander's mockup here if you can find it.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #9)
> Please attach a link to shorlander's mockup here if you can find it.

Awesomebar is awesome: http://cl.ly/image/47140P3k2Q3D & http://cl.ly/image/3q2f2q0R3T1C

Comment 11

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

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

Comment 12

[Justin Dolske [:Dolske]]

P1 on the basis that breaking the ability to session restore multiple windows (see comment 0) is bad.

Comment 13

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: Sketch

This is a sketch of the design we came up with, shorlander is working on the visual design.

Comment 14

[Justin Dolske [:Dolske]]

Can we get a final visual design? Is attachment 773371 [details] basically what we want? If so, let's start implementing and plug in icons/colors as needed.

Comment 15

[Mike de Boer [:mikedeboer]]

Justin, I understand that you'd like to get this done, especially considering this is a P1 bug, however...

I've have never implemented _anything_ from a sketch before (except during hobby-hours, but they don't count), because I believe it to be too costly as a rule.

On top of that, the implementation is as simple as can be; a button that, on command, executes an already defined and used action: exit the browser. The cost is not in the implementation, but rather in the design itself.

And it appears to be non-trivial to get this right, UX-wise, otherwise we'd have resolved this bug successfully a month ago.

I see you assigned this to Dão, while before it was assigned to me and to Blair before that. That would make him the third developer to work on this bug that is trivial to implement... just sayin'.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 786906 [details] [diff] [review]
patch

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

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +21,3 @@
>          <!-- The parentNode is used so that the footer is presented as the anchor
>               instead of just the button being the anchor. -->
> +        <toolbarbutton id="PanelUI-help" label="&helpMenu.label;" tabindex="0"

Why change all the IDs? In any case, this ID conflicts with the help view itself, and will break opening it completely.

Comment 18

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

Comment on attachment 786906 [details] [diff] [review]
patch

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

Gijs is right - PanelUI-help conflicts with the panelview that this button is supposed to open.

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #17)
> Comment on attachment 786906 [details] [diff] [review]
> patch
> 
> Review of attachment 786906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +21,3 @@
> >          <!-- The parentNode is used so that the footer is presented as the anchor
> >               instead of just the button being the anchor. -->
> > +        <toolbarbutton id="PanelUI-help" label="&helpMenu.label;" tabindex="0"
> 
> Why change all the IDs? In any case, this ID conflicts with the help view
> itself, and will break opening it completely.

Because the -btn suffix seems pretty random, other buttons don't have it.

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 21

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

Comment on attachment 786961 [details] [diff] [review]
patch v2

Hey Dao!

A grep has revealed multiple instances of PanelUI-customize-btn and PanelUI-help-btn still in use - check under browser/themes and browser/components/customizableui/src.

Those will need to be updated as well.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to Mike de Boer [:mikedeboer] from comment #15)
> [...]
> I see you assigned this to Dão, while before it was assigned to me and to
> Blair before that. That would make him the third developer to work on this
> bug that is trivial to implement... just sayin'.

Just for the public record... ;) This was my goof, I wasn't looking at the previous assignee history, and so thought this was a bug languishing in Steven's pile that just needed an engineer to push. Didn't mean to yank it away or shuffle it around again.

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch v3

(In reply to Mike Conley (:mconley) from comment #21)
> Comment on attachment 786961 [details] [diff] [review]
> patch v2
> 
> Hey Dao!
> 
> A grep has revealed multiple instances of PanelUI-customize-btn and
> PanelUI-help-btn still in use - check under browser/themes and
> browser/components/customizableui/src.
> 
> Those will need to be updated as well.

I miss mxr...

Comment 24

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

Comment on attachment 788299 [details] [diff] [review]
patch v3

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

(In reply to Dão Gottwald [:dao] from comment #23)
> Created attachment 788299 [details] [diff] [review]
> patch v3
> 
> (In reply to Mike Conley (:mconley) from comment #21)
> > Comment on attachment 786961 [details] [diff] [review]
> > patch v2
> > 
> > Hey Dao!
> > 
> > A grep has revealed multiple instances of PanelUI-customize-btn and
> > PanelUI-help-btn still in use - check under browser/themes and
> > browser/components/customizableui/src.
> > 
> > Those will need to be updated as well.
> 
> I miss mxr...

Yeah, manual grepping does feel a bit barbaric. :/

So this patch doesn't work because of the missing endif resulting in the yellow-XML-parsing-error-of-death. Adding the endif manually, I also noticed that the icons are not centered horizontally over the labels - I think we'd probably want that.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +31,5 @@
> +#ifdef XP_MACOSX
> +                       label="&quitApplicationCmdMac.label;"
> +#else
> +                       label="&quitApplicationCmd.label;"
> +#endif

You're missing an #endif here, which is causing the rest of browser.xul to not get included after pre-processing.

Comment 25

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #24)
> So this patch doesn't work because of the missing endif resulting in the
> yellow-XML-parsing-error-of-death.

Strange, I'm pretty sure I actually tried a local build with this before posting the patch. I wonder what went wrong there.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Mike Conley (:mconley) from comment #24)
> > So this patch doesn't work because of the missing endif resulting in the
> > yellow-XML-parsing-error-of-death.
> 
> Strange, I'm pretty sure I actually tried a local build with this before
> posting the patch. I wonder what went wrong there.

Ah, I tested it on Linux but the missing endif is only destructive on Windows.

Comment 27

[Dão Gottwald [:dao]]

Attachment: patch v4

(In reply to Mike Conley (:mconley) from comment #24)
> I also noticed
> that the icons are not centered horizontally over the labels - I think we'd
> probably want that.

This also wasn't an issue on Linux.

Comment 28

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

Comment on attachment 790358 [details] [diff] [review]
patch v4

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

This looks pretty good to me.

I have two remaining issues, but if you don't want to fix them here, they can be filed as follow-ups. Specifically:

1) I'm concerned that one the platforms where the string is "Exit", when the user is in customize mode, they might accidentally click on this button to leave customize mode, which would result in the entire browser closing. We might want to disable that exit button when in customize mode. Or, at the very least, make it clearer that this will *end the browsing session* instead of leaving customize mode.
2) I think the distance between the icons and the labels are a tad too close, but I think that's some polish that can be fixed later.

Comment 29

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #28)
> Comment on attachment 790358 [details] [diff] [review]
> patch v4
> 
> Review of attachment 790358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good to me.
> 
> I have two remaining issues, but if you don't want to fix them here, they
> can be filed as follow-ups. Specifically:
> 
> 1) I'm concerned that one the platforms where the string is "Exit", when the
> user is in customize mode, they might accidentally click on this button to
> leave customize mode, which would result in the entire browser closing. We
> might want to disable that exit button when in customize mode. Or, at the
> very least, make it clearer that this will *end the browsing session*
> instead of leaving customize mode.

A good icon might help with that. If that's not enough, we can revisit this then.

> 2) I think the distance between the icons and the labels are a tad too
> close, but I think that's some polish that can be fixed later.

Since I was messing with the icons' margin anyway, I added a 2px bottom margin.

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

Comment 30

[:Gijs (out for now; he/him)]

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