975111 - No option to Relaunch in Metro mode from old-style menubar

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P1)

Description

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

STR:

1) Open Firefox in a new profile on Windows 8, and enable the menubar (Firefox button > Options > Menu Bar)
2) Attempt to enter Metro mode from the newly exposed menu bar

ER:

There should be a menuitem in the menubar to allow the user to go to Metro mode.

AR:

There is no such menuitem, at least, not that I can see.

Filed on behalf of matej, who I am Cc'ing.

Comment 1

[Marina Samuel [:emtwo]]

Attachment: v1: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

Comment 2

[Matej Novak [:matej]]

(In reply to Marina Samuel [:emtwo] from comment #1)
> Created attachment 8381801 [details] [diff] [review]
> v1: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

Is this language deliberately different? In the Firefox menu, it says "Relaunch in Firefox for Windows 8 Touch." Unless there's a good reason, I think we should be consistent.

I also wonder if it needs to say "Beta" somewhere. In Aurora it says "Relaunch in Aurora for Windows 8 Touch."

Should this, perhaps, say "Relaunch in Firefox Beta for Windows 8 Touch" (of course, then it would have to be changed in both places)?

Thanks.

Comment 3

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

(In reply to Matej Novak [:matej] from comment #2)
> (In reply to Marina Samuel [:emtwo] from comment #1)
> > Created attachment 8381801 [details] [diff] [review]
> > v1: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+
> 
> Is this language deliberately different? In the Firefox menu, it says
> "Relaunch in Firefox for Windows 8 Touch." Unless there's a good reason, I
> think we should be consistent.

We are - I suspect Marina was just giving a precis of the change.

The string that she's using is the exact same one being used in the Firefox menu - specifically:

"Relaunch in &brandShortName; for Windows 8 Touch"

> 
> I also wonder if it needs to say "Beta" somewhere. In Aurora it says
> "Relaunch in Aurora for Windows 8 Touch."
> 
> Should this, perhaps, say "Relaunch in Firefox Beta for Windows 8 Touch" (of
> course, then it would have to be changed in both places)?
> 

At this point, Beta branding is essentially identical to Release. The update channel is different, but appearance-wise, they're not distinguishable. I'm not sure if we want to switch Beta to be more distinguishable, but even if we did, changing strings on the Beta channel is a no-can-do localization-wise. I suspect that's the case even for branding.

Comment 4

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

Comment on attachment 8381801 [details] [diff] [review]
v1: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

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

This is fine - just please change the ID of the menuitem so that we don't collide. Thanks Marina!

::: browser/base/content/browser-menubar.inc
@@ +40,5 @@
>                            accesskey="&openFileCmd.accesskey;"/>
> +#ifdef HAVE_SHELL_SERVICE
> +#ifdef XP_WIN
> +#ifdef MOZ_METRO
> +                <menuitem id="switch-to-metro"

The id being used for the appmenu button menuitem also has the ID "switch-to-metro" - ID collision is not a thing we should do. I suggest calling this something like "menu_switchToMetro" instead.

Comment 5

[Matej Novak [:matej]]

Thanks for the extra context. Looks god to me.

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

Made id change and carrying over r+ from mconley

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 924914

User impact if declined: Users who are familiar with and often use the menubar will not easily find a "switch to metro mode" option

Testing completed (on m-c, etc.): tested locally on beta branch

Risk to taking this patch (and alternatives if risky): low risk, adding a button with already existing functionality to the menubar

String or IDL/UUID changes made by this patch: none

Comment 7

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

Comment on attachment 8382383 [details] [diff] [review]
v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

Going ahead and requesting this for Aurora as well, to make sure it doesn't slip through the cracks.

Comment 8

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

Comment on attachment 8382383 [details] [diff] [review]
v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

So I just talked to matej about this, and because (at least for now) Australis is riding Aurora, we might not want to land this patch there.

The reasoning being that we're only putting this item in the menubar because on Windows, if the menubar is enabled, the appmenu button is gone, and there's no way to get to Metro mode.

With Australis, the menubar being enabled has no bearing on the availability of the Metro toggle in the menu panel.

So, withdrawing my request for Aurora unless there's any objections here.

Marina - you will, however, probably want to land this change on Holly, in the event that Australis does not ride the 29 train.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Marina, any reason why this didn't land in m-c before? thanks

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8382383 [details] [diff] [review]
v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

OK - approving this for the FF28 beta and leaving the rest tracked so we can have this on our radar just in case we don't put Australis to Beta.  To be clear, nothing further is needed if Australis does go forward?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/669b486de0fd

Comment 12

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

(In reply to Lukas Blakk [:lsblakk] from comment #10)
> Comment on attachment 8382383 [details] [diff] [review]
> v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+
> 
> OK - approving this for the FF28 beta and leaving the rest tracked so we can
> have this on our radar just in case we don't put Australis to Beta.  To be
> clear, nothing further is needed if Australis does go forward?

That's correct - and is also why this didn't land on Nightly (this patch is not needed for builds with Australis).

Comment 13

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

So I just got off the horn with shorlander in UX, and I think we're reversing this decision here.

We're going to go for landing this on post-Australis builds as well.

Sorry for the thrashing, y'all.

Comment 14

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

Comment on attachment 8382383 [details] [diff] [review]
v2: Add "Relaunch in Windows 8 Touch" button to the menu bar in beta+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 924914

User impact if declined: Users who are familiar with and often use the menubar will not easily find a "switch to metro mode" option

Testing completed (on m-c, etc.): tested locally on beta branch

Risk to taking this patch (and alternatives if risky): low risk, adding a button with already existing functionality to the menubar

String or IDL/UUID changes made by this patch: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was landed on fx-team, though the bug wasn't marked as such.
https://hg.mozilla.org/integration/fx-team/rev/2542878529f8

Comment 16

[u279076]

Does this have or need in-testsuite tests?

Comment 17

[Marina Samuel [:emtwo]]

Landed in holly:
https://hg.mozilla.org/projects/holly/rev/7b1567b25dc9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2542878529f8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/669b486de0fd

Comment 20

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49159873c740

Comment 22

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-03-03-02-01-mozilla-central/

Before verification, reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-20-03-02-02-mozilla-central/

- Ensured that the position is consistant throughout all the channels (Nightly, Aurora & BETA)
- Ensured that the wording is consistant throughout all the channels (Nightly, Aurora & BETA)
- Ensured that selecting the menu item correctly launches fxmetro
- Ensured that you can switch back and forth between both fxmetro and fxdesktop
- Went through all of the above test cases in several different variations of snapped view
- Went through all of the above test cases using both the X1 Carbon and the Surface Pro 2