Should add one button which opens private browsing window in the Private Browsing onBoarding tour

(In reply to Fischer [:Fischer] from comment #0) > Should add one button which opens private browsing window in the Private > Browsing onBoarding tour The latest specs have become as adding one button in the Private Browsing onBoarding tour which highlights the Private Window button in the hamburger menu

Suppose we can dupe this. Let's close it and just reopen it if the works there are too much to do.

remove whiteboard tag due to its duplicated

Reopened for the reason addressed in bug 1357046.

(In reply to KM Lee [:rexboy] from bug 1357046 comment #37) > (In reply to Dave Townsend [:mossop] from bug 1357046 comment #29) > > That's a strange error, I'd expect something else if we couldn't load the > > url. This does make me wonder what the security aspects of loading the code > > like this are. Can we instead inject a <script> element into the document to > > make sure it is the document doing the loading? > > Injecting script tag doesn't work either. The error keeps the same as long > as the window is opened in the content process (which is about:home and > activity stream case). I'd still like to use the <script> method of running the script in the webpage, it will ensure that we're using the webpage's privileges to run it safely. Have you tried using a resource URI instead of a chrome one. Both should work from the content process in theory but maybe because we're loading from chrome we're getting the wrong privileges.

I think we don't need to include the full UITour-lib into onboarding. Since the only function we use in UITour is `showHighlight`, we can pick the message sending portion from UITour and do it in onboarding. I tested the following script on about:home, about:newtab, and activity-stream, they all works fine. Put `_sendEvent` as one of the onboarding method ``` _sendEvent(action, data) { var event = new this._window.CustomEvent("mozUITour", { bubbles: true, detail: { action, data: data || {} } }); this._window.document.dispatchEvent(event); } ``` Embed this in button action (this might be too simple to not worth wrap another layer) ``` this._sendEvent("showHighlight", { target: "addons" }); ```

(In reply to Fred Lin [:gasolin] from comment #7) > I think we don't need to include the full UITour-lib into onboarding. > > Since the only function we use in UITour is `showHighlight`, we can pick the > message sending portion from UITour and do it in onboarding. I tested the > following script on about:home, about:newtab, and activity-stream, they all > works fine. > > > Put `_sendEvent` as one of the onboarding method > ``` > _sendEvent(action, data) { > var event = new this._window.CustomEvent("mozUITour", { > bubbles: true, > detail: { > action, > data: data || {} > } > }); > > this._window.document.dispatchEvent(event); > } > ``` > Just discussed this offline and we think we're not sure if it's an API that keep unchanged, and there's no similar way of using the library like that. So maybe we should try using UI-Tourlib.js as a whole first.

(In reply to Dave Townsend [:mossop] from comment #6) > I'd still like to use the <script> method of running the script in the > webpage, it will ensure that we're using the webpage's privileges to run it > safely. > > Have you tried using a resource URI instead of a chrome one. Both should > work from the content process in theory but maybe because we're loading from > chrome we're getting the wrong privileges. I tried to move UITour-lib.js as resource:// but the result is the same. I can't access Mozilla.UITour created from content window from framescript. But scripts inside content window can access it. I was told it's blocked intentionally for content window (but I didn't remember the detail for now, I'll put the corresponding details if needed). So some solutions came to me are to move some code to content window altogether. The selections may be like 1. Just move the event listener out to content window. And when we need to communicate something between listener and framescript we just dispatch a new CustomEvent. 2. Move almost all the code out to the content window, but we're not able to move them out completely since we still need some code from framescript for jobs like accessing properties in the future. 3. other possibility? I personally more prefer the 1st choice and just made a sample patch based on that. Mossop how do you think about it?

(In reply to KM Lee [:rexboy] from comment #10) > (In reply to Dave Townsend [:mossop] from comment #6) > > I'd still like to use the <script> method of running the script in the > > webpage, it will ensure that we're using the webpage's privileges to run it > > safely. > > > > Have you tried using a resource URI instead of a chrome one. Both should > > work from the content process in theory but maybe because we're loading from > > chrome we're getting the wrong privileges. > > I tried to move UITour-lib.js as resource:// but the result is the same. I > can't access Mozilla.UITour created from content window from framescript. > But scripts inside content window can access it. > I was told it's blocked intentionally for content window (but I didn't > remember the detail for now, I'll put the corresponding details if needed). Oh I didn't realise you were trying to access it from the framescript. You'd need to bypass the X-ray wrappers to get at the object in the content window. > So some solutions came to me are to move some code to content window > altogether. The selections may be like > 1. Just move the event listener out to content window. And when we need to > communicate something between listener and framescript we just dispatch a > new CustomEvent. > 2. Move almost all the code out to the content window, but we're not able to > move them out completely since we still need some code from framescript for > jobs like accessing properties in the future. > 3. other possibility? > > I personally more prefer the 1st choice and just made a sample patch based > on that. Mossop how do you think about it? This sounds like a sane approach to me. I'll review the patch now.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review151940 This looks good, please just double check the button wording before landing. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:14 (Diff revision 1) > onboarding.tour-search.description=Access all of your favorite search engines with a click. Search the whole Web or just one website right from the search box. > +onboarding.tour-search.button=Open One-Click Search > onboarding.tour-private-browsing=Private Browsing > onboarding.tour-private-browsing.title=Browser by yourself. > onboarding.tour-private-browsing.description=There's no reason to share your online life with trackers every time you browse. Want to keep something to yourself? Use Private Browsing with Tracking Protection. > +onboarding.tour-private-browsing.button=Show Me Private Browsing This prototype suggests the button should be "Show Me Where To Find Private Browsing". Is there a more up to date spec? https://dl.dropboxusercontent.com/u/105549/tour2.framer/index.html

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152210 Thank you for the works. ::: browser/extensions/onboarding/content/onboarding.css:172 (Diff revision 1) > width: 100%; > height: 100%; > border: none; > } > > +.onboarding-tour-page.no-button > .onboarding-tour-content { Nit: please use `.onboarding-no-button` since we are scoping our selectors and have doen this with other selectors, thanks.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152214 Nice to got some progress here! To make a correct button action I think we should also take `setCompleted` into consideration ::: browser/extensions/onboarding/content/onboarding-tour-agent.js:10 (Diff revision 1) > +"use strict"; > + > +document.getElementById("onboarding-overlay-dialog") > + .addEventListener('click', evt => { > + switch(evt.target.id){ > + case "onboarding-tour-search-button": let's organize with alphabet order ::: browser/extensions/onboarding/content/onboarding-tour-agent.js:11 (Diff revision 1) > + > +document.getElementById("onboarding-overlay-dialog") > + .addEventListener('click', evt => { > + switch(evt.target.id){ > + case "onboarding-tour-search-button": > + Mozilla.UITour.openSearchPanel(() => {}); Though `setCompleted` is now a placeholder, we still need to call `setCompleted` from the target tour to mark the call-to-action as completed (and implement the action later). So to complete a button action, we need find a way to access `tour.setCompleted` in this script.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152258 ::: browser/extensions/onboarding/content/onboarding.css:204 (Diff revision 1) > + > +.onboarding-tour-button > button:active { > + background: #0881dd; > +} > + > + a redundant line here

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152264 ::: browser/extensions/onboarding/content/onboarding.css:188 (Diff revision 1) > + display: none; > + grid-row: tour-page-end; > + grid-column: tour-page-end; > +} > + > +.onboarding-tour-button > button { put button in grid can't fulfill the spec position requirement. I suggest use `position: absolute` (as close button) instead. With this way we don't need to add `onboarding-no-button` class as well. Here's the style I've tested ``` .onboarding-tour-button > button { position: absolute; bottom: 30px; offset-inline-end: 26px; padding: 10px 20px; font-size: 15px; font-weight: 600; line-height: 21px; background: #0d96ff; border: none; border-radius: 3px; color: #fff; box-shadow: 0 1px 0 rgba(0,0,0,0.23); cursor: pointer; } ```

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152264 > put button in grid can't fulfill the spec position requirement. I suggest use `position: absolute` (as close button) instead. With this way we don't need to add `onboarding-no-button` class as well. > > Here's the style I've tested > > ``` > .onboarding-tour-button > button { > position: absolute; > bottom: 30px; > offset-inline-end: 26px; > padding: 10px 20px; > font-size: 15px; > font-weight: 600; > line-height: 21px; > background: #0d96ff; > border: none; > border-radius: 3px; > color: #fff; > box-shadow: 0 1px 0 rgba(0,0,0,0.23); > cursor: pointer; > } > ``` I saw you modified that in the polish bug, should I just leave that to you?

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152214 > Though `setCompleted` is now a placeholder, we still need to call `setCompleted` from the target tour to mark the call-to-action as completed (and implement the action later). > So to complete a button action, we need find a way to access `tour.setCompleted` in this script. For me this was in the scope of other bugs for checking tour's completion, but it's easy to fix. I've included that in the revised version.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review151940 > This prototype suggests the button should be "Show Me Where To Find Private Browsing". Is there a more up to date spec? > > https://dl.dropboxusercontent.com/u/105549/tour2.framer/index.html Since we got l10n string updated and l10n review in bug 1357046, I made the l10n string of the button there to reduce some review process. So no more l10n strings in this patch.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152846

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152264 > I saw you modified that in the polish bug, should I just leave that to you? Please fix in this bug. We may need to wait the final images in the polish bug.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review152264 > Please fix in this bug. We may need to wait the final images in the polish bug. The easiest solution for me is to stretch the range of the page grid to the bottom of the page, see the patch. Although page grid overlaps footer, but as long as we don't write page description and footer super long it should be fine, and we can place button near the bottom to met the latest spec.

Comment on attachment 8876111 [details] Bug 1357049 - add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. https://reviewboard.mozilla.org/r/147540/#review153258 please make sure other tests green before landing ::: browser/extensions/onboarding/content/onboarding.js:128 (Diff revision 5) > this._overlayIcon.addEventListener("click", this); > this._overlay.addEventListener("click", this); > // Destroy on unload. This is to ensure we remove all the stuff we left. > // No any leak out there. > this._window.addEventListener("unload", () => this.destroy()); > + this._window.addEventListener("onboardingAgentEvent", this); don't need this now

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8da76b2d3d1c add one button in the Private Browsing and Search tour of onBoarding overlay to demonstrate the corresponding features. r=Fischer,gasolin,mossop

I have seen the feature being implemented with latest Nightly 56.0a1 on Windows 10 (64 bit). This bug's fix is now verified in Latest Nightly. Build ID : 20170616030207 User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 [bugday 20170614]

This issue has been verified on Win 10 x64.