Closed Bug 862998 (fx-UITour) Opened 11 years ago Closed 11 years ago

Add glue to allow Firefox first run page to highlight UI elements

Categories

(Firefox :: Tours, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox27 --- disabled
firefox28 --- disabled

People

(Reporter: cers, Assigned: Unfocused)

References

(Depends on 18 open bugs, )

Details

(Whiteboard: [Australis:P2])

Attachments

(3 files, 10 obsolete files)

1.16 KB, patch
Dolske
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
1.79 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
35.52 KB, patch
Dolske
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
Attached patch WIP (obsolete) — Splinter Review
The new first run experience requires interaction with the UI, such as highlighting an element or pointing to it with a descriptive arrowpanel.

More details can be found here: http://people.mozilla.com/~zfang/FirstRun/FirstRun_PhaseTwo_0220.pdf
Attached patch WIP (obsolete) — Splinter Review
Forgot to add file to hg
Attachment #738678 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
Once it makes it through try, a build should be available here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/csonne@mozilla.com-0bcf2a56ea43/

And here is a video of it: http://people.mozilla.com/~csonne/firstrunv3.mov

The url that it handles events for can be changed in about:config under firstrun.url
Attachment #738681 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
Updated version of the patch. Applies cleanly to ux tip, but I haven't tested in it.
Attachment #742237 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
Updated the patch to work with the new bookmark button
Attachment #761120 - Attachment is obsolete: true
*Yoink!*
Assignee: cers → bmcbride
Status: NEW → ASSIGNED
Hey Blair, we just met with Christian today and he is putting the web page behind the modal/overlay. Once that code is done it will be here: http://geeksbynature.dk/ux/firstrun/firstrun.html


Christian's code is up to date in this bug for what he's done in the chrome. So it looks like once we have a build and drop that URL in, we should have a prototype ready for testing!
Flags: needinfo?(bmcbride)
Ok, a Try build should (in theory) appear here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-0d621b71e0cc/

On startup, it'll automatically show the first run thingy. On second startup, it'll show it again - eventually that can be used to load alternate content for a secondary tour, and only show after X amount of time has passed since the initial first run tour. The tour is also accessible via the help menu (both the menubar, and the Australis panel UI).
Flags: needinfo?(bmcbride)
Awesome. Thanks, Blair! I'll check it out today.
(In reply to Blair McBride [:Unfocused] from comment #7)
> On startup, it'll automatically show the first run thingy.

I should note, it currently does this for old profiles too - not just new ones.
A few quick comments:

- When talking with Blair on IRC this morning I asked if it was possible to generate a new profile when starting this build. He said it has been done before for a UR build, so if there is time, we can add this.

- Mac build: Upon opening the build for the first time, the dialog for choosing this as your default browser appears behind the First Run overlay.  http://cl.ly/image/2a0c3p2q1e0z

- When loading Christian's URL (http://geeksbynature.dk/ux/firstrun/firstrun.html) the overlay is the proper width. The above screenshot is just on opening this test build without his URL loaded.


Christian, have you had a chance to put the webpage behind the overlay? Once this is done I will see if we can put this on a Mozilla server, then set this as the start page in the build (open about:config and set browser.firstrun.initial.url and browser.firstrun.secondary.url to the new URL). 

I'll test the interactions and respond in a new comment.
Loading Christian's URL doesn't trigger any interaction within the chrome. Should it? I only have web interaction.
"Save Your Faves" and "Make a Shortcut" work fine for me. These are the only ones I am able to test in the build on Mac. I'll try on Windows later today to see if more features display in the overlay. (what I can see on the mac build: http://cl.ly/image/3C2F38260R41)
I've created a version of the page that has the proper content in the background: http://geeksbynature.dk/ux/firstrun/firstrun_demo.html

Blair: it looks to me like you are now either loading the content in a chrome overlay, or possibly loading it from a local copy? As I understand, it's pretty important that
1) it's a page webdev have access to, so translation etc doesn't have to ride the firefox train
2) that it appears only on a specific webpage (that looks like the one in above url)

