Opened 4 years ago

Closed 4 years ago

#18743 closed defect (fixed)

"Sign in to Sync" icon not hidden in ESR45-based Tor Browser

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff45-esr, TorBrowserTeam201605R
Cc: brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

With the patch for #16488 (rebased to the ESR45 codebase) applied, the "Sign in to Sync" text label is correctly hidden but the icon is still visible in the hamburger menu.

Child Tickets

Change History (9)

comment:1 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201604R added
Status: newneeds_review

I previously tried upstreaming our #16488 patch to Firefox, but Chris Karlof pointed out to me that the approach is rather Tor Browser-specific:
https://bugzilla.mozilla.org/show_bug.cgi?id=1217295

So here's a torbutton patch that accomplishes the same thing:
https://github.com/arthuredelstein/torbutton/commit/18743
Hash 242777b8336128995219470275d50b6b6788acb1

We could then drop the tor-browser.git patch:
https://github.com/arthuredelstein/tor-browser/commit/18743+1
Hash c23e50149bc76dfa0529b51f64947f556903c34f

comment:2 Changed 4 years ago by bugzilla

If you pay primary attention to hamburger menu, then don't forget about Synced Tabs that can be added through Customize in it. The rest is in #16778.

comment:3 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201604R removed

Moving reviews over to May 2016

comment:4 Changed 4 years ago by gk

Looks good to me. One nit: could you wrap that long line in torbutton_update_sync_ui()? It would make it a bit easier to read in my terminal.

comment:5 in reply to:  4 Changed 4 years ago by mcs

Replying to gk:

Looks good to me. One nit: could you wrap that long line in torbutton_update_sync_ui()? It would make it a bit easier to read in my terminal.

Maybe also add a try/catch in that function or check that the getElementById() succeeds. That way a failure there due to future Firefox changes won't break other Torbutton features such as the circuit display.

comment:6 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201605R removed
Status: needs_reviewneeds_revision

comment:7 Changed 4 years ago by arthuredelstein

Thanks for the good suggestions. Here's a new version with both things fixed.

https://github.com/arthuredelstein/torbutton/commit/18743+1
Hash 5b17ddc4efa21716cf666594930469a99e398d44

comment:8 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed
Status: needs_revisionneeds_review

comment:9 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. This is commit d127873b498fb83e4f608b82a8d382df25183dd3 on master.

Note: See TracTickets for help on using tickets.