Opened 4 months ago

Closed 3 months ago

Last modified 4 weeks ago

#22542 closed defect (fixed)

Tor Browser 7.0 Security Settings Window too small on macOS 10.12

Reported by: teor Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-7.0-issues, tbb-regression, tbb-security-slider, TorBrowserTeam201707R, tbb-backported
Cc: arthuredelstein, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When I run Tor Browser 7.0 on macOS 10.12, and open the security settings window, the last line of text is off the page. This is probably because the UI font changed.

Child Tickets

Attachments (2)

SecurityBefore.png (35.4 KB) - added by teor 3 months ago.
SecurityAfter.png (44.5 KB) - added by teor 3 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 4 months ago by gk

Cc: arthuredelstein added
Keywords: tbb-7.0-issues tbb-regression tbb-security-slider added
Status: newneeds_information

So, you don't get a scrollbar and the text is off the page? Or is the issue that you are already getting a scrollbar on the level with the least amount of text?

comment:2 in reply to:  1 ; Changed 4 months ago by teor

Status: needs_informationnew

Replying to gk:

So, you don't get a scrollbar and the text is off the page? Or is the issue that you are already getting a scrollbar on the level with the least amount of text?

What I see:

There is no visual indication that there is any extra text in the window that can be scrolled to.

There is a scrollbar, but on macOS, it is invisible until the user starts scrolling.
If a user just happens to start scrolling, then the final line becomes visible, but the first line is hidden.

The scrolling also makes it hard to tell exactly which lines apply to Low, Medium, and High.

What I expect:

All the text is shown on the screen when the user opens the window.

comment:3 in reply to:  2 Changed 4 months ago by teor

Replying to teor:

Replying to gk:

So, you don't get a scrollbar and the text is off the page? Or is the issue that you are already getting a scrollbar on the level with the least amount of text?

This only happens when I am in High Security Mode.
The scrollbar appears for a few seconds when I first switch to High Security Mode, then disappears. (I didn't see the scrollbar to start with, because I was in High Security Mode before I upgraded.)

What I see:

There is no visual indication that there is any extra text in the window that can be scrolled to.

There is a scrollbar, but on macOS, it is invisible until the user starts scrolling.
If a user just happens to start scrolling, then the final line becomes visible, but the first line is hidden.

The scrolling also makes it hard to tell exactly which lines apply to Low, Medium, and High.

What I expect:

All the text is shown on the screen when the user opens the window.

comment:4 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201706R added
Status: newneeds_review

Here is a simple fix (increase a hard-coded height):
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug22542-01
I assume this won't look too bad on the other platforms.

comment:5 Changed 4 months ago by cypherpunks

UI font changed

Spacing between lines changed.

comment:6 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. Merged to master (commit 6febc86aaca26984131b541e185323070de88ea7).

comment:7 Changed 4 months ago by teor

Resolution: fixed
Status: closedreopened

This works for me using macOS 10.12.5 outside a VM (retina), but doesn't work inside a VM (non-retina).

I am running macOS 10.12.5 with Tor Browser 7.0.1 and Tor Button 1.9.7.4 in both environments.

I think it's better than it used to be, but there are still 2 lines of text missing at the bottom on non-retina. (And on retina, there are a few pixels still hidden at the bottom, which makes it possible to trigger the scroll bar.)

comment:8 Changed 4 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706R removed

comment:9 Changed 4 months ago by gk

Cc: mcs brade added

comment:10 Changed 3 months ago by mcs

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Status: reopenedneeds_review

What we really want to do is adjust the height of the window so that no scrollbar is necessary. Here is a patch:
https://oniongit.eu/brade/torbutton/merge_requests/1

We tested this on a couple of OSX systems, on a Linux system, and on Windows 10. Here is a pre-built xpi for testing:
https://people.torproject.org/~brade/builds/bug22542/torbutton@torproject.org.xpi
teor: if you have time please test this on your OSX systems.

As an aside, Kathy and I don't know what the best approach is for using oniongit; maybe we should create a torbutton project under the tor-browser group and submit merge requests against it?

comment:11 in reply to:  10 Changed 3 months ago by teor

Replying to mcs:

...
We tested this on a couple of OSX systems, on a Linux system, and on Windows 10. Here is a pre-built xpi for testing:
https://people.torproject.org/~brade/builds/bug22542/torbutton@torproject.org.xpi
teor: if you have time please test this on your OSX systems.

I'm sorry, I am not sure I will have time to do this, I have some tight deadlines at the moment.

comment:12 Changed 3 months ago by teor

Status: needs_reviewmerge_ready

Thanks, this works on macOS 10.12.5/6 in both my VM (non-retina) and standard (retina) environments. I'll attach before and after screenshots for the VM case.

Changed 3 months ago by teor

Attachment: SecurityBefore.png added

Changed 3 months ago by teor

Attachment: SecurityAfter.png added

comment:13 in reply to:  10 Changed 3 months ago by gk

Replying to mcs:

What we really want to do is adjust the height of the window so that no scrollbar is necessary. Here is a patch:
https://oniongit.eu/brade/torbutton/merge_requests/1

Okay, I commented.

We tested this on a couple of OSX systems, on a Linux system, and on Windows 10. Here is a pre-built xpi for testing:
https://people.torproject.org/~brade/builds/bug22542/torbutton@torproject.org.xpi
teor: if you have time please test this on your OSX systems.

As an aside, Kathy and I don't know what the best approach is for using oniongit; maybe we should create a torbutton project under the tor-browser group and submit merge requests against it?

Dunno. I think for now just having a user repo there with an up-to-date master branch for merge requests and using the code review features Gitlab provides is the way to go. At least that's what I am currently doing. If there is something else/better I should set up and test I am all ears.

comment:14 Changed 3 months ago by brade

I replied to the comment in oniongit.

comment:15 in reply to:  14 ; Changed 3 months ago by cypherpunks

Status: merge_readyneeds_review

Replying to gk:

Okay, I commented.

Replying to brade:

I replied to the comment in oniongit.

Of course, it's good to know that you commented. But don't you think that openness and clearness of the project suffer? Especially, because oniongit without JS does nothing, except loading CPU. What was the reason to move the discussion to oniongit?

comment:16 in reply to:  15 Changed 3 months ago by gk

Replying to cypherpunks:

Replying to gk:

Okay, I commented.

Replying to brade:

I replied to the comment in oniongit.

Of course, it's good to know that you commented. But don't you think that openness and clearness of the project suffer? Especially, because oniongit without JS does nothing, except loading CPU. What was the reason to move the discussion to oniongit?

The reason is our testing of oniongit as a trac replacement (at least regarding code review). Thanks for raising the JS issue. teor went already ahead and pointed it out to a wider audience (https://lists.torproject.org/pipermail/tor-project/2017-July/001328.html). We'll see where the discussion goes.

comment:17 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. Let's test it in an alpha, though, first. I pushed the code to master (commit 8fbc96cab7ed9816da68d070b7cb01548a19a25a).

comment:18 Changed 7 weeks ago by gk

Keywords: tbb-backport added

comment:19 Changed 4 weeks ago by gk

Keywords: tbb-backported added; tbb-backport removed

Picking this up for 1.9.7.7 (commit 9aa6f29c896be65a4f883dd4bd73f48e07d359a7).

Note: See TracTickets for help on using tickets.