1246606 - "Sign in to Sync" in hamburger menu has no tooltip

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

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

Hover over the "sigin in to sync" area of the hamburger menu With a clean profile - there's no tooltip - there should be.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Ryan, I was told you're who I should ask about what this tooltip should say?

Comment 2

[Ryan Feeley [:rfeeley]]

Like when signed in, the tooltip should say "Open Sync Preferences", because that's what it does.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Review commit: https://reviewboard.mozilla.org/r/43811/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43811/

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

https://reviewboard.mozilla.org/r/43811/#review40551

Comment 5

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

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Thanks Thom and Drew - but I think this patch isn't going to work correctly when the user disconnects from Sync. Lines such as https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fxaccounts.js#296 will override that fixed value and not reset it back.

It looks like we intentionally have no tooltip in customization mode, so I think we will need to split the condition |if (!this._inCustomizationMode && userData) {| up, and when we have no |userData| explicitly set the tooltip text.

Could you please verify I'm correct, and if so, put a new patch up? If I'm not (it really *was* a quick look :), accept my apologies and reset the r+ flag back.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Ah, thanks. I wasn't certain about the logic here. I did try to test in all the cases it seemed to be handling, and I'm confident the tooltip is set even after logout (as well as incorrect login and cancel, and a couple other cases), but looking at it again, I think you're right, and it would need to be explicitly reset after those conditions are met.

Anyway, I'll update the patch to explicitly reset it if it doesn't have userData first thing on Monday.

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43811/diff/1-2/

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43811/diff/2-3/

Comment 9

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

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Thanks Thom,
  Reviewboard seems kinda dumb here and it's not re-requesting review from adw - but this version is how I suggested Thom tackle it, so I'm stealing the review back.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/033ae34a1c53

Comment 11

[Carsten Book [:Tomcat]]

backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=8446945&repo=fx-team

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/071ebc4f3ce7

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8737227 [details]
MozReview Request: Bug 1246606 - Ensure a tooltip is present on the 'Sign in to sync' button in the customizable menu r?adw

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43811/diff/3-4/

Comment 14

[Thom Chiovoloni [:tcsc] (ex-moco)]

Ah, that should be fixed now.

Comment 15

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

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

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3eed99cb17b

Comment 18

[Hossain Al Ikram [:ikram] (QA Contact)]

I have reproduced the bug with Nightly 47.0a1 (2016-02-08) on Windows 7, 64 Bit!

The Bug's fix is verified on Latest Beta 48.0b6.

Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

Comment 19

[Rezaul Huque Nayeem]

Reproduced the bug in firefox nightly 47.0a1 (2016-02-08) with ubuntu 16.04 (64 bit)

Verified as fixed with latest firefox beta 48.0b8 (Build ID: 20160714050942)
Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0