Opened 4 weeks ago

Closed 3 weeks ago

#31251 closed defect (fixed)

Security Level menu hangs off of the toolbarbutton element when it should hang off of the toolbarbutton's child element with toolbarbutton-icon class

Reported by: pospeselr Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201908R
Cc: tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When you click on the Security Level icon the resulting menu is lower than it should be as compared to other buttons (Downloads, Library, Hamburger Menu, etc). This happens because the dropdown element is parented to the wrong element in the DOM.

Child Tickets

Change History (9)

comment:1 Changed 4 weeks ago by pospeselr

Summary: Security Level menu hangs off of the toolbarbutton element when it should hang off of the toolbarbutton's 'anchor'Security Level menu hangs off of the toolbarbutton element when it should hang off of the toolbarbutton's child element with toolbarbutton-icon class

comment:2 Changed 4 weeks ago by pospeselr

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Status: assignedneeds_review

Fixed this and a few other UI polish issues relating to the Security Level toolbar button, details in commit log.

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

comment:3 Changed 4 weeks ago by acat

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201907R removed
Status: needs_reviewneeds_revision

I tested it, and works mostly fine, except when you try to move the button via Customize.... Then it stops working for the session, but it works for new windows and also when restarting. Is it because the patch is now caching the button instead of getting it every time?

Some minor issues:

		  classList.add("safest");         

Could you remove the trailing spaces?
Also, there are some empty lines with leading spaces, could you remove them too?

The get button() implementation in browser-places.js looks slightly simpler and also works for me, but up to you to keep this one, looks also good (unless this is what is breaking when moving the button around).

openAdvancedSecuritySettings : async function() {

This is not new code, but why async? This should be equivalent to the same function without async and returning Promise.resolve(). Is it because it's required to return a Promise? Otherwise I think we can remove async here.

comment:4 Changed 3 weeks ago by pospeselr

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Status: needs_revisionneeds_review

Ok I pushed a fixup commit. I'm now deleting the _anchor and _button members from the SecurityLevelButton object when the UI DOM changes.

I don't remember why that function was marked 'async' and I've verified that it now works without it. IIRC there was a bug associated (yep #29554) with getting the about:preferences to scroll down to our Security Level settings so perhaps async was an attempt of a work-around for that. Removed.

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

comment:5 Changed 3 weeks ago by acat

Looks good and works for me now.

comment:6 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201907R removed
Status: needs_reviewneeds_revision

posepeslr: Could you squash the commits and and shorten the summary of your commit message? It's useful to be able to read that in a terminal on one line (for more about that, see: https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html).

comment:7 Changed 3 weeks ago by pospeselr

Status: needs_revisionneeds_review

comment:8 Changed 3 weeks ago by pospeselr

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed

comment:9 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! Looks good and merged to tor-browser-60.8.0esr-9.0-1 (commit 0bed1d181d0839f058e311c01dbdb263a96270fd).

Note: See TracTickets for help on using tickets.