Opened 2 months ago

Closed 8 weeks ago

#29825 closed defect (fixed)

Intelligently insert the Security Level button to the user's taskbar rather than resetting to default on upgrade

Reported by: pospeselr Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201903R, tbb-8.5-must-alpha
Cc: tbb-team Actual Points:
Parent ID: #25658 Points:
Reviewer: Sponsor:

Description

torbutton currently resets toolbar which obliterates the user's UI customizations on upgrade. I should probably fix that.

Child Tickets

Change History (11)

comment:1 Changed 2 months ago by pospeselr

Cc: tbb-team added; tbb-browser removed

comment:2 Changed 2 months ago by pospeselr

Keywords: TorBrowserTeam201903R added; TorBrowserTeam201903 removed
Status: assignedneeds_review

torbutton patch: https://gitweb.torproject.org/user/richard/torbutton.git/commit/?h=bug_29825

This patch adjusts the serialized json struct stored in 'browser.uiCustomization.state' in such a way as to intelligently insert the new securitylevel button and move the torbutton button beside it while keeping other user settings unchanged.

tor-browser patch: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_29825

This patch tweaks the default toolbar layout so that the Downloads button appears to the right of the Security Level button to match the logic in the torbutton patch.

Toolbar layout now:

| Back | Forward | Refresh | URL Bar | Torbutton | Security Level | Downloads | Hamburger

Last edited 2 months ago by pospeselr (previous) (diff)

comment:3 Changed 2 months ago by pospeselr

Status: needs_reviewnew

comment:4 Changed 2 months ago by pospeselr

Status: newassigned

comment:5 Changed 2 months ago by pospeselr

Status: assignedneeds_review

comment:6 Changed 2 months ago by gk

Keywords: tbb-8.5-must added; tbb-8.5 removed

Marking blockers for Tor Browser 8.5.

comment:7 Changed 2 months ago by gk

Keywords: tbb-8.5-must-alpha added; tbb-8.5-must removed

Tickets that block the next 8.5 alpha.

comment:8 Changed 2 months ago by mcs

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201903R removed
Status: needs_reviewneeds_revision

This looks good and it seems to work. Kathy and I just found a few nits.

In the commit message:

  • s/to the right of/after/ (that way the commit message will be correct for RTL languages as well)
  • s/urlbar bar/url bar/

In the patch:

  • Should we preserve the #13378 comment? I think it might still be relevant but I did not look at the patch from that ticket.
  • Maybe rename placeButtonBesideUrlbar() to placeButtonAfterUrlBar() and add a blank line after its definition.
  • In or near the if urlbar isn't present comment, please mention that the UI does not allow users to remove the URL bar (which means we are very unlikely to end up putting our icons at the beginning of the toolbar).
  • In that same comment, I suggest s/front of/beginning of/

comment:9 Changed 8 weeks ago by pospeselr

Keywords: TorBrowserTeam201903R added; TorBrowserTeam201903 removed
Status: needs_revisionneeds_review

comment:10 Changed 8 weeks ago by mcs

r=brade, r=mcs
Thanks for updating the patch. It looks good to us.

comment:11 Changed 8 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! I cherry-picked the browser patch and applied it as a fixup to the patch for #25658 as reordering the taskbar is somewhat orthogonal to intelligently inserting the buttons. The commit is commit 2825e7f974d50a6394b5a5196c2133c068d85286 on tor-browser-60.6.1esr-8.5-1.

The Torbutton patch got applied to Torbutton's master branch (commit d7595079a561fb36220a83e1d60ba9e6bab289e5).

Note: See TracTickets for help on using tickets.