1233773 - [experiment] TP UI Tour should return to the previous page when not invoked from Private Windows

Status: RESOLVED FIXED

(www.mozilla.org :: Pages & Content, defect, P1)

Description

[:Paolo Amadini]

When the UITour API in bug 1233771 is ready, it should be used for the last step of the TP UI tour.

Comment 1

[:Paolo Amadini]

Tryserver build with preliminary support for Mozilla.UITour.closeTab():

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5874978856b8

Comment 2

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #1)
> Tryserver build with preliminary support for Mozilla.UITour.closeTab():

Note that the test failures are due to the fact that this build also enables the doorhanger on the first use of TP, but they don't prevent the build from working.

Comment 3

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #1)
> Tryserver build with preliminary support for Mozilla.UITour.closeTab():
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5874978856b8

I tested the try-build running the tour locally both in a private and non-private window, and calling Mozilla.UITour.closeTab() seemed to work as expected. 

It's worth noting that in the case of the private window, it closed the window since it was the only active tab.

Comment 4

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #2)
> Note that the test failures are due to the fact that this build also enables
> the doorhanger on the first use of TP, but they don't prevent the build from
> working.

I should also note a slight regression I noticed here when launching the tracking protection tour on step 1. Because the first use of TP might also be when someone launches the tour in a private window, this change results in a slight flicker on the first door-hanger appearing. I'm guessing because it is launched via the product, and then again straight after via the tour page loading. Everything still work ok, but we might want to add an exception here?

Comment 5

[:Paolo Amadini]

I think I'll use the query parameters "?step=2&doorhanger=true" when invoking the tour from the doorhanger. I also thought about something more generic like "?step=2&closewhenfinished=true" in case the UI for invoking it changes, not sure what is best. Let me know what name you'd like to have.

The Mozilla.UITour.closeTab API will do nothing if this is the last tab in the window. So, I guess the final version of the tour, when the "Got It" button is pressed, can just call "closeTab" and after a short timeout it can navigate to the last page with the "Restart Tour" message, like it does now. The result is that this last step would only be displayed if the user closed the original page in the meantime.

Comment 6

[:Paolo Amadini]

(In reply to Alex Gibson [:agibson] from comment #4)

Filed bug 1236231.

Comment 7

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #5)
> I think I'll use the query parameters "?step=2&doorhanger=true" when
> invoking the tour from the doorhanger. I also thought about something more
> generic like "?step=2&closewhenfinished=true" in case the UI for invoking it
> changes, not sure what is best. Let me know what name you'd like to have.

I'm happy with anything short and reasonably descriptive, so perhaps doorhanger=true, or newtab=true? I'll let you decide.

> 
> The Mozilla.UITour.closeTab API will do nothing if this is the last tab in
> the window. So, I guess the final version of the tour, when the "Got It"
> button is pressed, can just call "closeTab" and after a short timeout it can
> navigate to the last page with the "Restart Tour" message, like it does now.
> The result is that this last step would only be displayed if the user closed
> the original page in the meantime.

Relying on a delay after a button click doesn't sound ideal from a UX perspective, but hopefully this will be enough of an edge case that not many will see it.

Comment 8

[:Paolo Amadini]

(In reply to Alex Gibson [:agibson] from comment #7)
> I'm happy with anything short and reasonably descriptive, so perhaps
> doorhanger=true, or newtab=true? I'll let you decide.

I like "newtab=true", it's independent from the UI that starts the tour.

Comment 9

[:Paolo Amadini]

This build uses "newtab=true", and UITour.closeTab only works if the tour isn't in the last tab:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c580c3863a3

Comment 10

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #9)
> This build uses "newtab=true", and UITour.closeTab only works if the tour
> isn't in the last tab:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c580c3863a3

Thanks, will give this a test again shortly

Comment 11

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #9)
> This build uses "newtab=true", and UITour.closeTab only works if the tour
> isn't in the last tab:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c580c3863a3

Try build seems to work well. I've made a patch for the mozorg side and will push this to a demo server today.

Comment 12

[Alex Gibson [:agibson]]

I've pushed a patch up to demo2, which can be tested together with the try build linked in Comment 9

https://www-demo2.allizom.org/en-US/firefox/46.0a1/tracking-protection/start/

Please note for the tour to work on the demo2 server, you will need to whitelist https://www-demo2.allizom.org for UITour:

http://bedrock.readthedocs.org/en/latest/uitour.html#local-development

To test launching the tour in-product, you will also need to set the tour URL to point to demo2 using these instructions: http://bedrock.readthedocs.org/en/latest/uitour.html#tracking-protection-uitour

Comment 13

[:Paolo Amadini]

I've also tried it and works nicely! I think we can already move forward with the review of the required site changes.

We can move the code to production when the Desktop API is also reviewed, or even earlier in case you're fine with separately handling any possible API changes that result from the Desktop side review.

Comment 14

[Alex Gibson [:agibson]]

(In reply to :Paolo Amadini from comment #13)
> I've also tried it and works nicely! I think we can already move forward
> with the review of the required site changes.
> 
> We can move the code to production when the Desktop API is also reviewed, or
> even earlier in case you're fine with separately handling any possible API
> changes that result from the Desktop side review.

Great, thanks! - I'll hang on for the Desktop side review, just in case there are any review changes. The website code changes are fairly simple, so I don't think it should take long to reach production once we're ready.

Comment 15

[Alex Gibson [:agibson]]

Attachment: GitHub pull request

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/939a2f01b4f5fe7171d34945e5adcb8bea254959
[fix bug 1233773] TP UITour should close tab when not invoked from Private Window

https://github.com/mozilla/bedrock/commit/2e740ce43354466a7d053d1697629b39857ae4c9
Merge pull request #3788 from alexgibson/bug-1233773-tp-tour-close-tab

[fix bug 1233773] TP UITour should close tab when not invoked from Private Window

Comment 17

[Alex Gibson [:agibson]]

This has been merged to master. Will update the bug here once things are on production.

Comment 18

[Alex Gibson [:agibson]]

This has been pushed to production, thanks all.