Is there a way to fulfill those requirements in your version? (Others: please correct me if I am wrong on any of those points)
(In reply to Holly Habstritt [:Habber] from comment #10)
> I asked if it was possible to
> generate a new profile when starting this build. He said it has been done
> before for a UR build, so if there is time, we can add this.

Bug 879846 has the relevant stuff to make this happen.
 
> - Mac build: Upon opening the build for the first time, the dialog for
> choosing this as your default browser appears behind the First Run overlay. 

That's a bug I'll have to fix.


(In reply to Holly Habstritt [:Habber] from comment #11)
> Loading Christian's URL doesn't trigger any interaction within the chrome.
> Should it? I only have web interaction.

Hm, that should work. Maybe I broke something.

(In reply to Holly Habstritt [:Habber] from comment #12)
> "Save Your Faves" and "Make a Shortcut" work fine for me. These are the only
> ones I am able to test in the build on Mac. I'll try on Windows later today
> to see if more features display in the overlay. (what I can see on the mac
> build: http://cl.ly/image/3C2F38260R41)

I constrained the width of the panel - the web page isn't fitting in the space. Eventually it'll have to adapt to the available width, but for now this build should (in theory) show everything:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-6c7d10aaa735
(Note: Builds won't show up for an hour or two after I post this comment.)
(Note 2: See below.)

(In reply to Christian Sonne [:cers] from comment #13)
> I've created a version of the page that has the proper content in the
> background: http://geeksbynature.dk/ux/firstrun/firstrun_demo.html

Sadly, you domain seems to be down :( Can't test anything, and I don't have a local copy.

> Blair: it looks to me like you are now either loading the content in a
> chrome overlay

Yes, loading the remote URL it in a <xul:browser> inside a <xul:panel> that has it's width constrained (originally it was half of the browser window - seems that was too small). See above for a build with the panel set as the width of the browser window. The page will either have to adapt to a smaller width (my preference), or we'll have to be ok with showing scrollbars for people with low resolution screens.

> 1) it's a page webdev have access to, so translation etc doesn't have to
> ride the firefox train

Yep.

> 2) that it appears only on a specific webpage (that looks like the one in
> above url)

Yep. Both are catered for, although I've relaxed "specific webpage" to matching just domain, since URLs on mozilla.org for this type of thing usually end up being something like https://www.mozilla.org/%LANG%/%OS%/firstrun-initial.html?src=firefox
(In reply to Blair McBride [:Unfocused] from comment #14)
Hey Blair, Thank you for the update and the try build! The page now shows up correctly for me (on Mac) and all the features works :) Here's some feedback: 

1. The firstrun panel is within another panel, it shows two close button http://cl.ly/image/1N3a3n3u1S2O
2. When open a new tab, the panel doesn't go away.
3. For highlighting the awesome bar, we probably want to highlight the entire URL bar in stead of a circle. and it doesn't have to move around (since it's too big) http://cl.ly/image/430I2a0h1V0R
4. I don't see the button for "Menu", it should work almost exactly like the button for Bookmarks. Are you blocked on screenshots of the menu? I remember I put it somewhere but I couldn't find it now. I'll send them your way later today, along with the copy.

This is great! I think after fixing some minor issues and put the first run webpage behind the panel, we can do some usability testing. Holly and I will start recruiting and keep you posted :)
Thanks for new build! Glad to see that we super close. I have a few things to add to Zhenshuo's comments.

1. What needs to happen get the web page behind the overlay like Christian has done, here with :
http://geeksbynature.dk/ux/firstrun/firstrun_demo.html

We could just load a web page that we need and then use the help menu to trigger the overlay over it, but then it wouldn't be faded out as you see in Christian's example. If the overlay needs to be rendered in the chrome, then can we still have a transparent layer behind it that would appear over any web page?


2. When going to the add-ons page, the first run overlay should go away. 


3. The full message for the awesome bar does not fit in the pop-over/tool-tip. It just says "The awesome bar gets smarter as you use it. Start by typing"  Could you get the entire message to fit? Does the awesome bar interaction work to display the video after typing a message in the awesome bar?

here is the copy and details for the awesome bar:

	• Pop-Over:  
	• The awesome bar gets smarter as you use it. 
	• Start by typing "I Love Firefox" and wait a few seconds for a surprise. 
	• Video reveal easter egg for prototype: Firefox Flicks 2012 show reel.+ link to "Learn more about the Firefox Flicks film contest" firefoxflicks.mozilla.org
	• http://www.youtube.com/watch?v=LnaidgLYBI8&feature=share&list=UUajipi80aORRDz6gZ8ZyCWw


I have some users available that I'd like to test at the end of this week if possible. Blair, could you let us know what your next steps are?
Here's the copy and screenshot for the Menu Panel:

Display Name: Quick access your tools

Hover Message: Access and customize all your tools from this menu !

Pop-Over: None. Open Menu.

Screenshots:
Windows: https://www.dropbox.com/s/4919ls8y8w3bnur/windows-Menu.png
Mac https://www.dropbox.com/s/x7wi5aaxo1vkj4p/mac-menu.png
New build (will take an hour or two to show up):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-f482262e021e


(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #15)
> 3. For highlighting the awesome bar, we probably want to highlight the
> entire URL bar in stead of a circle. and it doesn't have to move around
> (since it's too big) http://cl.ly/image/430I2a0h1V0R

Fixed. Although, the highlight does have an effect for the URL bar, to see how it feels. 

Had to change the way the animation was implemented anyway, so ended up playing around with other different effects. So as a bit of whimsy, the next build uses a random effect (wobble, zoom, or colour cycle) for the highlight ring.


(In reply to Holly Habstritt [:Habber] from comment #16)
> 1. What needs to happen get the web page behind the overlay like Christian
> has done, here with :
> http://geeksbynature.dk/ux/firstrun/firstrun_demo.html

Oh! I thought that page was to be used separately (ie, we'd show either the overlay, OR that page). Ok, that's easy to fix - will just load that page in a tab instead of into browser chrome. That'll be in the next build.


> 2. When going to the add-ons page, the first run overlay should go away. 

Fixed with the version of overlay loaded in browser chrome, which is kinda irrelevant now.

When we load http://geeksbynature.dk/ux/firstrun/firstrun_demo.html in a tab, should that page be replaced by the addons page, or should the addons page just open in a new tab (so you can switch back to the first-run tab)?


> 3. The full message for the awesome bar does not fit in the
> pop-over/tool-tip. It just says "The awesome bar gets smarter as you use it.
> Start by typing"  Could you get the entire message to fit? 

I thought that test looked odd - it's an error in the webpage (Christian: escape your quotes!).


> Does the awesome
> bar interaction work to display the video after typing a message in the
> awesome bar?

Not yet. I've been purposefully leaving that til last, as it's gonna be tricky.


> 	• Video reveal easter egg for prototype: Firefox Flicks 2012 show reel.+
> link to "Learn more about the Firefox Flicks film contest"
> firefoxflicks.mozilla.org
> 	•
> http://www.youtube.com/
> watch?v=LnaidgLYBI8&feature=share&list=UUajipi80aORRDz6gZ8ZyCWw

Christian: Can you do a page for this? And I'll open that as a tab.


> I have some users available that I'd like to test at the end of this week if
> possible. Blair, could you let us know what your next steps are?

New build tomorrow that should have everything mentioned fixed. Not sure if it'll have the easter egg though.
(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #17)
> Here's the copy and screenshot for the Menu Panel:

Christian: If you can get this into the page, I'll use uitarget="APPMENU" and type="appmenu"
(In reply to Blair McBride [:Unfocused] from comment #18)
> New build (will take an hour or two to show up):
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.
> com-f482262e021e
> 
> 
> (In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #15)
> > 3. For highlighting the awesome bar, we probably want to highlight the
> > entire URL bar in stead of a circle. and it doesn't have to move around
> > (since it's too big) http://cl.ly/image/430I2a0h1V0R
> 
> Fixed. Although, the highlight does have an effect for the URL bar, to see
> how it feels. 
> 
> Had to change the way the animation was implemented anyway, so ended up
> playing around with other different effects. So as a bit of whimsy, the next
> build uses a random effect (wobble, zoom, or colour cycle) for the highlight
> ring.

Yay whimsy!
 
> (In reply to Holly Habstritt [:Habber] from comment #16)
> > 1. What needs to happen get the web page behind the overlay like Christian
> > has done, here with :
> > http://geeksbynature.dk/ux/firstrun/firstrun_demo.html
> 
> Oh! I thought that page was to be used separately (ie, we'd show either the
> overlay, OR that page). Ok, that's easy to fix - will just load that page in
> a tab instead of into browser chrome. That'll be in the next build.

Yea the page shows up behind the overlay so that when the user closes the tour (some will close it immediately) they can still see some info about the product, brand, have share options, etc. 


> > 2. When going to the add-ons page, the first run overlay should go away. 
> 
> Fixed with the version of overlay loaded in browser chrome, which is kinda
> irrelevant now.
> 
> When we load http://geeksbynature.dk/ux/firstrun/firstrun_demo.html in a
> tab, should that page be replaced by the addons page, or should the addons
> page just open in a new tab (so you can switch back to the first-run tab)?
> 

Opening the add-ons page in a new tab would be great. Good idea.

> 
> > 3. The full message for the awesome bar does not fit in the
> > pop-over/tool-tip. It just says "The awesome bar gets smarter as you use it.
> > Start by typing"  Could you get the entire message to fit? 
> 
> I thought that test looked odd - it's an error in the webpage (Christian:
> escape your quotes!).
> 
> 
> > Does the awesome
> > bar interaction work to display the video after typing a message in the
> > awesome bar?
> 
> Not yet. I've been purposefully leaving that til last, as it's gonna be
> tricky.

Ok, no problem. I can do some tests this week without that. 


> > 	• Video reveal easter egg for prototype: Firefox Flicks 2012 show reel.+
> > link to "Learn more about the Firefox Flicks film contest"
> > firefoxflicks.mozilla.org
> > 	•
> > http://www.youtube.com/
> > watch?v=LnaidgLYBI8&feature=share&list=UUajipi80aORRDz6gZ8ZyCWw
> 
> Christian: Can you do a page for this? And I'll open that as a tab.
> 
> 
> > I have some users available that I'd like to test at the end of this week if
> > possible. Blair, could you let us know what your next steps are?
> 
> New build tomorrow that should have everything mentioned fixed. Not sure if
> it'll have the easter egg though.


Thanks again!
Tada!
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-c0d9e450f7fb

Everything mentioned so far fixed, except anything that needs fixed in the web page itself. And no easter egg, but I have figured out a vastly simpler way to do it (which seems obvious now I've thought of it). And I still have various other to-do's, but this at least is a milestone that should be appropriate for testing.
Attached patch WiP v6 (obsolete) — Splinter Review
Bah, could have sworn I remembered uploading an earlier version of this...
Attachment #761140 - Attachment is obsolete: true
Attachment #780920 - Flags: feedback?(dolske)
This is really awesome, Blair. It's definitely ready to do some testing with. 

I have an addition to the items above that is a last priority. I just thought I'd mention it since it was part of our original plan for this prototype. The idea is that interaction in the chrome is also triggered by a few elements in the web page (behind the overlay) as well since many users will close the overlay. By hovering over the "Innovative features" "customization" and "shortcuts" areas in the web page, the elements in the chrome would have the highlight around them to show where they are or in the case of themes, change the browser style as we did in the overlay.  http://cl.ly/image/163e2v1M2r0B

Even if we don't get to this I'd like to understand how the web prod team would work with you to add this functionality to a web page in the future. If we have a bucket of interactions to choose from that are set up in the chrome (and wouldn't be asking you to add new ones) what would a web prod developer need to know to turn the interactions on from a web page? There are use cases other than First Run and Updating where this would be very helpful. For instance, on a SUMO help page, showing a user where a feature is in the chrome directly instead of just showing a screenshot. 

Thanks!
(In reply to Holly Habstritt [:Habber] from comment #23)
> This is really awesome, Blair. It's definitely ready to do some testing
> with. 

\o/


> I have an addition to the items above that is a last priority. I just
> thought I'd mention it since it was part of our original plan for this
> prototype. The idea is that interaction in the chrome is also triggered by a
> few elements in the web page (behind the overlay) as well since many users
> will close the overlay. By hovering over the "Innovative features"
> "customization" and "shortcuts" areas in the web page, the elements in the
> chrome would have the highlight around them to show where they are or in the
> case of themes, change the browser style as we did in the overlay. 
> http://cl.ly/image/163e2v1M2r0B

The build in comment 21 already supports this - the web page just needs to advertise what it wants done when hovering over those parts of the page (using the same mechanism as the overlay uses - see below).


> Even if we don't get to this I'd like to understand how the web prod team
> would work with you to add this functionality to a web page in the future.
> If we have a bucket of interactions to choose from that are set up in the
> chrome (and wouldn't be asking you to add new ones) what would a web prod
> developer need to know to turn the interactions on from a web page? 

The page just needs to send a "BrowserFirstrun" event (note: will probably get renamed in the future) with the relevant details. The page's existing code has examples of how to use it (and helper functions to make it even easier). It's done in such a way that the page can be updated without needing any changes to Firefox. The only limitation is adding new areas of the browser UI to highlight (that's due to security reasons) - eg, downloads button, etc.

And adding new areas for the highlight/tooltip is really simple for me to do. I'd like to get a list of things we may want to highlight and include them in this patch - even if we don't use them right away, they'll be available to use in the future (and on SUMO).


> There
> are use cases other than First Run and Updating where this would be very
> helpful. For instance, on a SUMO help page, showing a user where a feature
> is in the chrome directly instead of just showing a screenshot. 

I'd only need to change the security check to allow SUMO to do this. Then SUMO would just trigger the interactions the same way the firstrun page does.
Whiteboard: [Australis:P2]
Comment on attachment 780920 [details] [diff] [review]
WiP v6

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

Generally looking good, although I a bit confused by the initial setup and events.

::: browser/app/profile/firefox.js
@@ +1279,5 @@
> +// First run tour experience
> +pref("browser.firstrun.themeOrigin", "https://addons.mozilla.org/en-US/firefox/themes/");
> +pref("browser.firstrun.appTabUrl", "https://www.mozilla.org/");
> +
> +pref("browser.firstrun.primary.url", "http://geeksbynature.dk/ux/firstrun/firstrun_demo.html");

Obviously these will need to be mozilla.org SSL links. :)

::: browser/base/content/browser-firstrun.js
@@ +56,5 @@
> +      let url = Services.urlFormatter.formatURLPref("browser.firstrun.secondary.url");
> +      return Services.io.newURI(url, null, null);
> +    });
> +
> +    gBrowser.addEventListener("BrowserFirstrun", this, true, true);

How about making this a content frame script (content.js), so that this .jsm doesn't even get instantiated until the first time the even happens? (ala LoginManagerContent).

mozBrowserFirstrun, just to be explicit. :)

@@ +58,5 @@
> +    });
> +
> +    gBrowser.addEventListener("BrowserFirstrun", this, true, true);
> +
> +    this.maybeShow();

Presumably this was just for testing?

@@ +75,5 @@
> +    //       the way the tour is marked as shown is done asynchronously.
> +    //       May need to refactor into a JSM? That would also make it easy to
> +    //       lazy-load much of this code too.
> +    if (!this.maybeShowType("primary"))
> +      this.maybeShowType("secondary");

What's the difference between "primary" and "secondary"?

@@ +116,5 @@
> +
> +  handleEvent: function(aEvent) {
> +    switch (aEvent.type) {
> +      case "pageshow":
> +        this.handleTourStarted();

I'm not sure why pageshow is involved here. Can't this all just sit and wait listening for |BrowserFirstrun|? (At which point that page is obviously shown :).

@@ +161,5 @@
> +    if (aSafeURI.host != aDocumentURI.host)
> +      return false;
> +
> +    if (aSafeURI.schemeIs("https") && !aDocumentURI.schemeIs("https"))
> +      return false;

I think this can be stricter -- just an exact string match. Or maybe prefix match, but that makes be nervous about empty-string. Do we know if this with be a simple static URL, or do we need to format it for l10n/version? We should probably open a dep bug to get that page going (perhaps as a simple placeholder right away).

Check to ensure it's a top-level doc? (ie not an iframe).

Principal checks?

@@ +184,5 @@
> +    switch (aEvent.detail.type) {
> +      case "description": {
> +        let target = null;
> +        let targetName = aEvent.detail.uitarget;
> +        if (targetName == "APPTAB")

WHY CAPS?

@@ +264,5 @@
> +
> +    return this.appTab;
> +  },
> +
> +  removeAppTab: function() {

Just kind of skimming for this feedback, but we should be very confident that we don't leave this tab permanently pinned. And maybe even make it a custom web page that explains what a pinned tab is / why it's great, and how to remove it.

@@ +315,5 @@
> +  },
> +
> +  openAppMenu: function() {
> +    //XXXunf This is ugly - should just change PanelUI.toggle() to accept the
> +    //       anchor directly.

Sounds reasonable.
Attachment #780920 - Flags: feedback?(dolske) → feedback+
(In reply to Justin Dolske [:Dolske] from comment #25)
> mozBrowserFirstrun, just to be explicit. :)

Went with mozUITour, as the plan is to use this for more than just first-run.


> I think this can be stricter -- just an exact string match. Or maybe prefix
> match, but that makes be nervous about empty-string. Do we know if this with
> be a simple static URL, or do we need to format it for l10n/version?

It's never a simple static URL :) And after discovering we can let existing code open the page on a new profile and after upgrade, and seeing all the different pages where this interaction is applicable on mozilla.org (first run, whats new, feature pages, SUMO articles, etc etc), a strict match just wouldn't work. So, moved to using the permissions manager and whitelisting specific domains (in addition to requiring secure connections, and a top-level document). That depends on the patch in bug 910172.


> Principal checks?

At least with the permissions manager, using a principal check just gets the URL for the principal and compares that - so we don't actually gain anything.


> Just kind of skimming for this feedback, but we should be very confident
> that we don't leave this tab permanently pinned.

Currently if you exit Firefox with that pinned tab open, it'll be saved in session restore and restored next time. Seems closing it when the window closes doesn't update the session store DB :\ Yet to figure out a way around that.

> And maybe even make it a
> custom web page that explains what a pinned tab is / why it's great, and how
> to remove it.

Now opens a SUMO article about pinned tabs.
Attached patch WiP v7 (obsolete) — Splinter Review
Still a few todos:
* s/Firstrun/UITour/ for element IDs/classes
* Trouble with the pinned tab being permanent 
* Additional input checks, making sure malformed events can't break us
* An odd bug where if you open the arrow panel for the urlbar before opening it for any other element, the arrow panel is broken. Works if the arrow panel is opened for any other element first. Utterly lost as to why that's happening.


As mentioned in the other bug, I have a modified mozilla.org firstrun page running at:
http://home.theunfocused.net:8000/en-US/firefox/22.0/firstrun/b/
Attachment #780920 - Attachment is obsolete: true
Attachment #796587 - Flags: feedback?(dolske)
Comment on attachment 796587 [details] [diff] [review]
WiP v7

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

::: browser/modules/UITour.jsm
@@ +53,5 @@
> +
> +    let window = this.getChromeWindow(contentDocument);
> +
> +    let tab = window.gBrowser._getTabForContentWindow(contentDocument.defaultView);
> +    this.originTabs.add(tab);

originTabs seems to be unused, except for adding things to it? Remnant from an older patch?

@@ +54,5 @@
> +    let window = this.getChromeWindow(contentDocument);
> +
> +    let tab = window.gBrowser._getTabForContentWindow(contentDocument.defaultView);
> +    this.originTabs.add(tab);
> +    tab.addEventListener("TabClose", () => {

Shouldn't this be pagehide (unload) instead of TabClose? If you browse away from the UITour page within the name tab, I'd should think that is the time to clean up.

Hmm, isn't every onPageEvent() call going to add a new listener?

@@ +91,5 @@
> +        this.openMenu(window, data.name);
> +        break;
> +      }
> +    }
> +	},

nit: Weird indent.

@@ +113,5 @@
> +  },
> +
> +  importPermissions: function() {
> +    try {
> +    PermissionsUtils.importFromPrefs(PREF_PERM_BRANCH, UITOUR_PERMISSION);

This API / .jsm doesn't seem to exist in mozilla-central. Where's it coming from?

This is kind of a weird API, why does it take a whole pref branch instead of a specific permission? What's the 260 in ""browser.uitour.whitelist.add.260" for?

@@ +116,5 @@
> +    try {
> +    PermissionsUtils.importFromPrefs(PREF_PERM_BRANCH, UITOUR_PERMISSION);
> +  } catch (e) {
> +    Cu.reportError(e);
> +  }

Nit: Weird indent on the catch.

@@ +127,5 @@
> +    let uri = aDocument.documentURIObject;
> +
> +    //XXXunf Re-enable this check when page is on a Mozilla server.
> +    //if (!uri.schemeIs("https") && !uri.schemeIs("chrome"))
> +    //  return false;

This and the browser.uitour.whitelist.add pref list need to be fixed for checkin.

@@ +191,5 @@
> +      aWindow.gBrowser.removeTab(tabInfo.tab);
> +  },
> +
> +  showHighlight: function(aTarget, aShow) {
> +  	let highlighter = aTarget.ownerDocument.getElementById("FirstrunHighlight");

Nit: more weird indent!

@@ +193,5 @@
> +
> +  showHighlight: function(aTarget, aShow) {
> +  	let highlighter = aTarget.ownerDocument.getElementById("FirstrunHighlight");
> +
> +    if (aShow) {

Should we just have separate "highlight" and "unhighlight" actions?

@@ +224,5 @@
> +  showTooltip: function(aAnchor, aTitle, aDescription) {
> +    let document = aAnchor.ownerDocument;
> +   	let tooltip = document.getElementById("FirstrunTooltip");
> +   	let tooltipTitle = document.getElementById("FirstrunTooltipTitle");
> +   	let tooltipDesc = document.getElementById("FirstrunTooltipDescription");

Nit: indents! tabs! *fistshake*

@@ +250,5 @@
> +    if (aMenuName == "appmenu") {
> +      aWindow.PanelUI.toggle();
> +    } else if (aMenuName == "bookmarks") {
> +      aWindow.document
> +      			 .getElementById("bookmarks-menu-button")

Nit: indents
Attachment #796587 - Flags: feedback?(dolske) → feedback+
(In reply to Justin Dolske [:Dolske] from comment #28)
> originTabs seems to be unused, except for adding things to it? Remnant from
> an older patch?

Bah, yep.

> Shouldn't this be pagehide (unload) instead of TabClose? If you browse away
> from the UITour page within the name tab, I'd should think that is the time
> to clean up.

Yep - actually originally had it like that. Must have changed it for testing and forgot to revert.

> Hmm, isn't every onPageEvent() call going to add a new listener?

Fixed.


> This API / .jsm doesn't seem to exist in mozilla-central. Where's it coming
> from?

See bug 910172 (mentioned in comment 26) - it's re-using the code that the Add-ons Manager uses for xpinstall permissions, rather than re-inventing the wheel.

> This is kind of a weird API, why does it take a whole pref branch instead of
> a specific permission? 

This is a requirement of how the Add-ons Manager works, as it supports adding to either a whitelist or blacklist, based on the pref name. Bug 861115 would also allow for removing from the whitelist/blacklist. So we take a branch, and all the child branches follow an expected pattern for doing all that.

We don't actually need all that functionality for this (well, yet). But as I said above, it saves re-inventing the wheel.

> What's the 260 in ""browser.uitour.whitelist.add.260"
> for?

"260" as in v26.0 - we version additions based on the app version it landed in, so we can mostly safely make additions in different branches (nightly/aurora/beta/release) without having to deal with conflicts.

> Should we just have separate "highlight" and "unhighlight" actions?

Done - all "show" actions now have a corresponding "hide" action. Except menus, which close by themselves anyway. This results in more actions, but it seems cleaner than having the action's parameters differ based on a "state" parameter.

> Nit: indents! tabs! *fistshake*

Grr, don't know where all these tabs came from suddenly.
(In reply to Blair McBride [:Unfocused] from comment #29)
> > Shouldn't this be pagehide (unload) instead of TabClose? If you browse away
> > from the UITour page within the name tab, I'd should think that is the time
> > to clean up.
> 
> Yep - actually originally had it like that. Must have changed it for testing
> and forgot to revert.

Er, correction: Meant to keep both, as pagehide doesn't handle the case where the tab is closed.
Depends on: 916732
Depends on: 910172
Attached patch Patch v1 (obsolete) — Splinter Review
Little later than I wanted, because software is hard.

Some caveats:
* Due to differences between UX and mozilla-central, this applies cleanly only to UX. I'll fix it up tomorrow, but would like my need for sleep to not hold up review.
* Still has a couple of things (see inline comments) to remove before landing. Those are needed for testing - I'll split those into a separate patch tomorrow.
* Dependent on bug 910172 (PermissionsUtils.jsm) and bug 916732 (to make it compatible with Australis) - neither of which currently have r+

Other comments:
* Remembered why I wanted originTabs!
Attachment #796587 - Attachment is obsolete: true
Attachment #805272 - Flags: review?(dolske)
Attached patch UX patch v1 (obsolete) — Splinter Review
And this is needed only on UX branch.
Attachment #805273 - Flags: review?(dolske)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Ok, this applies cleanly to both mozilla-central and UX.
Attachment #805272 - Attachment is obsolete: true
Attachment #805273 - Attachment is obsolete: true
Attachment #805272 - Flags: review?(dolske)
Attachment #805273 - Flags: review?(dolske)
Attachment #805918 - Flags: review?(dolske)
This is needed only on mozilla-central - UX already has this change.
Attachment #805919 - Flags: review?(dolske)
Attached patch UX patch v1.1Splinter Review
And this is needed only on UX, for compatibility with the changes there.
Attachment #805920 - Flags: review?(dolske)
Hi Blair, you can hold off on any further work on this prototype. We have completed our testing with the prototype and our next step is to design an experience based on what we learned. Without the prototype we wouldn't have had such thorough feedback from our users, so thanks again! Soon we will have compiled feedback to share with you if you are curious how it went.
(In reply to hhabstritt from comment #36)
> Hi Blair, you can hold off on any further work on this prototype.

Yea, I'm not actively working on the first-run page - that's waiting on someone from webdev for any further work (code is at https://github.com/Unfocused/bedrock/tree/firstrun-tour).

The patches here are just building blocks allowing the webpage to do define what it wants Firefox to show/do. So we can land these patches, and webdev can continue building the first run page independently. The only changes that would need changing what's in Firefox are things like what the highlight looks like, what the arrow panel looks like, allowing new things to be highlighted, etc. And that's easy to fix in followup patches.

And if it turns out it's not ready for use in a release version of Firefox, it's as simple as flipping a hidden preference to completely disable the feature. But the benefit from landing sooner rather than later is additional QA time, and it makes it much easier for webdev to build things with the functionality.

> Soon we will
> have compiled feedback to share with you if you are curious how it went.

Of course I'm curious :)
Ok, great! Just wanted to make sure you weren't doing extra work. That makes sense and will definitely help save us some time once we start to implement the new design for this. Thanks :)
Attachment #805919 - Flags: review?(dolske) → review+
Attachment #805920 - Flags: review?(dolske) → review+
Comment on attachment 805918 [details] [diff] [review]
Patch v1.1

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

We should ask QA to do some targeted testing of this, in particular around interactions that could result in this not calling teardownUITour() (or the firstrun page itself not canceling some interaction, but that's perhaps best left to when the page is more final?).

::: browser/app/profile/firefox.js
@@ +223,5 @@
> +pref("browser.uitour.themeOrigin", "https://addons.mozilla.org/%LOCALE%/firefox/themes/");
> +pref("browser.uitour.pinnedTabUrl", "https://support.mozilla.org/%LOCALE%/kb/pinned-tabs-keep-favorite-websites-open");
> +pref("browser.uitour.whitelist.add.260", "www.mozilla.org,support.mozilla.org");
> +//XXXunf This is for testing only - it needs to be removed before landing.
> +pref("browser.uitour.whitelist.add.testing", "home.theunfocused.net:8000");

Yes, please remove. Or maybe just comment out so it's easy for people to see how to add a pref for local testing?

::: browser/base/content/content.js
@@ +47,5 @@
>      LoginManagerContent.onUsernameInput(event);
>    });
> +
> +  if (Services.prefs.getBoolPref("browser.uitour.enabled")) {
> +    addEventListener("mozUITour", function(event) {

Not that it really matters, but I'd flip these around so the pref is live. I'd assume a test would want that. (Speaking of... Ahem, tests? Should have at least a basic test for at least one trigger. Doesn't need to be exhaustive.)

::: browser/modules/UITour.jsm
@@ +253,5 @@
> +    let uri = aDocument.documentURIObject;
> +
> +    //XXXunf Re-enable this check when page is on a Mozilla server.
> +    //if (!uri.schemeIs("https") && !uri.schemeIs("chrome"))
> +    //  return false;

Please.

@@ +386,5 @@
> +    else if (aMenuName == "bookmarks")
> +      openMenuButton("bookmarks-menu-button");
> +  },
> +
> +  startUrlbarCapture: function(aWindow, aExpectedText, aUrl) {

What's this intended to do?
Attachment #805918 - Flags: review?(dolske) → review+
Flags: sec-review?
Flags: sec-review? → sec-review?(dveditz)
(In reply to Justin Dolske [:Dolske] from comment #39)
> > +  startUrlbarCapture: function(aWindow, aExpectedText, aUrl) {
> 
> What's this intended to do?

That's the easter egg UX requested - ie, type in "i love firefox" in the urlbar (only when activated by the firstrun page), and it opens an easter egg URL.
Attached patch Patch v2Splinter Review
Now with tests!
Attachment #805918 - Attachment is obsolete: true
Attachment #815282 - Flags: review?(dolske)
Comment on attachment 815282 [details] [diff] [review]
Patch v2

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

This isn't working with http://home.theunfocused.net:8000/en-US/firefox/22.0/firstrun/b/, I assume it's just out of date? (I added the testing pref.)
Attachment #815282 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #42)
> This isn't working with
> http://home.theunfocused.net:8000/en-US/firefox/22.0/firstrun/b/, I assume
> it's just out of date? (I added the testing pref.)

Yea, I un-commented out the explicit https check - so it's expected to not work over http. Will get my copy of bedrock running on HTTP later today, until we have it up on a proper staging site.
https://hg.mozilla.org/integration/fx-team/rev/6d4f3127e050
https://hg.mozilla.org/integration/fx-team/rev/491b452af425

Will leave this open until the Australis part of the patch lands, which has to wait til these end up on the UX branch.
Keywords: feature
Whiteboard: [Australis:P2] → [Australis:P2][leave open]
Attachment #805919 - Flags: checkin+
Attachment #815282 - Flags: checkin+
Huh, that really shouldn't have passed locally (it did, I just re-ran it over 100 times to be sure).

Relanded with test fix, indentation fixes, and an inconsequential typo fix:

https://hg.mozilla.org/integration/fx-team/rev/300cc4ded5d6
And I have the test page available over HTTPS now:
https://bedrock-unfocused.ngrok.com/en-US/firefox/22.0/firstrun/b/

To get it working, you'll need to add the following pref in about:config and restart:

  browser.uitour.whitelist.add.testing = bedrock-unfocused.ngrok.com
https://hg.mozilla.org/projects/ux/rev/213ebacc64e2
https://hg.mozilla.org/projects/ux/rev/300cc4ded5d6

(resolving per comment #44 )
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][leave open] → [Australis:P2][fixed-in-ux]
Target Milestone: --- → Firefox 27
Depends on: 933023
Depends on: 934116
We'll use the Australis meta bug to track this feature.
Keywords: feature
Depends on: 935815
Depends on: 935823
Depends on: 935826
Depends on: 935829
Depends on: 936187
Depends on: 936195
Depends on: 936265
Depends on: 936266
Depends on: 936273
Depends on: 936378
Blocks: 934116
No longer depends on: 934116
Depends on: 937415
Depends on: 938079
Depends on: 939008
Which version of Firefox is this targetted? This bug says 27, but the latest commit for this feature says 28: Bug 939008 - Move UITour CSS into themes/shared/ and browser/base/content/browser.css
As far as access goes:
* Please make sure that this cannot be abused in a security respect.
* Please make this available to other domains. This would be useful for extensions, too.
  At the minimum, allow chrome://, so that an extension can ship the tutorial as part of the XPI.
* Confine permissions stricter than just the whole mozilla.org domain.
  For example, testcases attached to bugzilla shouldn't be allowed.

Alternatively, you can always ship the tutorial document (without images) as part of Firefox chrome, with chrome:// URL.
> * Please make sure that this cannot be abused in a security respect.

FWIW, from that point of view, mozilla.org website is untrusted.
https://hg.mozilla.org/mozilla-central/rev/213ebacc64e2
Whiteboard: [Australis:P2][fixed-in-ux] → [Australis:P2]
Depends on: 940902
(In reply to Alfred Kayser from comment #51)
> Which version of Firefox is this targetted?

This patch landed in Firefox 27 but we plan to use this first to introduce Australis changes (whatever release that ends up being).

(In reply to Ben Bucksch (:BenB) from comment #52)
>   At the minimum, allow chrome://, so that an extension can ship the
> tutorial as part of the XPI.

Extensions can simply use UITour.jsm from their chrome:// page without touching the whitelist.
Depends on: 941385
Depends on: 941387
Depends on: 941862
Depends on: 941864
> Extensions can simply use UITour.jsm from their chrome:// page without touching the whitelist.

That's great. Thank you.

I would propose to do the same for the Firefox tour, to sidestep any security problems. Images can be on the server, but page and code should be chrome:// . It's probably just a few KB. Balance with the security reviews, discussion etc..
Enabling add-on use of this is a non-goal. If it happens to be useful, fine, but the scope of the current work is limited to the whatsnew/firstrun page for Firefox. "Patches welcome."

I'm also ambivalent about dev-doccing this. It's not really intended as a general-use API.
Depends on: 941428
Depends on: 943748
Depends on: 943759
Depends on: 943765
Disabled on pre-Australis builds in bug 941385.
Alias: fx-UITour
Hardware: x86 → All
Depends on: 949663
Depends on: 949659
Depends on: 956160
Depends on: 956162
Depends on: 956202
Depends on: 958300
Depends on: 958679
Depends on: 958673
Depends on: 963212
Depends on: 952568
Depends on: 952567
Depends on: 966014
Depends on: 966068
Depends on: 966072
No longer depends on: 966072
Depends on: 966241
Depends on: 966913
Depends on: 967391
Depends on: 967625
Depends on: 968039
Depends on: 969350
Depends on: 969374
Depends on: 969370
Depends on: 969359
Depends on: 969377
Depends on: 969353
Depends on: 968838
Depends on: 970321
Depends on: 970788
Depends on: 972566
Depends on: 974086
No longer depends on: 969377
Depends on: 977638
Depends on: 979599
Depends on: 980220
Depends on: 980746
Depends on: 982620
No longer depends on: 982620
Depends on: 984042
Depends on: 985489
Depends on: 985532
Depends on: 985933
No longer depends on: 985933
Depends on: 987480
Depends on: 987676
Depends on: 988144
Depends on: 989101
Depends on: 989947
Depends on: 991081
No longer depends on: 991081
Depends on: 993366
No longer depends on: 993366
Depends on: 987407
Depends on: 996616
Depends on: 997142
Depends on: 997719
Depends on: 1003924
Depends on: 1003031
Flags: sec-review?(dveditz)
Depends on: 506446
Depends on: 1068401
Depends on: 1065525
Depends on: 1063698
Depends on: CVE-2015-0819
Depends on: 1081772
Depends on: fx-UITour-Hello
Depends on: 1085330
Depends on: 1087230
Depends on: 1091785
Depends on: 1095519
Depends on: 1096903
Depends on: 1096544
Depends on: 1097587
Depends on: 1097600
Depends on: 1098071
Depends on: 1098482
Depends on: 1100343
Depends on: 1101669
Depends on: 1107488
Depends on: 1109868
Depends on: 1110602
Moving open UITour bugs to Firefox::Tours. Filter on firefox-tours-20150121.
Component: General → Tours
You need to log in before you can comment on or make changes to this bug.