Closed Bug 1233771 Opened 8 years ago Closed 8 years ago

[experiment] Add a UITour API to close the current tab and return to the previous page

Categories

(Firefox :: Tours, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 46
Iteration:
46.3 - Jan 25
Tracking Status
firefox46 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [fxprivacy])

Attachments

(1 file)

We need a UITour API to invoke during the last step of the TP tour, to make it close the current tab and return to the previous page, instead of restarting the tour.
Flags: qe-verify?
Priority: -- → P1
Blocks: 1233773
We also need to keep in mind that this API won't be part of Firefox 42, which also uses the same tour code/logic. We need a solution that will work for both use cases - perhaps something we should discuss if you do plan on implementing a new API for this.
(In reply to Alex Gibson [:agibson] from comment #1)
> We also need to keep in mind that this API won't be part of Firefox 42,
> which also uses the same tour code/logic. We need a solution that will work
> for both use cases - perhaps something we should discuss if you do plan on
> implementing a new API for this.

Can simple feature detection work for you here? Like this pseudo-code:

if (UITour.closeTab && URLParameterPresent("usereturnbutton")) {
  // Display alternate finish button that does: UITour.closeTab();
}
Flags: qe-verify? → qe-verify-
It depends on if we have a feature to detect or not? This is kind of down to the proposed API. 

UITour.closeTab could just be a method in the library, which would work in the newer version but not in FF 42 for example. So not something that would be used as a feature detect. If we're using a URL param to signal something, then that's workable. It would probably help if we discussed this on Monday so we're all clear on how the tour is put together.
Summary: Add a UITour API to close the current tab and return to the previous page → [experiment] Add a UITour API to close the current tab and return to the previous page
We just discussed this one. Two things:

1. The user is only going to see the doorhanger drop if they've turned on TP outside of PBM, because the only way we can save state is if they've turned it on outside PBM. But it doesn't matter if they see the doorhanger in normal mode or PBM, we dont' need logic to only drop the doorhanger outside PBM. They can see the hanger in either place and we can save state around them clicking it because we won't know where they interacted with it. This simplifies logic a lot. 

2. We'll pass a new query string param to the web tour page that is something like "path=secondary" or "doorhanger=true" or whatever. This will indicate to the web page that the user is entering the tour through the secondary path and that a new tab is opened. Then on the final tour page, this new query param will tell the tour to display a "close this tab" button to return to the prior page.

We'll need copy for that final workflow.
Attachment #8700704 - Flags: review?(MattN+bmo)
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28783/diff/1-2/
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

This is a preliminary version of the closeTab API that we would use to return to the previous page once a TP tour started from the normal mode doorhanger is finished.

One improvement we could make is to prevent the browser from being closed if this is the last tab in the window, in case the user closes the original tab before finishing the tour.

I'm not clear about the windowless browser case mentioned in the patch, but I saw some regression tests for it. Do we have to worry about it?
Attachment #8700704 - Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
A reminder that there is documentation on the UITour API here that we may want to update:

http://bedrock.readthedocs.org/en/latest/uitour.html
Keywords: dev-doc-needed
Iteration: 46.1 - Dec 28 → 46.2 - Jan 11
https://reviewboard.mozilla.org/r/28783/#review26099

I would prefer that we avoid the need for this API in the first place as it doesn't seem broadly useful and it opens the door to affect which tab is shown. It's fairly simple though so I'm okay with it if we don't have other solutions.

::: browser/components/uitour/UITour.jsm:740
(Diff revision 1)
> +        window.gBrowser.removeTab(window.gBrowser.selectedTab);

I'm at bit nervous about this since I think our concept of whether the tour tab is selected can already be out-of-sync in some cases and the solution to that seemed quite complex. See bug 1123010 and related for an example

I would feel better looking up the tab to use via `browser` which comes from the message manager where security checks were done.

I also think we need to make sure this doesn't quit Firefox if it's the only window+tab open.
Attachment #8700704 - Flags: feedback?(MattN+bmo) → feedback+
Attachment #8700704 - Flags: feedback+ → review?(MattN+bmo)
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28783/diff/1-2/
(In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #9)
> I would prefer that we avoid the need for this API in the first place as it
> doesn't seem broadly useful and it opens the door to affect which tab is
> shown. It's fairly simple though so I'm okay with it if we don't have other
> solutions.

Yeah, we need this API for the doorhanger tour flow, and we've tested it in bug 1233773. I've updated the code to ignore the request if this is the last tab as anticipated in comment 7, I've updated the way we look for the tab as you suggested, and I think the patch is ready for final review.
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

https://reviewboard.mozilla.org/r/28783/#review26119

::: browser/components/uitour/UITour.jsm:740
(Diff revision 2)
> +        // is windowless, just ignore the request to close the tab. The request
> +        // is also ignored if this is the only tab in the window.
> +        let tabBrowser = browser.ownerDocument.defaultView.gBrowser;
> +        if (tabBrowser && tabBrowser.browsers.length > 1) {

Why don't we want this to work in single-tab windows that aren't the only window?

For the case where we are in the last window and it has a single tour tab, simply doing nothing for a closeTab call may lead to perceived brokenness by the user which isn't ideal. Is there no way that will happen in practice for the proposed usage? If not, we should probably have a plan to deal with that case (e.g. replace the tab with the user's homepage on non-OSX).
Attachment #8700704 - Flags: review?(MattN+bmo)
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

(In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #12)
> Why don't we want this to work in single-tab windows that aren't the only
> window?

What we're trying to avoid here is just that the entire browser might close unexpectedly if the user closed the original page. This is guaranteed by this simple check. At present, I think that checking for other windows will not be necessary and only adds some code complexity, even if small.

> For the case where we are in the last window and it has a single tour tab,
> simply doing nothing for a closeTab call may lead to perceived brokenness by
> the user which isn't ideal. Is there no way that will happen in practice for
> the proposed usage? If not, we should probably have a plan to deal with that
> case (e.g. replace the tab with the user's homepage on non-OSX).

When the tab does not close, we display the final page of the tour after a small delay, so this does not result in perceived brokenness. This was the simplest solution to our immediate need.

If you want, you can try this patch together with the current UI tour on the www-demo2 server as described in bug 1233773 comment 12, and see how this works in practice.

The current design of the API does not prevent us from extending it or adding new checks for other windows if needed by other UI tours in the future.
Attachment #8700704 - Flags: review?(MattN+bmo)
Iteration: 46.2 - Jan 11 → 46.3 - Jan 25
Comment on attachment 8700704 [details]
MozReview Request: Bug 1233771 - Add a UITour API to close the current tab and return to the previous page.

https://reviewboard.mozilla.org/r/28783/#review27645

Thanks for the test

::: browser/components/uitour/UITour-lib.js:307
(Diff revision 2)
>  	};
> +
> +    Mozilla.UITour.closeTab = function() {
> +        _sendEvent('closeTab');

Identation here is off

::: browser/components/uitour/UITour-lib.js:309
(Diff revision 2)
> +    Mozilla.UITour.closeTab = function() {
> +        _sendEvent('closeTab');
> +    };

Nit: I've been thinking about the Javascript API section at https://bedrock.readthedocs.org/en/latest/uitour.html#javascript-api and think that it would be better for this documentation to exist in m-c with the responsible code (as bedrock isn't the only consumer) so I think we should start to document the methods in this file with JSDoc syntax. We can generate the equivalent of the Read The Docs section by running jsdoc on UITour-lib.js so we don't have to manually maintain it.

For this method can you document that the last tab in a window won't be closed.

(In reply to :Paolo Amadini from comment #13)
> Comment on attachment 8700704 [details]
> MozReview Request: Bug 1233771 - Add a UITour API to close the current tab
> and return to the previous page.
> 
> (In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #12)
> > Why don't we want this to work in single-tab windows that aren't the only
> > window?
> 
> What we're trying to avoid here is just that the entire browser might close
> unexpectedly if the user closed the original page. This is guaranteed by
> this simple check. At present, I think that checking for other windows will
> not be necessary and only adds some code complexity, even if small.

See my assumption from below for why I thought this could be a UX problem.

> > For the case where we are in the last window and it has a single tour tab,
> > simply doing nothing for a closeTab call may lead to perceived brokenness by
> > the user which isn't ideal. Is there no way that will happen in practice for
> > the proposed usage? If not, we should probably have a plan to deal with that
> > case (e.g. replace the tab with the user's homepage on non-OSX).
> 
> When the tab does not close, we display the final page of the tour after a
> small delay, so this does not result in perceived brokenness. This was the
> simplest solution to our immediate need.

I see. My assumption was that this API would be used from a button click indicating it would close the tab or something similar which would lead to the poor UX that I describe if nothing ends up happening. It seems like you intend to handle that case in this specific tour so I guess it's OK but it seems like we're adding a footgun for future tours if consumers of this API don't realize that it doesn't always close a tab.

> The current design of the API does not prevent us from extending it or
> adding new checks for other windows if needed by other UI tours in the
> future.

Sure, I'd just rather we don't have to make those changes once we notice the issue in production due to incorrect assumptions about what this API does.
Attachment #8700704 - Flags: review?(MattN+bmo) → review+
As far as the last tab closing UX issue goes, I agree it's not ideal. I'm sure people will want to use this in future tours as well, where it could likely be an issue. I'll leave it to you both to decide, but I think the current unpredictable UX will likely become a source of future bugs / hacks to work around.
I've added the comment about the API usage directly in the file.

I think that initially UI tours would have to handle the case where the API does nothing anyways, in case the tour is executed in an older version of Firefox. Maybe what we'll end up needing is a separate API to query whether the tab can be closed, so tours can display different UI. For the moment, what we have is enough to proceed with the TP tour improvements.
https://hg.mozilla.org/mozilla-central/rev/27ec60a34ac0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(In reply to :Paolo Amadini from comment #0)
> We need a UITour API to invoke during the last step of the TP tour, to make
> it close the current tab and return to the previous page, instead of
> restarting the tour.
Couple of questions here:
1. How about closing the tour at step 2? It shows now the "restart tour" page, without returning back.
2. While the step 3 is displayed, open a new empty tab and move it before the tour tab. Now, completing the last step of the tour, returns me to the previous tab in the tab bar (empty tab in our case), and not necessarily to the page the tour was started from.
Thoughts?
Flags: needinfo?(paolo.mozmail)
(In reply to Paul Silaghi, QA [:pauly] from comment #19)
> 1. How about closing the tour at step 2? It shows now the "restart tour"
> page, without returning back.

Good catch, you may file a bug on this for the mozilla.org UI Tour team to evaluate.

> 2. While the step 3 is displayed, open a new empty tab and move it before
> the tour tab. Now, completing the last step of the tour, returns me to the
> previous tab in the tab bar (empty tab in our case), and not necessarily to
> the page the tour was started from.

This is expected, we're using normal tab close logic, that will return to the previous page in most cases, where users don't interact with other tabs in the meantime.
Flags: needinfo?(paolo.mozmail)
Depends on: 1245521
Depends on: 1245107
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: