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

VERIFIED FIXED in Firefox 28

Status

()

defect
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: emtwo)

Tracking

unspecified
Firefox 28
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(6 attachments, 8 obsolete attachments)

511.44 KB, image/jpeg
Details
253.87 KB, patch
emtwo
: review+
Details | Diff | Splinter Review
136.28 KB, image/png
Details
1.47 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
4.42 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
106.82 KB, image/png
Details
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.
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.
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 HAVE_SHELL_SERVICE
#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:
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.
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
Status: NEW → ASSIGNED
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)
Posted 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:
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.
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
Posted image australis_ux2.png (obsolete) —
Attachment #831485 - Flags: review?(netzen)
Posted file firefox-touch_icons.zip (obsolete) —
Attached, Firefox Touch Menu and Toolbar icon set.
Posted file firefox-touch_icons.zip (obsolete) —
Resized.
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+
Posted image australis_ux.png
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.
Status: ASSIGNED → RESOLVED
Closed: 6 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:
https://tbpl.mozilla.org/?tree=Oak&rev=8c16b0b4853d

Could you take a look?
Flags: needinfo?(msamuel)
Posted 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: https://tbpl.mozilla.org/?tree=Try&rev=bbd718593fe6 which hopefully should fix the test failures.
Flags: needinfo?(msamuel)
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.
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: https://tbpl.mozilla.org/?tree=Try&rev=52788555dbe1
Attachment #8336373 - Attachment is obsolete: true
Comment on attachment 8337847 [details] [diff] [review]
address-test-failures

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

I'll land this on oak now
Attachment #8337847 - Flags: review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
https://hg.mozilla.org/mozilla-central/rev/bd024cf6af34
https://hg.mozilla.org/mozilla-central/rev/b22896e60da9
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 944790
No longer depends on: 944790
Depends on: 944781
Blocks: 944563
Blocks: 944939
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.