Closed Bug 934032 Opened 11 years ago Closed 11 years ago

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


(Firefox :: General, defect, P1)

Windows 8.1



Firefox 28


(Reporter: bbondy, Assigned: emtwo)



(Whiteboard: [block28][completed-oak] feature=story c=tbd u=tbd p=3)


(6 files, 8 obsolete files)

511.44 KB, image/jpeg
253.87 KB, patch
: review+
Details | Diff | Splinter Review
136.28 KB, image/png
1.47 KB, patch
: review+
Details | Diff | Splinter Review
4.42 KB, patch
: review+
Details | Diff | Splinter Review
106.82 KB, image/png
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.


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.
Here's code for checking if we are already default:

> shell = Components.classes[";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.
To show the default browser prompt if you're not the default browser do this:
shell.setDefaultBrowser(claimAllTypes, false);
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.
All of these changes for this bug should be behind:
#ifdef XP_WIN
#ifdef MOZ_METRO
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:
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.
Whiteboard: p=2
Whiteboard: p=2 → [block28][Blocking only if UX branch lands in 28][p=2]
Depends on: 934029
p += 1 to account for updated UX design.
Whiteboard: [block28][Blocking only if UX branch lands in 28][p=2] → [block28][Blocking only if UX branch lands in 28][p=3]
Assignee: nobody → msamuel
Depends on: metrov1it19
Blocks: metrov1it19
No longer blocks: metrov1backlog
No longer depends on: metrov1it19
Priority: -- → P1
Whiteboard: [block28][Blocking only if UX branch lands in 28][p=3] → [block28][Blocking only if UX branch lands in 28] feature=story c=tbd u=tbd p=3
* 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
Attachment #830487 - Flags: feedback?(netzen)
Attached image australis_ux.png (obsolete) —
QA Contact: jbecerra
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:

The patch looks good overall.
1 thing that I noticed that was missing is a check like this:

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.
Attachment #830487 - Flags: feedback?(netzen)
Also please ask UX for the asset if you're missing it sliced up or whatever, thanks! :D
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).
Flags: needinfo?(ywang)
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.
Flags: needinfo?(ywang)
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.
Ignore the placement of customize and help.
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
Attached image australis_ux2.png (obsolete) —
Attachment #831485 - Flags: review?(netzen)
Attached file (obsolete) —
Attached, Firefox Touch Menu and Toolbar icon set.
Attached file (obsolete) —
Attachment #832935 - Attachment is obsolete: true
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
Attachment #831485 - Flags: review?(netzen)
Only add switch-to-metro-button if in Windows 8.
Attachment #831485 - Attachment is obsolete: true
Attachment #833124 - Flags: review?(netzen)
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
Attachment #833124 - Flags: review?(netzen) → review+
Includes new metro icons \o/
Attachment #830487 - Attachment is obsolete: true
Attachment #830489 - Attachment is obsolete: true
Attachment #831486 - Attachment is obsolete: true
Attachment #832946 - Attachment is obsolete: true
Attachment #833124 - Attachment is obsolete: true
Attachment #8334077 - Flags: review+
Attached image australis_ux.png
Landed on oak here:

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.
Closed: 11 years ago
Resolution: --- → FIXED
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]
Flags: needinfo?(msamuel)
I updated bug 924914 with a pre-australis patch and I believe the current patch here should be good for australis.
Flags: needinfo?(msamuel)
Blocks: 935099
Whiteboard: [block28][Blocking only if UX branch lands in 28] feature=story c=tbd u=tbd p=3 → [block28][completed-oak] feature=story c=tbd u=tbd p=3
marina looks like this is causing a test failure here:

Could you take a look?
Flags: needinfo?(msamuel)
Attached patch address-test-failures (obsolete) — Splinter Review
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: which hopefully should fix the test failures.
Flags: needinfo?(msamuel)
Comment on attachment 8336373 [details] [diff] [review]

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.
Attachment #8336373 - Flags: feedback-
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.
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:
Attachment #8336373 - Attachment is obsolete: true
Comment on attachment 8337847 [details] [diff] [review]

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

I'll land this on oak now
Attachment #8337847 - Flags: review+
Resolution: FIXED → ---
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 944790
No longer depends on: 944790
Depends on: 944781
Blocks: 944563
Blocks: 944939
Attached image 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)
You need to log in before you can comment on or make changes to this bug.