934032 - Story - Add View on Metro feature to Desktop Firefox in Australis

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

This bug builds upon Bug 934029 (UX work) to add the actual button code for switch to Metro Firefox.  The actual work for switching is already complete.

p=1

msamuel this is another good one you can start on soon. Just copy one of the other firefox buttons as the asset until you get the ones from Bug 934029.

Comment 1

[Brian R. Bondy [:bbondy]]

Here's code for checking if we are already default:


> shell = Components.classes["@mozilla.org/browser/shell-service;1"].
>   getService(Components.interfaces.nsIShellService);
>
> let isDefault = shell.isDefaultBrowser(false, false);

Note the first param is false because it's not a startup check.
Note the second param is false because we only want to check for http.

Comment 2

[Brian R. Bondy [:bbondy]]

To show the default browser prompt if you're not the default browser do this:
shell.setDefaultBrowser(claimAllTypes, false);

Comment 3

[Brian R. Bondy [:bbondy]]

To show the default browser prompt if you're not the default browser and the user clicks the button, do this:

> shell.setDefaultBrowser(false, false);

The first false will only show the little flyout instead of opening control panel
The second false is because we only want to set the default for the current user.

Comment 4

[Brian R. Bondy [:bbondy]]

All of these changes for this bug should be behind:
#ifdef HAVE_SHELL_SERVICE
#ifdef XP_WIN
#ifdef MOZ_METRO

Comment 5

[Brian R. Bondy [:bbondy]]

After you launch the setDefaultBrowser you won't know if they actually selected us or not.  If they did then you would launch the switch to Metro code from the other bug.
Also if they already have us as default you do that initially.

In preferences, we put a timer like this:
http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.js#l40
Then we update the UI in preferences to not show set as default if they selected us.  
We should do the same here, but with some maximum timeout so we aren't running this timeout forever.  (Every second with a max of let's say 10 seconds).

Upgrading this from the previous p=1 to p=2.
There's quite a few things to do but it's spec'ed out pretty well.

Comment 6

[Brian R. Bondy [:bbondy]]

p += 1 to account for updated UX design.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v1: Switch to Metro with Australis UI

* I used the icon for 'customize' for now.
* All the other patches had to be rebased on the australis branch but I'm only uploading the update 'switch to metro' patch

Comment 8

[Marina Samuel [:emtwo]]

Attachment: australis_ux.png

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 830487 [details] [diff] [review]
v1: Switch to Metro with Australis UI

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

Hey Marina, would you mind getting me a UX build? Pushing your patch queue to try along with an extra changeset like this should do the trick:
https://hg.mozilla.org/projects/oak/rev/3e9597e346e4

The patch looks good overall.
1 thing that I noticed that was missing is a check like this:
http://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#736

we want to only show the button when we're on windows 6.2 and above.
Could you also do a similar additional patch for the 6.2 check for the menu item in bug 934032?


You made the right call by the way by only rebasing locally for the other patches for now. There's an equal chance we'll land our stuff on m-c before Australis.

Comment 10

[Brian R. Bondy [:bbondy]]

Also please ask UX for the asset if you're missing it sliced up or whatever, thanks! :D

Comment 11

[Justin Dolske [:Dolske]]

This seems like a lot of UI realestate, has a normal toolbar button been considered? Presumably this should also be removable (as are normal toolbar buttons) for people who don't intend to use the Metro flavor of Firefox?

Also, note that the panel design is changing a bit (bug 878065, see URL field for mockup).

Comment 12

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Yes, mmaslaney and I thought about using the normal icon size in the panel. We agreed that this switching function is not a contextual command which targets the current tab or window, but a browser-level command. Clicking on the link will close Classic Firefox and switch the users to Metro mode. So we think it should be treated as the same level as "Customize" and "Exit".

Since it's a browser-level command and its position is supposed be fixed, it shouldn't be customized the same way as other commands in the Customization panel. An option to turn the switch ON and OFF is probably better.  

I have to blame MS for not having a clear brand name for Metro. I know the term "Windows 8 style + brand" is long but that's the best way we can display this option without confusion for now.

Comment 13

[Marina Samuel [:emtwo]]

A try build with the changes:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-296db8ec200e/try-win32/firefox-28.0a1.en-US.win32.installer.exe

Comment 14

[Michael Maslaney [:mmaslaney] ([e-mail])]

Talk to Laura Forrest who recommended "Touch Friendly Mode".



Also, the Icon and messaging should be left aligned, not centered. This may change, but will know more after our design critique.

Comment 15

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Windows8_Switch_View_v1a.jpg

Ignore the placement of customize and help.

Comment 16

[Marina Samuel [:emtwo]]

Attachment: v2: Switch to Metro with Australis UI (option 2)

