1122213 - Investigation: two tour tabs open at the same time

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

This bug is to discuss interactions with tours in the case where two tour tabs are open at the same time.

What happens now?
What should happen?

Comment 1

[Justin Dolske [:Dolske]]

Specifically, this is for the case of:

1) User is shown a whatsnew/firstrun tour page upon upgrading/installing Firefox
2) That page promotes Hello, by opening the Hello panel and inviting then to click Get Started
3) The user clicks Get Started, which opens up another tab containing the Hello-specific tour.

In theory this should be fine, but at the very least we certainly haven't tested this. Matt said we may have some existing bugs due to the old pinned-tabs tour support (where we try to persist open info panels / hilights between tabs), which may be undesirable now.

Comment 2

[Matthew N. [:MattN]]

Yeah, we had code that said if the tab we're switching to is also a tour tab then don't teardown which means menus or annotations may stay open when switching tabs. I've removed this code from Nightly in bug 1122830 now so we should test this and we may need to uplift bug 1122830 (which may happen due to conflicts anyways).

Comment 3

[Justin Dolske [:Dolske]]

Is there any remaining investigation to do here?

Comment 4

[Matthew N. [:MattN]]

Yeah, someone should do the manual testing (investigation) on current Nightly.

The workaround (see bug 1125764) that the pages are using until bug 1123010 is fixed in Firefox could lead to problems relating to race conditions depending on the timing of visibilitychange events and whether the page is also using setTimeout in the event handlers.

Example problem: User switches from tour tab A to B.
1) Tour tab B opens some tour UI
2) Tour tab A hides tour UI since we're switching away (intending to only hide tour UI related to tour A)

Once Firefox handles teardowns properly in all cases itself then I think we'll be better off. The other issue related to teardown was fixed (see comment 2).

Another possible problem may be related to the Hello panel being already opened when the user lands on a tour page. I think in that case the somewhat known issue is that the panel won't be sticky and there isn't much we can do about that unless we are okay with a flicker from re-opening it as sticky (XUL popup limitation due to OS integration). I could see this happening if e.g. an about:home snippet opens the panel and the tour somehow loads in that about:home tab (see bug 1123010) e.g. if the snippet links directly to the tour from the snippet text (which I don't think we're planning on doing anymore).

I guess some of this is more important to test once we actually have snippets and completed tours to test with but some testing can be done in advance with the 35 tour and test pages.

Comment 5

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] from comment #4)
> The workaround (see bug 1125764) that the pages are using until bug 1123010
> is fixed in Firefox could lead to problems relating to race conditions
> depending on the timing of visibilitychange events and whether the page is
> also using setTimeout in the event handlers.
> 
> Example problem: User switches from tour tab A to B.
> 1) Tour tab B opens some tour UI
> 2) Tour tab A hides tour UI since we're switching away (intending to only
> hide tour UI related to tour A)

Once my Nightly updates and I have the fix for bug 1125764, I'll see if I can do some manual testing on this.

> Another possible problem may be related to the Hello panel being already
> opened when the user lands on a tour page. I think in that case the somewhat
> known issue is that the panel won't be sticky and there isn't much we can do
> about that unless we are okay with a flicker from re-opening it as sticky
> (XUL popup limitation due to OS integration). I could see this happening if
> e.g. an about:home snippet opens the panel and the tour somehow loads in
> that about:home tab (see bug 1123010) e.g. if the snippet links directly to
> the tour from the snippet text (which I don't think we're planning on doing
> anymore).

I think we may be ok here - maybe Cory can confirm, but as far as I know we're only planning on sending users to the Hello FTE by prompting them to click "Get Started" in the info panel. In this case, the panel should close when they click the button and then re-open in a new tab when the FTE page loads.

Comment 6

[Alex Gibson [:agibson]]

(In reply to Alex Gibson [:agibson] from comment #5)
> I think we may be ok here - maybe Cory can confirm, but as far as I know
> we're only planning on sending users to the Hello FTE by prompting them to
> click "Get Started" in the info panel. In this case, the panel should close
> when they click the button and then re-open in a new tab when the FTE page
> loads.

To be clear, when I said "info panel" here I meant to say the Hello panel.

Comment 7

[Alex Gibson [:agibson]]

Ok I've done some initial testing in Nightly. Generally things seem ok, I have however discovered one bug so far:

STR:

1.) In Nightly click Help -> Nightly Tour to open (#tab1).
2.) Once the tour page opens, open a second tour (#tab2) by clicking Help -> Nightly Tour.
3.) Close #tab1 by clicking (x).

Expected results:

The door-hanger in #tab2 should remain visible

Actual results:

When the #tab1 closes, the door-hanger in #tab2 gets torn down.

I imaging if someone launches the FTE from the /firstrun or /whatsnew tour, they might decide to close the first tour tab - which would result in the FTE Hello panel and door-hanger on the first step being hidden unintentionally.

Hope that makes sense?

Comment 8

[Alex Gibson [:agibson]]

I filed Bug 1127810 for what I found in Comment 7

Comment 9

[Matthew N. [:MattN]]

Thanks, that's the type of problem I was worried about.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Hey is there any more work needed here or can we close this now?

Comment 11

[Alex Gibson [:agibson]]

I think we should probably keep this bug open while we build the 36.0 tours, as we may still find something else. I filled Bug 1127810 so far from this investigation, which I believe still needs fixing.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Alex Gibson [:agibson] from comment #11)
Thanks.

> I filed Bug 1127810 so far from this
> investigation, which I believe still needs fixing.

To be clear, that bug says that it doesn't affect Firefox 36.

Comment 13

[Alex Gibson [:agibson]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> (In reply to Alex Gibson [:agibson] from comment #11)
> Thanks.
> 
> > I filed Bug 1127810 so far from this
> > investigation, which I believe still needs fixing.
> 
> To be clear, that bug says that it doesn't affect Firefox 36.

Bug 1127810 is a regression caused by Bug 1125764, which in itself is a regression caused by Bug 1110602.

Bug 1110602 was uplifted to FF36, so Bug 1125764 probably needs uplifting too. This will likely mean 1127810 will also be needed.

Comment 14

[Matthew N. [:MattN]]

Yeah, I'm planning on doing all the uplift requests today

Comment 15

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Think it's safe to close this. We've identified bug 1127810 as something we need to fix.