1185579 - Highlighting 'accountStatus' is broken when the user is logged in

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Alex Gibson [:agibson]]

41.0a2 (2015-07-20)

"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0"

STR:

1.) Visit https://www.mozilla.org/en-US/firefox/41.0a2/tour/
2.) Close the two UITour popups that appear on page load.
3.) Click Tools -> Web Developer -> Web Console
4.) Paste the following JS in the console:

Mozilla.UITour.showHighlight('accountStatus');

Expected results:

The menu should open and highlight Sync 'accountStatus'.

Actual results:

Nothing happens.

This is actually breaking the Dev Edition /firstrun flow, as the last step in the tour tries to highlight this target.

Comment 1

[Alex Gibson [:agibson]]

Also worth noting, when querying 'getconfiguration' using 'availableTargets', 'accountStatus' is still in the list of targets. It just doesn't seem to be working.

My guess is a regression caused by the profile image changes which are fairly recent.

Comment 2

[Alex Gibson [:agibson]]

Matt, any chance that someone (I know you're busy) has some time to get some eyeballs on this one? It's currently breaking things in production in a few places.

Comment 3

[Matthew N. [:MattN]]

This works fine for me in Nightly and Aurora when I'm not logged in. I'm guessing this is only an issue when you are logged in which would likely make it lower priority as we're mostly likely only highlighting this after checking if sync is setup via getConfiguration.

Comment 4

[Matthew N. [:MattN]]

I can reproduce now that I'm logged in:

"UITour: showHighlight: Not showing a highlight since the target isn't visible"

Comment 5

[Matthew N. [:MattN]]

(In reply to Alex Gibson [:agibson] from comment #1)
> Also worth noting, when querying 'getconfiguration' using
> 'availableTargets', 'accountStatus' is still in the list of targets. It just
> doesn't seem to be working.

I can't reproduce this. Can you double-check this?

> My guess is a regression caused by the profile image changes which are
> fairly recent.

That is correct. For some reason a new <image> element was added for the avatar instead of using the existing one.

Comment 6

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] from comment #5)
> (In reply to Alex Gibson [:agibson] from comment #1)
> > Also worth noting, when querying 'getconfiguration' using
> > 'availableTargets', 'accountStatus' is still in the list of targets. It just
> > doesn't seem to be working.
> 
> I can't reproduce this. Can you double-check this?

Ah, you are correct - I tried again and `accountStatus` is indeed not in the list of targets, my bad.

Comment 7

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] from comment #3)
> This works fine for me in Nightly and Aurora when I'm not logged in. I'm
> guessing this is only an issue when you are logged in which would likely
> make it lower priority as we're mostly likely only highlighting this after
> checking if sync is setup via getConfiguration.

Fwiw, generally we only query getConfiguration('sync') if we need to know the login status. Up until now, we have not had a need to do this before highlighting `accountStatus` since it was always there (irrespective of being logged in or not).

Comment 8

[Jonathan Petto [:jpetto]]

(In reply to Alex Gibson [:agibson] from comment #1)
> Also worth noting, when querying 'getconfiguration' using
> 'availableTargets', 'accountStatus' is still in the list of targets. It just
> doesn't seem to be working.

I'm also unable to reproduce this. Running through the available targets in 41.0a2 (2015-07-22), I see the following:

addons
appMenu
backForward
bookmarks
customize
devtools
help
home
forget
loop
pocket
privateWindow
quit
readerMode-urlBar
search
searchIcon
urlbar
webide
searchEngine-google
searchEngine-yahoo
searchEngine-bing
searchEngine-amazondotcom
searchEngine-eBay
searchEngine-twitter
searchEngine-wikipedia
searchEngine-ddg

(In reply to Matthew N. [:MattN] from comment #3)
> This works fine for me in Nightly and Aurora when I'm not logged in.

Same here. Running Mozilla.UITour.showHighlight('accountStatus'); when *not* logged in does highlight Sync in the hamburger menu.

Comment 9

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Comment 10

[Matthew N. [:MattN]]

Mark, if you can tell me a simple way to mock the logged in state in the panel I can add a test for this.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

https://reviewboard.mozilla.org/r/13859/#review12459

Looks great, thanks. Re testing, there's no mock story for browser tests yet (which should change, but it is what it is for now). Probably best to check out browser_aboutAccounts - it sets a real FxA user and should trigger the correct notifications so the menu changes.

This is messier than it should be, so if you get part way through this and give up in disgust, please feel free to hand it back to me.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r?markh

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Now with tests

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

https://reviewboard.mozilla.org/r/13859/#review13527

Awesome, thanks!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7f106466e9ad

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Approval Request Comment
[Feature/regressing bug #]: FxA avatar support
[User impact if declined]: Devedition tour won't work properly if the user is logged in
[Describe test coverage new/current, TreeHerder]: m-bc
[Risks and why]: Low risk change isolated to the one UITour target for sync/fxa status
[String/UUID change made/needed]: None

Comment 18

[Carsten Book [:Tomcat]]

seems this caused memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=4081274&repo=fx-team and so i had to back this out

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/444b5b18cef4

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1376c328fbb3

Comment 21

[Matthew N. [:MattN]]

I just relanded this with it disabled for debug/asan (leak check) builds. updateAppMenuItem is the cause of the leak but I don't have time to learn that code to understand the leak. Mark, do you want to look at this leak in a follow-up at some point?

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/53560cc91161 for test failures in browser_fxa.js:

https://treeherder.mozilla.org/logviewer.html#?job_id=4087992&repo=fx-team

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1ba8b8c60de6

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ba8b8c60de6

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8637389 [details]
MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh

Let's uplift to Aurora. Seems safe given that new tests were added.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Matt landed this on Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d65d42aa35af

Matt - pulsebot doesn't mark uplifts. Please don't forget to update the bug in the future.

Comment 28

[Matthew N. [:MattN]]

Mistakes happen.

Comment 29

[Mark Hammond [:markh] [:mhammond]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #21)
> I just relanded this with it disabled for debug/asan (leak check) builds.
> updateAppMenuItem is the cause of the leak but I don't have time to learn
> that code to understand the leak. Mark, do you want to look at this leak in
> a follow-up at some point?

better late than never :) I opened bug 1332985