I removed the 'add-ons' button for now (I'm just using its icon). I'm not sure what button we want 'metro mode' to replace

Comment 17

[Marina Samuel [:emtwo]]

Attachment: australis_ux2.png

Comment 18

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: firefox-touch_icons.zip

Attached, Firefox Touch Menu and Toolbar icon set.

Comment 19

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: firefox-touch_icons.zip

Resized.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 831485 [details] [diff] [review]
v2: Switch to Metro with Australis UI (option 2)

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

per IRC we need to explicitly hide the button pre win8

Comment 21

[Marina Samuel [:emtwo]]

Attachment: v2: Switch to Metro with Australis UI (option 2)

Only add switch-to-metro-button if in Windows 8.

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 833124 [details] [diff] [review]
v2: Switch to Metro with Australis UI (option 2)

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +148,5 @@
> +
> +#ifdef XP_WIN
> +#ifdef MOZ_METRO
> +    // Show switch-to-metro-button if in Windows 8.
> +    let isMetro = false;

nit: Please call this isMetroCapable because we're not actually in metro

Comment 23

[Marina Samuel [:emtwo]]

Attachment: v3: Switch to Metro with Australis UI (option 2)

Includes new metro icons \o/

Comment 24

[Marina Samuel [:emtwo]]

Attachment: australis_ux.png

Comment 25

[Brian R. Bondy [:bbondy]]

Landed on oak here:
https://hg.mozilla.org/projects/oak/rev/8c16b0b4853d

Bug 935099 will track the landing on m-c.
When it lands on m-c a new comment will be added here as well with the m-c changeset.
This is being done so we can still use scrumbugs efficiently. 
See bug 935099 for further details.

Comment 26

[Brian R. Bondy [:bbondy]]

jaws: if you land your patches, please separate out the australis specific patch and place [Australis] in the commit message so it will be backed out when merging over to Holly

Marina, looks like we're going to have to land the stuff in bug 924914 on Holly.
Could you put the common functionality back in bug 924914, and then put the australis specific work here?

Sorry I know this is a pain but it's out of our control :)

Patch the patches that are for Holly only with [Holly]

Comment 27

[Marina Samuel [:emtwo]]

I updated bug 924914 with a pre-australis patch and I believe the current patch here should be good for australis.

Comment 28

[Brian R. Bondy [:bbondy]]

marina looks like this is causing a test failure here:
https://tbpl.mozilla.org/?tree=Oak&rev=8c16b0b4853d

Could you take a look?

Comment 29

[Marina Samuel [:emtwo]]

Attachment: address-test-failures

So it looks like the failures were due to the fact that tests were looking for an add-ons button which was replaced with a metro mode button.

This patch puts the add-ons button back where it was, except for when in win8, and it marks the two failing tests to be skipped in Windows. #ifdef doesn't seem to work in the tests so I think windows would need separate tests from these.

For now, here is the try build I'm waiting on: https://tbpl.mozilla.org/?tree=Try&rev=bbd718593fe6 which hopefully should fix the test failures.

Comment 30

[Brian R. Bondy [:bbondy]]

Comment on attachment 8336373 [details] [diff] [review]
address-test-failures

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

I don't think there's anything specific to addon functionality, just addon id.
Also we wouldn't want to disable the tests on non-win8 windows.

Let's instead change the tests when XP_WIN is defined and when the os version >= 6.2

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +143,5 @@
>        "history-panelmenu",
>        "fullscreen-button",
>        "find-button",
> +      "preferences-button",
> +      "add-ons-button",

I think this patch should put back the addons button but leave the below off.

@@ +165,1 @@
>        panelPlacements.push("switch-to-metro-button");

Let's do this change in a followup bug, and flag gavin to see which button should be replaced. Please mention that bbondy thinks it may make sense to replace the full screen button with the metro one instead of the addon one.

Please block the shared profile bug with that new bug.

Comment 31

[Brian R. Bondy [:bbondy]]

Just putting the addons back may be enough to fix the test failure in this bug and you can handle the rest in the new bug. As long as we can grab the button from the customizations section that's good enough for this bug.

Comment 32

[Marina Samuel [:emtwo]]

Attachment: address-test-failures

This patch removes the 'Metro Mode' button from the panel as a default button, but it can still be added as a custom button. Bug 942915 will decide where to place this button.

Tests pass locally, Waiting on try here: https://tbpl.mozilla.org/?tree=Try&rev=52788555dbe1

Comment 33

[Brian R. Bondy [:bbondy]]

Comment on attachment 8337847 [details] [diff] [review]
address-test-failures

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

I'll land this on oak now

Comment 34

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/bd024cf6af34
https://hg.mozilla.org/integration/fx-team/rev/b22896e60da9

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd024cf6af34
https://hg.mozilla.org/mozilla-central/rev/b22896e60da9

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: Platform related changes for Holly branch only

Comment 37

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/holly/rev/4f7211316095

Comment 38

[Manuela Muntean [Away]]

Attachment: screenshot_Nightly.png

Verified as fixed, for iteration #19, with latest Nightly, 2013-12-01, on Win 8.1 Pro 64-bit.

The 'Metro Mode' button opens Metro when clicked and it can be added as a custom button. (as shown in the attached screenshot)