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

VERIFIED FIXED in Firefox 48

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: tcsc)

Tracking

unspecified
Firefox 48
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox48 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Hover over the "sigin in to sync" area of the hamburger menu With a clean profile - there's no tooltip - there should be.
Flags: firefox-backlog+
(Reporter)

Updated

3 years ago
Assignee: markh → nobody
(Assignee)

Comment 1

3 years ago
Ryan, I was told you're who I should ask about what this tooltip should say?
Flags: needinfo?(rfeeley)
(Assignee)

Updated

3 years ago
Assignee: nobody → tchiovoloni
Like when signed in, the tooltip should say "Open Sync Preferences", because that's what it does.
Flags: needinfo?(rfeeley)

Updated

3 years ago
Attachment #8737227 - Flags: review?(adw) → review+
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
(Reporter)

Comment 5

3 years ago
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.
Attachment #8737227 - Flags: review+
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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/
(Assignee)

Comment 8

3 years ago
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/
(Reporter)

Comment 9

3 years ago
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.
Attachment #8737227 - Flags: review+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=8446945&repo=fx-team
Flags: needinfo?(tchiovoloni)
(Assignee)

Comment 13

3 years ago
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/
Attachment #8737227 - Flags: review+
(Assignee)

Comment 14

3 years ago
Ah, that should be fixed now.
(Assignee)

Updated

3 years ago
Flags: needinfo?(tchiovoloni)

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b3eed99cb17b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify][testday-20160708]
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify][testday-20160708] → [good first verify][testday-20160708][bugday-20160720]
You need to log in before you can comment on or make changes to this bug.