Closed
Bug 862998
(fx-UITour)
Opened 12 years ago
Closed 11 years ago
Add glue to allow Firefox first run page to highlight UI elements
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
RESOLVED
FIXED
Firefox 27
People
(Reporter: cers, Assigned: Unfocused)
References
(Depends on 20 open bugs, )
Details
(Whiteboard: [Australis:P2])
Attachments
(3 files, 10 obsolete files)
(deleted),
patch
|
Dolske
:
review+
Unfocused
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dolske
:
review+
Unfocused
:
checkin+
|
Details | Diff | 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
Reporter | ||
Comment 1•12 years ago
|
||
Forgot to add file to hg
Attachment #738678 -
Attachment is obsolete: true
Reporter | ||
Comment 2•12 years ago
|
||
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
Reporter | ||
Comment 3•11 years ago
|
||
Updated version of the patch. Applies cleanly to ux tip, but I haven't tested in it.
Attachment #742237 -
Attachment is obsolete: true
Reporter | ||
Comment 4•11 years ago
|
||
Updated the patch to work with the new bookmark button
Attachment #761120 -
Attachment is obsolete: true
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
Awesome. Thanks, Blair! I'll check it out today.
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Comment 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
Loading Christian's URL doesn't trigger any interaction within the chrome. Should it? I only have web interaction.
Comment 12•11 years ago
|
||
"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)
Reporter | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
(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
Comment 15•11 years ago
|
||
(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 :)
Comment 16•11 years ago
|
||
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?
Comment 17•11 years ago
|
||
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
Assignee | ||
Comment 18•11 years ago
|
||
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.
Assignee | ||
Comment 19•11 years ago
|
||
(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"
Comment 20•11 years ago
|
||
(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!
Assignee | ||
Comment 21•11 years ago
|
||
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.
Assignee | ||
Comment 22•11 years ago
|
||
Bah, could have sworn I remembered uploading an earlier version of this...
Attachment #761140 -
Attachment is obsolete: true
Attachment #780920 -
Flags: feedback?(dolske)
Comment 23•11 years ago
|
||
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!
Assignee | ||
Comment 24•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [Australis:P2]
Comment 25•11 years ago
|
||
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+
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
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 28•11 years ago
|
||
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+
Assignee | ||
Comment 29•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
(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.
Assignee | ||
Comment 31•11 years ago
|
||
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)
Assignee | ||
Comment 32•11 years ago
|
||
And this is needed only on UX branch.
Attachment #805273 -
Flags: review?(dolske)
Assignee | ||
Comment 33•11 years ago
|
||
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)
Assignee | ||
Comment 34•11 years ago
|
||
This is needed only on mozilla-central - UX already has this change.
Attachment #805919 -
Flags: review?(dolske)
Assignee | ||
Comment 35•11 years ago
|
||
And this is needed only on UX, for compatibility with the changes there.
Attachment #805920 -
Flags: review?(dolske)
Assignee | ||
Updated•11 years ago
|
Comment 36•11 years ago
|
||
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.
Assignee | ||
Comment 37•11 years ago
|
||
(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 :)
Comment 38•11 years ago
|
||
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 :)
Updated•11 years ago
|
Attachment #805919 -
Flags: review?(dolske) → review+
Updated•11 years ago
|
Attachment #805920 -
Flags: review?(dolske) → review+
Comment 39•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Flags: sec-review?
Updated•11 years ago
|
Flags: sec-review? → sec-review?(dveditz)
Assignee | ||
Comment 40•11 years ago
|
||
(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.
Assignee | ||
Comment 41•11 years ago
|
||
Now with tests!
Attachment #805918 -
Attachment is obsolete: true
Attachment #815282 -
Flags: review?(dolske)
Comment 42•11 years ago
|
||
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+
Assignee | ||
Comment 43•11 years ago
|
||
(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.
Assignee | ||
Comment 44•11 years ago
|
||
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]
Assignee | ||
Updated•11 years ago
|
Attachment #805919 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #815282 -
Flags: checkin+
Comment 45•11 years ago
|
||
had to backout https://hg.mozilla.org/integration/fx-team/rev/491b452af425 because of test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=29235733&tree=Fx-Team
Assignee | ||
Comment 46•11 years ago
|
||
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
Assignee | ||
Comment 47•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Comment 48•11 years ago
|
||
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]
Comment 49•11 years ago
|
||
Updated•11 years ago
|
Target Milestone: --- → Firefox 27
Assignee | ||
Updated•11 years ago
|
Comment 51•11 years ago
|
||
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
Comment 52•11 years ago
|
||
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.
Comment 53•11 years ago
|
||
> * Please make sure that this cannot be abused in a security respect.
FWIW, from that point of view, mozilla.org website is untrusted.
Comment 54•11 years ago
|
||
Whiteboard: [Australis:P2][fixed-in-ux] → [Australis:P2]
Comment 55•11 years ago
|
||
(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.
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 56•11 years ago
|
||
> 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..
Comment 57•11 years ago
|
||
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.
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•11 years ago
|
Comment 58•11 years ago
|
||
disabled |
Disabled on pre-Australis builds in bug 941385.
Alias: fx-UITour
status-firefox27:
--- → disabled
status-firefox28:
--- → disabled
Hardware: x86 → All
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Assignee | ||
Updated•10 years ago
|
Flags: sec-review?(dveditz)
Updated•10 years ago
|
Depends on: CVE-2015-0819
Updated•10 years ago
|
Depends on: fx-UITour-Hello
Comment 60•10 years ago
|
||
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.
Description
•