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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: agibson, Assigned: MattN)

Tracking

({regression})

41 Branch
Firefox 42
Points:
3
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Reporter

Comment 1

4 years ago
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.
Reporter

Comment 2

4 years ago
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.
Flags: needinfo?(MattN+bmo)
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.
I can reproduce now that I'm logged in:

"UITour: showHighlight: Not showing a highlight since the target isn't visible"
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Summary: Highlighting 'accountStatus' is broken → Highlighting 'accountStatus' is broken when the user is logged in
(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.
Flags: needinfo?(agibson)
Reporter

Comment 6

4 years ago
(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.
Flags: needinfo?(agibson)
Reporter

Comment 7

4 years ago
(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).
(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.
Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh
Attachment #8637389 - Flags: review?(markh)
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.
Points: --- → 3
Flags: firefox-backlog+
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.
Attachment #8637389 - Flags: review?(markh) → review+
Comment hidden (obsolete)
Attachment #8637389 - Attachment description: MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh → MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r?markh
Comment hidden (obsolete)
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
Attachment #8637389 - Flags: review+ → review?(markh)
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!
Attachment #8637389 - Flags: review?(markh) → review+
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
Attachment #8637389 - Flags: approval-mozilla-aurora?
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
Flags: needinfo?(MattN+bmo)
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?
Flags: needinfo?(MattN+bmo) → needinfo?(markh)
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
Attachment #8637389 - Attachment description: MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r?markh → MozReview Request: Bug 1185579 - UITour: Make the "accountStatus" target use the FxA avatar when logged in. r=markh
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/1ba8b8c60de6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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.
Attachment #8637389 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(MattN+bmo)
Mistakes happen.
Flags: needinfo?(MattN+bmo)
(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
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.