831514 - Work - You can't pin sites that have a '/' character in the tile ID

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Andrew Scott]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20130116 Firefox/20.0
Build ID: 20130116092409

Steps to reproduce:

I invoked the appbar on a site and clicked on the pushpin toggle button.


Actual results:

The Toggle button is highlighted and it looked like the site is pinned, but I didn't find the site on the start screen. this does not happen on mozilla sites, but it does happen on other sites.


Expected results:

I should have had to confirm my choice before the pushpin looks like the site is pinned, then I should have seen a tile on the start screen.

Comment 1

[Mihai Morar, (:MihaiMorar)]

What about using new profile or in safe mode?

http://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode?redirectlocale=en-US&redirectslug=Safe+Mode

Comment 2

[Andrew Scott]

H

Comment 3

[Andrew Scott]

How do I do it in "Metro" mode?

Comment 4

[Matt Brubeck (:mbrubeck)]

Using a Metro-enabled nightly build [1], you can right-click (or swipe from the bottom edge of the screen) to display a toolbar that has a "pin" button at the bottom right corner of the screen. Clicking this button should pin the current page to the start screen.

1. https://wiki.mozilla.org/Firefox/Windows_8_Integration#Elm_Nightly_Builds

This will only work if Firefox is your default browser and is running in Metro mode.  Does this work for you?  If not, can you describe exactly what happens when you try it?

Comment 5

[Andrew Scott]

It doesn't work for me.  When I try to pin a non mozilla site to start, like google, it  highlights the toggle button white as if I pinned it, but it doesn't prompt me to name the tile, neither does it show up at the end off Start like it should.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I see this too, with the current Firefox Nightly build. I was able to successfully pin one site to the Start Screen, with the naming confirmation panel shown first, but it hasn't worked since.

Comment 7

[Brian R. Bondy [:bbondy]]

KWierso, for the times it hasn't worked since, do you get a confirmation panel or not?

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I do not see the panel. The button in the app bar changes to the "pinned" image (and changes back to the "unpinned" one if I click it again), but no panel appears.

Comment 9

[Andrew Scott]

Same here, you should see that panel but it doesn't show. neither does any tiles appear on the start screen.

Comment 10

[Brian R. Bondy [:bbondy]]

Kwierso do you have a debug build handy that you can try to look at the console after pinning for any messages? If so do you at least get this?

Async operation status: %d

Your .mozconfig should contain:
ac_add_options --enable-metro

Comment 11

[Brian R. Bondy [:bbondy]]

KWierso btw I could not reproduce with the new device.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Toget to "new device" state, I just delete %APPDATA%\MetroFirefox and start it up, right? I'll try that tonight.

Comment 13

[Brian R. Bondy [:bbondy]]

I think so, I always nuke both the roaming and local one.  Maybe rename them though so you can reproduce again in case you can no longer reproduce after.  Thanks!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So, on my Dell XPS 17 (which I was using when I originally confirmed this bug), I deleted both %APPDATA%\Mozilla\MetroFirefox and %LOCALAPPDATA%\Mozilla\MetroFirefox

I then started up Metro Firefox and saw that the newly regenerated profile was created. I then tried to pin a page to Start. No panel shown; nothing pinned.

My somewhat-recently acquired Surface Pro does not reproduce this. I've pinned at least three different sites to Start on it.

Only difference I can think of is that I've had various Elm builds of Firefox installed on my Dell, while I've only had Nightly (no older than late January) installed on the Surface.

I'll try doing a debug build tonight or tomorrow to see if that sheds any light on this.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #14)
> I'll try doing a debug build tonight or tomorrow to see if that sheds any
> light on this.

Just read in #windev that the error console can now be opened with Ctrl-Shift-J in Metro Firefox. 
Using either yesterday or today's standard m-c Nightly, after attempting to pin a site to Start, the Metro error console shows the following:
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWinMetroUtils.pinTileAsync]
Source File: chrome://browser/content/browser.js   Line: 683

Comment 16

[Brian R. Bondy [:bbondy]]

So one of these checks are failing although I'm not sure what:
http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/nsWinMetroUtils.cpp#l63

Comment 17

[Chris Gadd]

Using 23.0a1 I get similar behaviour. New Metro profile, I seem to be able to pin anything except for this site:
http://home.nzcity.co.nz/tvnow/tvguide.aspx
Tried loads of other sites, including aspx and other sites which I have bookmarked already in my desktop profile, all fine. That's just weird. Same error as #15, but line 689 now. Happened to be the first site I tried to pin after launching Metro, that's the only special thing about it I can think of. But after deleting roaming and local profile it's still apparent.

Comment 18

[Brian R. Bondy [:bbondy]]

Thanks Chris Gadd, I can reproduce reliably with this URL. I'm going to use this bug to fix your issue.  I think Kwierso's issue is probably different and if it is then I'll post a new issue for that after this is fixed.

Comment 19

[Marco Mucci [:MarcoM]]

Point estimation =2.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 741528 [details] [diff] [review]
Patch v1.

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

::: browser/metro/base/content/browser.js
@@ +670,5 @@
>      file.append("VisualElements_logo.png");
>      var ios = Components.classes["@mozilla.org/network/io-service;1"]. 
>                getService(Components.interfaces.nsIIOService); 
>      var uriSpec = ios.newFileURI(file).spec;
> +    MetroUtils.pinTileAsync(this._currentPageTileID,

nit - indentation.

there's a little bit of white space too above this if you feel like cleaning it up.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

w/ review comments.

Comment 23

[Brian R. Bondy [:bbondy]]

a536a231fafb

Comment 24

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a536a231fafb

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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