Opened 11 months ago

Closed 6 months ago

Last modified 5 months ago

#28329 closed enhancement (fixed)

Design TBA+Orbot configuration UI/UX

Reported by: sysrqb Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, ux-team, TBA-a3, tbb-8.5-must-alpha, TorBrowserTeam201904
Cc: antonela, igt0, tbb-team, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor8

Description

I now have TBA and Orbot glued together. They co-exist, but I'm not sure how they should look or interact together. Where in the TBA app/screen should we have a button that switches to the Orbot screen? Do we want Orbot's onboarding UX or do we want our own?

Please help :)

Child Tickets

TicketTypeStatusOwnerSummary
#29858defectclosedtbb-teamTor Browser for Android 8.5a9 does not show onboarding anymore on first start
#29906defectclosedsysrqbTor for Android stops working immediately on app start

Attachments (22)

28329.png (241.4 KB) - added by antonela 11 months ago.
userflow.png (203.6 KB) - added by antonela 11 months ago.
mockups-1.1.png (369.8 KB) - added by antonela 11 months ago.
userflow-1.1.png (214.2 KB) - added by antonela 11 months ago.
mockups-1.1.2.png (371.1 KB) - added by antonela 11 months ago.
userflow-2.png (216.5 KB) - added by antonela 10 months ago.
mockups-2.png (225.0 KB) - added by antonela 10 months ago.
network-settings.png (818.4 KB) - added by antonela 9 months ago.
mockups-3.png (813.2 KB) - added by antonela 9 months ago.
mockups-4.png (749.0 KB) - added by antonela 9 months ago.
network-settings-2.png (420.2 KB) - added by antonela 9 months ago.
01-tba-review-1.0.png (147.7 KB) - added by dunqan 8 months ago.
01-tba-review-2.0.png (146.0 KB) - added by dunqan 8 months ago.
01-tba-review-3.0.png (149.3 KB) - added by dunqan 8 months ago.
01-tba-review-4.0.png (184.1 KB) - added by dunqan 8 months ago.
measurements.png (68.1 KB) - added by antonela 8 months ago.
TBA Settings Network - Bridge - known.png (76.1 KB) - added by antonela 7 months ago.
icon1x1.svg (1.1 KB) - added by antonela 7 months ago.
icon.svg (1.3 KB) - added by antonela 7 months ago.
Screenshot from 2019-04-09 15-37-14.png (211.1 KB) - added by sysrqb 6 months ago.
about_tor_no_space.png (55.7 KB) - added by gk 6 months ago.
about_tor_space.png (97.1 KB) - added by gk 6 months ago.

Change History (121)

comment:1 Changed 11 months ago by antonela

Keywords: ux-team added

comment:2 Changed 11 months ago by sysrqb

Parent ID: #28051

Adding this as a dependency of #28051, too (we can delete it later, if needed).

comment:3 Changed 11 months ago by gk

Keywords: TBA-a2 added

Moving on TBA-a2 radar

comment:4 Changed 11 months ago by gk

Priority: MediumVery High

Changed 11 months ago by antonela

Attachment: 28329.png added

comment:5 Changed 11 months ago by antonela

hello! I made a first approach to this task:

I know that the current .apk is just a glued version, but is cool to have it as a reference to think about how to improve it.

The current First Time User flow looks like

  1. intro
  2. value proposition #1
  3. bridge suggestion - [Learn More]
  4. vpn mode
  5. start tor
  • browser opens
  1. welcome
  2. privacy
  3. tor network
  4. tips
  5. onions

I drafted a first version of an improved flow. On the big picture, it includes a bridge configuration step and removed the VPN settings. Also, I merged the privacy and tor network steps from the current onboarding into this bigger flow.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/28329.png

I have questions for the Tor Network Settings in TBA. I see four options for config bridges at orbot now:

  • Connect directly to Tor (recommended)
  • Connect through community servers
  • Connect throught cloud servers
  • Request New Bridges.... (If all else fails)

Modo Bridge

Website
Cancel
Email

What is the plan for them? Are we going to carry all of them? Will MOAT work in TBA? Do we want to have a similar Tor Launcher UX?

Don't worry about the UI for now, let's first define how we want the user flow to be, which are requirements, and then we can go deep with screens.

comment:6 in reply to:  5 Changed 11 months ago by sysrqb

Replying to antonela:

hello! I made a first approach to this task:

Thanks!

I know that the current .apk is just a glued version, but is cool to have it as a reference to think about how to improve it.

I have a new version available now :)

http://sbe5fi5cka5l3fqe.onion/~sysrqb/fennec-60.3.0.en-US.android-arm_28051_1.apk
https://people.torproject.org/~sysrqb/fennec-60.3.0.en-US.android-arm_28051_1.apk

Orbot now works (or it should work, let me know if it doesn't). I removed the onboarding screens due to some coding issues, but we'll have new onboarding screens in any case. After Tor starts, the user must tap the back button to return to the browser. If the user's device is not running the newest version of Android, then they should have a notification that says Tor Browser in the drop-down list, and they can return to Orbot using that. We have a problem on the newest version of Android, so the notification isn't created and there isn't a way for the user to re-open Orbot except for killing the app and restarting it.

The current First Time User flow looks like

[snip]

I drafted a first version of an improved flow. On the big picture, it includes a bridge configuration step and removed the VPN settings. Also, I merged the privacy and tor network steps from the current onboarding into this bigger flow.

Yes, that looks good! I don't think we should say anything about the VPN mode (and we'll probably remove that in the future).

[snip]

I have questions for the Tor Network Settings in TBA. I see four options for config bridges at orbot now:

  • Connect directly to Tor (recommended)
  • Connect through community servers
  • Connect throught cloud servers
  • Request New Bridges.... (If all else fails)

Bridge Mode

Website
Cancel
Email

What is the plan for them? Are we going to carry all of them? Will MOAT work in TBA?

Yes, I don't know a reason why we wouldn't want Moat available in TBA. In fact, I think Moat provides an even better experience on mobile than on desktop. But, considering Orbot doesn't currently have support for moat, I think we should add it as a follow-up ticket for this, and we can implement it soon (but later).

Do we want to have a similar Tor Launcher UX?

I think we should use the better UX :) (whatever that is)

I think using the Tor Launcher UX is the simplest answer and it provides a consistent experience across desktop and mobile, so that is a good option. On the other hand, (saying the obvious) mobile provides a different UX than desktop, so creating a new experience that is mobile-specific may be better. I'd be happy with choosing the easy/simple option now, and we can iterate on it over the next few months.

comment:7 Changed 11 months ago by n8fr8

Are you just hard forking Orbot code and merging it directly into the Tor Browser repo? I thought you were going to a bit more modular approach?

This library: https://github.com/guardianproject/Tor_Onion_Proxy_Library is probably a better starting point, and something we could both collaborate on improving together. I see how using the Orbot code gives you a lot of config UI, PT's, etc quickly, but it isn't a library.

As for MOAT it is something I'm already looking into tackling with Orbot, and again, seems like it would be best implemented as a library so that other apps can utilize it.

Changed 11 months ago by antonela

Attachment: userflow.png added

comment:8 in reply to:  7 Changed 11 months ago by sysrqb

Replying to n8fr8:

Are you just hard forking Orbot code and merging it directly into the Tor Browser repo? I thought you were going to a bit more modular approach?

No, we're handling this gentler than that - see #28051. In the short term, we'll maintain a patch that we'll apply on top of the official Orbot repo. We don't want to hard-fork Orbot, but using Orbot is the fastest and "easiest" solution given the timeline we set.

This library: https://github.com/guardianproject/Tor_Onion_Proxy_Library is probably a better starting point, and something we could both collaborate on improving together. I see how using the Orbot code gives you a lot of config UI, PT's, etc quickly, but it isn't a library.

We'll most likely use Tor Onion Proxy Library in the future, but that depends on #27609 - and we didn't have time for that within the last two months. I believe we all agree that Orbot (as it is today), is not the long-term answer to our problem of bundling Tor with Tor Browser for Android.

As for MOAT it is something I'm already looking into tackling with Orbot, and again, seems like it would be best implemented as a library so that other apps can utilize it.

Yes, that'd be great. I was thinking we can coordinate this and collaborate, and make sure we're not duplicating effort in this area - but the last month has been focused on building a working MVP where the result isn't worse than TBA and Orbot (separately). After this release, we'll start looking at #27609 and work toward using that instead of Orbot.

Changed 11 months ago by antonela

Attachment: mockups-1.1.png added

comment:9 Changed 11 months ago by antonela

By aiming for the most minimal orbot integration for this release, I:

  • updated the userflow. We are not interacting with Bridges/PT yet.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/userflow-1.1.png

  • made an approach for the Connecting... screen
  • suggested a layout for Tor Network Settings

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/mockups-1.1.2.png

Last edited 11 months ago by antonela (previous) (diff)

Changed 11 months ago by antonela

Attachment: userflow-1.1.png added

Changed 11 months ago by antonela

Attachment: mockups-1.1.2.png added

comment:10 Changed 11 months ago by gk

Parent ID: #28051

Unparenting so we can close #28051.

comment:11 Changed 10 months ago by gk

Keywords: TorBrowserTeam201812 added

comment:12 Changed 10 months ago by gk

Keywords: TBA-a3 added; TBA-a2 removed

comment:13 Changed 10 months ago by gk

Sponsor: Sponsor8

Adding Sponsor8 tag.

Changed 10 months ago by antonela

Attachment: userflow-2.png added

Changed 10 months ago by antonela

Attachment: mockups-2.png added

comment:14 Changed 10 months ago by antonela

Hello team, matt and I discussed yesterday what could be our users' best scenario when starting TBA.

We think we can aim to bootstrap tor during the initial screen and then open about:tor.

As you know, I'm working with a designer to have our new icon animated, and we can use it as a loader while tor is bootstrapping. A progress bar is a good option too, but the right speed on the icon animation will make our users feel that it is loading faster.

I updated the user flow, and I approached some mockups for this flow.

What do you think?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/userflow-2.png

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/mockups-2.png

comment:15 in reply to:  14 Changed 10 months ago by gk

Replying to antonela:

Hello team, matt and I discussed yesterday what could be our users' best scenario when starting TBA.

We think we can aim to bootstrap tor during the initial screen and then open about:tor.

As you know, I'm working with a designer to have our new icon animated, and we can use it as a loader while tor is bootstrapping. A progress bar is a good option too, but the right speed on the icon animation will make our users feel that it is loading faster.

I updated the user flow, and I approached some mockups for this flow.

What do you think?

I like the animated icon. However, I think we should not only rely on it indicating that Tor Browser is starting up but rather having some kind of progress visible seems useful to me. (especially given the bootstrap could take quite some time).

So, the overall idea is to just connect directly and only try other ways once the direct connection fails? What about scenarios where users need to hide the fact that they are using Tor in the first place by using things like the built-in meek PT right from start?

Changed 9 months ago by antonela

Attachment: network-settings.png added

comment:16 Changed 9 months ago by antonela

Hi folks,

I have been working on the Connecting flow and also on Network Settings.

As we signed, this version will follow this user flow
https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/userflow-1.1.png

Essentially, we will still have a Connect button.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/mockups-3.png

1.0
It is the first screen when TBA start. The Connect button will start the bootstrapping. Users need to trigger it manually. The settings icon at the top right will go to Network Settings.

2.0
Once the user taps to connect, the icon will get animated and we will show the bootstrapping messages. We want advanced users to be able to see/copy tor logs. We will expose it by swiping to the left. If Tor fails, we will move users to the Settings Network screen.

For the unsuccessful scenario, we can take different paths:

a - Have a retry button
b - Suggest users to move to the Network Setting screen to config a Bridge
c - Move the user to the Network Setting screen with a message about what failed and some encouragement on how to fix it.

Do we have data about why bootstrapping fail in mobiles? Which are the most common failing cases?

3.0
If users arrive here after a failed connection, the copy at the top will change to encourage users to take action to fix it. I made the first approach on this screen. My idea is to divide this section into two: Bridges and Advanced Settings. With this split, users are lead to tap the first option instead of start exploring options that they don't know what are.


Network Settings

Since we want to have a balanced experience compared with desktop, I include the same options we have on the desktop. We'll not have moat for this version, so I skipped it.

The user flow is the same for each option. Users can switch on/off each setting, and then we move them to a second screen where the configuration is made.

When the user returns to the main network settings screen, they can see a status of the changes they made, at the description. If users want to change it, they can tap the entire row.

The copy is up to review. Material Design has excellent suggestions about how to approach this copy https://material.io/design/platform-guidance/android-settings.html

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/network-settings.png

Last edited 9 months ago by antonela (previous) (diff)

Changed 9 months ago by antonela

Attachment: mockups-3.png added

comment:17 Changed 9 months ago by pospeselr

The user-flow and ui mockups look great!

The only thing that stands out to me is the language used here in the proxy settings. You would probably only need these settings when connecting via wifi rather than when on using 4G or whatever mobile data. We're sort of implicitly saying that by way of mentioning work or university networks but maybe mobile-specific language should be used here?

comment:18 in reply to:  16 ; Changed 9 months ago by gk

Replying to antonela:

[snip]

For the unsuccessful scenario, we can take different paths:

a - Have a retry button
b - Suggest users to move to the Network Setting screen to config a Bridge
c - Move the user to the Network Setting screen with a message about what failed and some encouragement on how to fix it.

c) seems to me the best option from those three. a) I doubt users understand why we just give them a "Retry" button in case things failed. And why should things get fixed just by tapping that button after the first try already failed? b) we could do that but we would move the user to the network settings anyway if they decided to tap the link, so we can do c) right from the beginning...

Do we have data about why bootstrapping fail in mobiles? Which are the most common failing cases?

As far as I know we don't have that data.

3.0
If users arrive here after a failed connection, the copy at the top will change to encourage users to take action to fix it. I made the first approach on this screen. My idea is to divide this section into two: Bridges and Advanced Settings. With this split, users are lead to tap the first option instead of start exploring options that they don't know what are.


Network Settings

Since we want to have a balanced experience compared with desktop, I include the same options we have on the desktop. We'll not have moat for this version, so I skipped it.

The user flow is the same for each option. Users can switch on/off each setting, and then we move them to a second screen where the configuration is made.

When the user returns to the main network settings screen, they can see a status of the changes they made, at the description. If users want to change it, they can tap the entire row.

The copy is up to review. Material Design has excellent suggestions about how to approach this copy https://material.io/design/platform-guidance/android-settings.html

There is no firewall ports configuration for Tor Launcher on start-up. While this is confusing to some users I think this is currently the right choice for mobile users, too. (I think we'll eventually drop that option altogether on desktop). See: #24452 for some discussion.

comment:19 Changed 9 months ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201812 removed

Moving tickets to Jan 2019.

comment:20 in reply to:  18 ; Changed 9 months ago by sysrqb

Replying to gk:

Replying to antonela:

[snip]

For the unsuccessful scenario, we can take different paths:

a - Have a retry button
b - Suggest users to move to the Network Setting screen to config a Bridge
c - Move the user to the Network Setting screen with a message about what failed and some encouragement on how to fix it.

c) seems to me the best option from those three. a) I doubt users understand why we just give them a "Retry" button in case things failed. And why should things get fixed just by tapping that button after the first try already failed? b) we could do that but we would move the user to the network settings anyway if they decided to tap the link, so we can do c) right from the beginning...

Agreed. The three failure cases I foresee are:

  1. The device doesn't have internet connectivity
  2. The network is censoring Tor connections (and the user needs a bridge)
  3. The network is throttling the user's connection (and the user would be happier using a PT)

We can provide different messages in these cases, but I think we can use the same message for 2) and 3). I suspect we can be a little smart here, too. If the user pressed the "connect" button without configuring a bridge/PT, then maybe we should automatically try one of the built-in bridges if bootstrapping fails. We can choose one or two at random from the list (probably obfs4). This isn't any worse than letting tor bootstrap directly (which is already did, and failed).

There is no firewall ports configuration for Tor Launcher on start-up. While this is confusing to some users I think this is currently the right choice for mobile users, too. (I think we'll eventually drop that option altogether on desktop). See: #24452 for some discussion.

I read through that ticket and some of the other mail. I think the argument "tor will eventually choose a working Guard node" is not very satisfying but I agree it is a much better solution than providing an option that confuses the users. I think it's worth not including the firewall options now.

comment:21 in reply to:  20 Changed 9 months ago by sysrqb

Replying to sysrqb:

Replying to gk:

There is no firewall ports configuration for Tor Launcher on start-up. While this is confusing to some users I think this is currently the right choice for mobile users, too. (I think we'll eventually drop that option altogether on desktop). See: #24452 for some discussion.

I read through that ticket and some of the other mail. I think the argument "tor will eventually choose a working Guard node" is not very satisfying but I agree it is a much better solution than providing an option that confuses the users. I think it's worth not including the firewall options now.

On second thought, that isn't completely true. If the user is connected on a network that requires configuring a proxy server (not simply filtering all non-80/443 destination ports), then providing a configuration option seems important. But, maybe we should think more about how we expose this and present this option. Maybe there's a better flow such that a user is less likely to be confused by this option when they need a bridge. I'm happy with deferring this improvement to #24452.

Changed 9 months ago by antonela

Attachment: mockups-4.png added

Changed 9 months ago by antonela

Attachment: network-settings-2.png added

comment:22 Changed 9 months ago by antonela

Great, so

  • we will move users to the Network Settings screen if something wrong happens.
  • I have a +1 about removing firewall options too.

If the user pressed the "connect" button without configuring a bridge/PT, then maybe we should automatically try one of the built-in bridges if bootstrapping fails. We can choose one or two at random from the list (probably obfs4). This isn't any worse than letting tor bootstrap directly (which is already did, and failed).

I like this idea! Though, I could approach the flow differently: If the connection fails, we move the user to the Network Settings screen. Here, the user will be allowed to enable "Internet is censored here" option. Note that I'm not mentioning Tor but Internet. We can discuss it once we'll review the copy.

Once the user switches ON that option, TBA can "choose one or two at random from the list (probably obfs4)". Then the user will back to the main screen, and the bootstrapping will continue/happen.

If users want to have a different bridge configuration, then they can press Change and so do it.

What do you think?

Updated Userflow
https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/mockups-4.png

Updated Network Settings
https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/network-settings-2.png

Early prototype (allows tap!)
https://marvelapp.com/33d23hc/screen/52368796

comment:23 Changed 9 months ago by sysrqb

Two questions.

  1. Do you have a preference for the gear icon we use? There is one available in Tor Browser already (but it isn't used on mobile, as far as I know). It's slightly different from the gear in the mock-up. This is the icon next to "Preferences" in the menu bar, from the hamburger button.
  2. From the user-studies, is presenting a "connect" button confusing? I know we discussed this and we want something the user may click/press so tor browser doesn't automatically begin bootstrapping, but should we experiment with using another word/phrase instead of "connect"? Or, should we simply use "connect" (because it is good enough)? :)

comment:24 in reply to:  23 ; Changed 9 months ago by sysrqb

Replying to sysrqb:

Two questions.

  1. Do you have a preference for the gear icon we use? There is one available in Tor Browser already (but it isn't used on mobile, as far as I know). It's slightly different from the gear in the mock-up. This is the icon next to "Preferences" in the menu bar, from the hamburger button.

Ignore this one. I found it on MD :)

comment:25 in reply to:  23 Changed 9 months ago by antonela

Yep, Connect is confusing if users need to choose between Configure and Connect, like our Tor Launcher for desktop scenario. In fact, there is not any hierarchy at the UI that allows users to understand which one will do the main action. This is not the situation here, though.

For sure, the ideal scenario is not having an opt-in from users to start a bootstrapping. We already have this consent when they launch the app.

That said, I'm happy to test other options. I'm thinking about "Start" since a while, but it doesn't change the overall user experience. What are your ideas? :)

Replying to sysrqb:

Two questions.

  1. Do you have a preference for the gear icon we use? There is one available in Tor Browser already (but it isn't used on mobile, as far as I know). It's slightly different from the gear in the mock-up. This is the icon next to "Preferences" in the menu bar, from the hamburger button.
  2. From the user-studies, is presenting a "connect" button confusing? I know we discussed this and we want something the user may click/press so tor browser doesn't automatically begin bootstrapping, but should we experiment with using another word/phrase instead of "connect"? Or, should we simply use "connect" (because it is good enough)? :)

comment:26 in reply to:  24 Changed 9 months ago by antonela

Yes, everything is MD! I'm not using custom icons here.

Replying to sysrqb:

Replying to sysrqb:

Two questions.

  1. Do you have a preference for the gear icon we use? There is one available in Tor Browser already (but it isn't used on mobile, as far as I know). It's slightly different from the gear in the mock-up. This is the icon next to "Preferences" in the menu bar, from the hamburger button.

Ignore this one. I found it on MD :)

Changed 8 months ago by dunqan

Attachment: 01-tba-review-1.0.png added

Changed 8 months ago by dunqan

Attachment: 01-tba-review-2.0.png added

Changed 8 months ago by dunqan

Attachment: 01-tba-review-3.0.png added

Changed 8 months ago by dunqan

Attachment: 01-tba-review-4.0.png added

comment:27 Changed 8 months ago by dunqan

Hey everyone,

Antonela asked me to review the designs above. I'm not as knowledgeable about TBA & Orbot as most of the people in this thread though, so please take these recommendations with a pinch of salt!

Firstly I agree with everyone who's suggested that the connection (and bridging) should happen automatically without requiring any input from the user. However the following review assumes there's a good reason for requiring manual prompts, since it seems to be the route we've gone down!

1.0 (Start screen)

  • Is there enough affordance in the settings cog as an icon alone, or should we be presenting users with two clear options instead?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/01-tba-review-1.0.png

2.0 (Bootstrapping)

  • Should the settings icon be accessible from here at all, since it's a transitionary screen with a process already underway?
  • Increasing the contrast of "Swipe to left to see Tor log" will greatly help our visually impaired users (also note the wee typo in there too).
  • Do we need a link to cancel this process in case it hangs? Or provide a back button to screen 1.0?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/01-tba-review-2.0.png

3.0 (Network)

  • Are users knowledgeable enough to understand that censorship in their location is responsible for Tor's failure to connect?
  • It may not be obvious enough to the user that the browser has successfully connected after hitting the switch, as the success state is quite subtle.
  • Users may not understand that they need to hit the back button to continue (which seems a little counter-intuitive), and could erroneously hit "Change" instead.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/01-tba-review-3.0.png

4.0 (Combined Network/Bridge proposal)

It's my understanding that there are basically three mutually exclusive options on the table for when Tor's blocked:

  1. Automatic selection
  2. Select from list
  3. Enter manually

So with that in mind, I've wireframed a proposal that combines both the "Network" and "Select a bridge" screens into a single menu to:

  • Help address any ambiguity about censorship in the UI
  • Reduce confusion about how to proceed once an option's been selected (as selecting an option will automatically advance to the next screen to attempt connection).
  • Better surface manual selection and entry for more advanced users.

In this scenario, hitting the automatic option would cycle through the built-in list until a bridge can be successfully connected to.

However nested radio-buttons may not be the best way to do this – thoughts?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/01-tba-review-4.0.png

Edits: Sorry, took me a few attempts to figure out how to embed images properly!

Last edited 8 months ago by dunqan (previous) (diff)

comment:28 Changed 8 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:29 in reply to:  27 Changed 8 months ago by sysrqb

Replying to dunqan:

Hey everyone,

Antonela asked me to review the designs above. I'm not as knowledgeable about TBA & Orbot as most of the people in this thread though, so please take these recommendations with a pinch of salt!

Thanks for the feedback!

Firstly I agree with everyone who's suggested that the connection (and bridging) should happen automatically without requiring any input from the user. However the following review assumes there's a good reason for requiring manual prompts, since it seems to be the route we've gone down!

This is a long-standing/outstanding question we have. In the general case, we can likely start connecting automatically. But there are some situations where that may put the person in harm's way, and we don't know in which situation we're operating, so we default to being safer.

1.0 (Start screen)

  • Is there enough affordance in the settings cog as an icon alone, or should we be presenting users with two clear options instead?

This is an interesting question. In my testing on a physical device, I'm finding it difficult to press the settings cog because it is a little small. This wasn't apparent when I was testing with an emulator. I think I'll make the settings cog larger or maybe we should consider using a different button. I like the existing design, I think it's more aesthetically pleasing than showing multiple large buttons.

2.0 (Bootstrapping)

  • Should the settings icon be accessible from here at all, since it's a transitionary screen with a process already underway?

I think this is helpful. Pressing that button would effectively cancel the current process and restart the bootstrapping process after the person changes the settings and returns to this screen.

  • Increasing the contrast of "Swipe to left to see Tor log" will greatly help our visually impaired users (also note the wee typo in there too).

Yep, I saw that too :)

  • Do we need a link to cancel this process in case it hangs? Or provide a back button to screen 1.0?

Ideally, we'll detect when the process hangs and we automatically bring the user to the settings screen so they can change the configuration (assuming they know what they should do such that it'll work).

3.0 (Network)

  • Are users knowledgeable enough to understand that censorship in their location is responsible for Tor's failure to connect?

Right. This is a hard problem. Tor Browser *should* know and be smart about how it handles connection failures. Unfortunately, we don't have that information at this time, so currently we put the burden on the user. Hopefully this will improve in the future.

  • It may not be obvious enough to the user that the browser has successfully connected after hitting the switch, as the success state is quite subtle.
  • Users may not understand that they need to hit the back button to continue (which seems a little counter-intuitive), and could erroneously hit "Change" instead.

Yes, I agree, my implementation of this takes this into account. I used the switch (enabling) as a button. If the switch is currently disabled, and the user presses the switch, then bridges becomes enabled and it instantly moves to the next Bridge configuration screen. If the user returns to this screen after configuring bridges (so the switch is currently enabled), and they press the switch again so it becomes disabled, then the bridges are disabled and the user is returned to the first bootstrapping screen.

4.0 (Combined Network/Bridge proposal)

It's my understanding that there are basically three mutually exclusive options on the table for when Tor's blocked:

  1. Automatic selection
  2. Select from list
  3. Enter manually

So with that in mind, I've wireframed a proposal that combines both the "Network" and "Select a bridge" screens into a single menu to:

  • Help address any ambiguity about censorship in the UI
  • Reduce confusion about how to proceed once an option's been selected (as selecting an option will automatically advance to the next screen to attempt connection).
  • Better surface manual selection and entry for more advanced users.

In this scenario, hitting the automatic option would cycle through the built-in list until a bridge can be successfully connected to.

However nested radio-buttons may not be the best way to do this – thoughts?

Our idea with this is Tor Browser should detect when bootstrapping stalls. If the user tried connecting directly and that failed, then there's likely no harm in automatically trying some of the built-in bridges. If any of those result in successfully bootstrapping, then we're done. If none of them work or if the user already configured a bridge and it failed, then we'll show them the configuration screen so the user can configure an addition bridge for use. In the future, when we have Moat support, we'll be able to automatically query bridges.torproject.org for new bridges, as well, and that'll improve this flow, too - except for the added CAPTCHA, but that's what we have right now.

comment:30 Changed 8 months ago by sysrqb

Status: newneeds_review

There's a branch for review on 28329_6. I ran into multiple bugs, and I resolved some of them. The remaining bugs are:

  1. The (spinning) onion image isn't as large as it's shown in the mockup
  2. There is a up button (back button) in the upper-lefthand corner of the settings screen and it does nothing
  3. The mockup shows a dividing line between radio button bridge types, I failed getting that working
  4. When the gear/cog button is clicked from the Tor Log screen, the bootstrap process isn't stopped

(I'm probably forgetting some others, too)

I took a different approach for built-in bridges than Orbot. The bridges are provided by Gecko preferences the same as Tor Launcher. I think this'll make maintaining the list easier. However, as a result of this, the bridges are fetched asynchronously, so I added a placeholder in the radio button list while we wait for the response from Gecko.

I also added a Save button on the "Provide a Bridge" screen, because inputting a bridge into the the text box and then pressing "Back" didn't seem like an obvious flow.

On the branch, you can ignore the last commit. I simply used that for testing the Radio Button creation.

comment:31 Changed 8 months ago by gk

Keywords: TorBrowserTeam201902R added; TorBrowserTeam201902 removed

Setting the "R" keyword

comment:32 Changed 8 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201902R removed
Status: needs_reviewneeds_revision

Okay, thanks for this. The UI looks nice. I have some issues, though, getting this to run properly in the first place. Here is the build I used for testing:

https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329.apk
https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329.apk.asc

It's based on tor-browser-build's master branch pointing the Firefox project to my bug_28329_test.

If I press the Connect button the app is crashing with OOM errors (I restarted my Samsung Galaxy S5 mini, no dice):

02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: java.lang.OutOfMemoryError: Failed to allocate a 795676 byte allocation with 266048 free bytes and 259KB until OOM
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at dalvik.system.VMRuntime.newNonMovableArray(Native Method)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.BitmapFactory.nativeDecodeAsset(Native Method)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.BitmapFactory.decodeStream(BitmapFactory.java:856)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.BitmapFactory.decodeResourceStream(BitmapFactory.java:675)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.drawable.Drawable.createFromResourceStream(Drawable.java:2230)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.loadDrawableForCookie(Resources.java:4282)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.loadDrawable(Resources.java:4156)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.loadDrawable(Resources.java:4006)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.TypedArray.getDrawable(TypedArray.java:886)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.drawable.AnimationDrawable.inflateChildElements(AnimationDrawable.java:324)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.drawable.AnimationDrawable.inflate(AnimationDrawable.java:294)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.drawable.Drawable.createFromXmlInner(Drawable.java:2551)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.graphics.drawable.Drawable.createFromXml(Drawable.java:2322)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.loadDrawableForCookie(Resources.java:4277)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.loadDrawable(Resources.java:4156)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.getDrawable(Resources.java:2045)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.res.Resources.getDrawable(Resources.java:2027)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.support.v7.widget.ResourcesWrapper.getDrawable(ResourcesWrapper.java:133)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.content.Context.getDrawable(Context.java:464)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.view.View.setBackgroundResource(View.java:18598)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.support.v7.widget.AppCompatImageView.setBackgroundResource(AppCompatImageView.java:76)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at org.mozilla.gecko.torbootstrap.TorBootstrapPanel.startBootstrapping(TorBootstrapPanel.java:175)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at org.mozilla.gecko.torbootstrap.TorBootstrapPanel.access$000(TorBootstrapPanel.java:37)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at org.mozilla.gecko.torbootstrap.TorBootstrapPanel$1.onClick(TorBootstrapPanel.java:61)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.view.View.performClick(View.java:5721)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.widget.TextView.performClick(TextView.java:10955)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.view.View$PerformClick.run(View.java:22620)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.os.Handler.handleCallback(Handler.java:739)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.os.Handler.dispatchMessage(Handler.java:95)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.os.Looper.loop(Looper.java:148)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at android.app.ActivityThread.main(ActivityThread.java:7331)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at java.lang.reflect.Method.invoke(Native Method)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1230)
02-27 10:40:50.210  5350  5350 E GeckoCrashHandler: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1120)

or

02-27 11:44:00.989 24280 24280 E GeckoCrashHandler: java.lang.OutOfMemoryError: OutOfMemoryError thrown while trying to throw OutOfMemoryError; no stack trace available
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: Icon task crashed
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: java.lang.OutOfMemoryError: Failed to allocate a 21 byte allocation with 3024 free bytes and 3024B until OOM
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at java.util.Scanner.<init>(Scanner.java:158)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.loadSites(SuggestedSites.java:221)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.loadFromProfile(SuggestedSites.java:362)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.refresh(SuggestedSites.java:418)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.get(SuggestedSites.java:491)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.get(SuggestedSites.java:459)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.db.SuggestedSites.get(SuggestedSites.java:449)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.icons.preparation.SuggestedSitePreparer.refreshSiteFaviconMap(SuggestedSitePreparer.java:75)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.icons.preparation.SuggestedSitePreparer.initialise(SuggestedSitePreparer.java:29)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.icons.preparation.SuggestedSitePreparer.prepare(SuggestedSitePreparer.java:99)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.icons.IconTask.prepareRequest(IconTask.java:117)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.mozilla.gecko.icons.IconTask.call(IconTask.java:52)
02-27 11:44:01.099 24280 24367 E Gecko/IconTask: 	at org.

comment:33 Changed 8 months ago by gk

antonela: What about

https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329_v2.apk
https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329_v2.apk.asc

?

I don't get it to crash on my phone. The patch I used on top of sysrqb's is:

diff --git a/mobile/android/base/java/org/mozilla/gecko/torbootstrap/TorBootstrapPanel.java b/mobile/android/base/java/org/mozilla/gecko/torbootstrap/TorBootstrapPanel.java
index 743842dca88d..e097a31abb2c 100644
--- a/mobile/android/base/java/org/mozilla/gecko/torbootstrap/TorBootstrapPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/torbootstrap/TorBootstrapPanel.java
@@ -170,16 +170,16 @@ public class TorBootstrapPanel extends FirstrunPanel implements TorBootstrapLogg
         }
         connectButton.setVisibility(View.GONE);
 
-        ImageView spinningOnionHolder = (ImageView) mRoot.findViewById(R.id.tor_bootstrap_onion);
+        //ImageView spinningOnionHolder = (ImageView) mRoot.findViewById(R.id.tor_bootstrap_onion);
 
-        spinningOnionHolder.setBackgroundResource(R.drawable.tor_spinning_onion);
-        AnimationDrawable spinningOnion = (AnimationDrawable) spinningOnionHolder.getBackground();
+        //spinningOnionHolder.setBackgroundResource(R.drawable.tor_spinning_onion);
+        //AnimationDrawable spinningOnion = (AnimationDrawable) spinningOnionHolder.getBackground();
 
         // Begin spinning
-        spinningOnion.start();
+        //spinningOnion.start();
 
         // Make the still image 100% transparent
-        spinningOnionHolder.setImageAlpha(0);
+        //spinningOnionHolder.setImageAlpha(0);
 
         TextView torStatus = (TextView) mRoot.findViewById(R.id.tor_bootstrap_last_status_message);
 
@@ -201,13 +201,13 @@ public class TorBootstrapPanel extends FirstrunPanel implements TorBootstrapLogg
         }
         connectButton.setVisibility(View.VISIBLE);
 
-        ImageView spinningOnionHolder = (ImageView) mRoot.findViewById(R.id.tor_bootstrap_onion);
-        if (null == spinningOnionHolder) {
+        //ImageView spinningOnionHolder = (ImageView) mRoot.findViewById(R.id.tor_bootstrap_onion);
+        /*if (null == spinningOnionHolder) {
             Log.w(LOGTAG, "stopBootstrapping: spinningOnionHolder is null?");
             return;
-        }
-        AnimationDrawable spinningOnion = (AnimationDrawable) spinningOnionHolder.getBackground();
-        if (null != spinningOnion) {
+        }*/
+        //AnimationDrawable spinningOnion = (AnimationDrawable) spinningOnionHolder.getBackground();
+        /*if (null != spinningOnion) {
             // Stop spinning. This is null if we didn't
             // previously call startBootstrapping.
             spinningOnion.stop();
@@ -215,7 +215,7 @@ public class TorBootstrapPanel extends FirstrunPanel implements TorBootstrapLogg
             // Make the still image 0% transparent, but only
             // if there is an animation in the background
             spinningOnionHolder.setImageAlpha(100);
-        }
+        }*/
 
         TextView torStatus = (TextView) mRoot.findViewById(R.id.tor_bootstrap_last_status_message);
         if (null == torStatus) {

which just disables the animation.

Changed 8 months ago by antonela

Attachment: measurements.png added

comment:35 Changed 8 months ago by antonela

comment:33 > That worked! Thanks!

UI Review

1.0 / 2.0 - Bootstrapping

  • The phone gets suspended during the bootstrapping, going black screen. It is probably a local configuration, but can we do something to avoid it?
  • The settings icon needs margin. And probably a better support for different screens dpi. Could we have a SVG?
  • I attached a quick screen elements measurements, maybe it helps to match the design with the development. Also, you can find a clickable one here

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/measurements.png

  • Let me know how you want to work with the animation, I can give you the assets you need or maybe we want to try something different for this release.
  • Could we have Roboto Monospace font at the log screen? If we are keeping the out of the box material, Body2 seems the appropriate font-size :)

3.0 - Network Settings

  • When the user switches the bridge on, let's first animate the switcher to on and then go to the second screen.
  • "You are using a bridge to connect to Tor" needs to fit in two lines and should replace "Config a bridge to connect to Tor" when the bridge is enabled.
  • Let's don't move users to the main screen right after they select a bridge. It's confusing. We can move them to the previous screen, or they can navigate back. I prefer the second option.
  • When users switch off, the "You are using a built-in bridge to connect to Tor" line needs to disappear. Even if that config is going to be persistent until the next switch on, let's hide it when the option is off to avoid confusion.

3.1 - Network Settings/Select a bridge

  • Select a bridge needs left margin
  • The back arrow doesn't work, the bottom navigation one yes

3.1.1 - Network Settings/Provide a Bridge

  • Enter Bridge needs left margin
  • The back arrow doesn't work, the bottom navigation one yes

Can't wait for the next iteration. This work has been huge! We are almost there!

comment:36 Changed 8 months ago by gk

FWIW: I built an .apk with the patches I used for testing on top of the #28802 patch to check for bridge support. I did not get it working so far (while bridges seem to work without the changes in this bug). I'll dig deeper while reviewing.

The bundle I used is:

https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329_28802.apk
https://people.torproject.org/~gk/testbuilds/tor-browser-tbb-nightly-android-armv7-multi-qa-28329_28802.apk.asc

comment:37 Changed 8 months ago by gk

Oh, an I guess we could add a third radio button that let's users choose meek.

comment:38 in reply to:  36 ; Changed 8 months ago by gk

Replying to gk:

FWIW: I built an .apk with the patches I used for testing on top of the #28802 patch to check for bridge support. I did not get it working so far (while bridges seem to work without the changes in this bug). I'll dig deeper while reviewing.

Okay, obfs4 has been failing as the bridge seems unreachable. Some obfs3 bridges worked, though. Fun bugs I found while testing are (for each one start with a clean new installation:

A)

1) Select a built-in bridge
2) Close Tor Browser
3) Start again
4) Click onto the gear icon
5) Bridge usage is selected and it's said you are using a default bridge
6) Tap on the "Change"-link -> bridge panel opens but now nothing is selected

B)

1) Select a built-in bridge by opening the gear icon and tapping on the switch
UI and choosing e.g. obfs4
2) Click again on the gear icon
3) Tapping on the switch UI does *only* change the UI it seems but does not close
the current UI nor opens the bridge selection one (while this happened in step 1) )
4) However, tapping the region "around" the switch UI does disable bridge
selection

(Thus, the switch UI is behaving inconsistenly)

C)

1) Enable bridges
2) Go back quickly so that available default bridge options are not shown yet
3) repeat step 1) + 2)
4) App crashes

comment:39 in reply to:  38 Changed 7 months ago by gk

Replying to gk:

C)

1) Enable bridges
2) Go back quickly so that available default bridge options are not shown yet
3) repeat step 1) + 2)
4) App crashes

I wonder if we can help that issue by reading in the prefs way earlier and just adjust the UI once we open the bridge selection view. I think it's fair game to assume users would very likely want to select a bridge once they tap the gear icon. Thus, we could read the prefs in on the first preferences related view while users are thinking about whether they want to activate bridges or not. However, I think it's even more straightforward to just read them in when Tor Browser starts, especially with the idea that we might want to kind of automate the connection anyway in some near future. That way only the UI needs to get drawn which should go considerably faster and might avoid the crash.

comment:40 Changed 7 months ago by gk

Here comes the detailed review adding to the things mentioned in my previous comments. Overall: really nice work! The biggest issue I have is dealing with our bridge pref handling. I feel we should try avoid custom parsing code (aka TorBridgeList.java). It's less error prone and probably easier to just write a script that converts the .js file we have right now for desktop into whatever is easiest to deal with on mobile during our build.

Anyway, here are my comments: I looked at the code commit-wise and then for b2c12e4881e3aacf192d3cf623028246f40ab3c4 I wrote comments down per file and for TorPreferences.java per class and method. This review is based on code in 28329_6.

dc4d883cefa60af9caa3894360e7f07992174fa1 - not okay

+    <color name="tor_input_background">#B0B0B0</color>

declared, but where is this needed/used?

76e19a2eaa415a5465b4f103232b862362dec515 - okay
183b5baae0849aa0263c47b6df4306110e978070 - okay

b2c12e4881e3aacf192d3cf623028246f40ab3c4

general: often if (null != $foo) and if ($foo != null) mixed, please stick to the latter. That's the overwhelming pattern when doing null checks in the mobile code (although I am not sure why there are small exceptions).

Is preference_tor_network_bridges_enabled.xml copied over from somewhere? If so, please add a hint from where. (I got curious as it's the only file that contains an Apache license and it mentions a PreferenceActivity (you use PreferenceFragment, though).

There are some .xml files that contain MPL 2.0 license headers and some not, please make the behavior consistent. (I guess this means adding the respective license where it is issing).

TorAnimationContainer.java

    public void hide() {

        visible = false;

superfluous newline

this.onFinishListener = listener;

Is this here needed? You don't have it in hide()?

TorBootstrapLogger.java - not okay

Missing license header?

TorBootstrapLogPanel.java - okay

TorBootstrapPanel.java - not okay

s/persistant/persistent/

                // menu again. If the don't solve the problem, then we'll disable it
                // again.

"If they don't"?

        if (null != spinningOnion) {
            // Stop spinning. This is null if we didn't
            // previously call startBootstrapping.
            spinningOnion.stop();

            // Make the still image 0% transparent, but only

How can this be null after you checked for it not being null? Should the comment go above the if-block?

"Make the still image"? Something seems to be missing in that comment.

TorLogEventListener.java - okay

TorBootstrapPagerConfig.java - not okay

Indentation is off by 1 for getDefaultConnectPanel() and getDefaultBootstrapPanel().

TorBootstrapPager.java - okay

TorBridgeList.java - not okay

What about distributing the prefs in torbrowser/assets/distribution/preferences.json and using e.g.
DistroSharedPrefsImport instead of doing our own pref import/parsing? Or some other more Android-y way of dealing with that that does not involve our custom prefs parse logic...

TorPreferences.java

+ * This class (PreferenceActivity)

You meant (TorPreferences)?

+ *   pref_bridges_enabled=null

false instead of null
The line length in the big comment above the class is different which is confusing

+ * always match, and pref_bridges_list must either match

superfluous whitespace at the end of the line

TorPreferences
::onCreate() (okay)
::isValidFragment() (okay)

TorNetworkPreferenceFragment
::onCreate() (okay)
::walkTreeView() <- just for printing debug information? Do we need that at all the places currently using it?

::getListView()

            if (!(view instanceof ViewGroup)) {
                return null;
            }

            if (view == null) {
                return null;
            }

better:

        if (view == null || !(view instanceof ViewGroup)) {
            return null;
        }

::getBridges() (okay, but see my general comment above)
::setBridges()

            // Set Orbot's preference
            Prefs.setBridgesList(bridgeLine);

            if (!editor.commit()) {
                return false;
            }

Swap both so that we are sure writing the pref succeeded and set the Orbot prefs only afterwards to be not out of sync?

::disableBridgesEnabledPreference() <- why not just "disableBridges()"?

            // Clear the saved prefs (it's okay we're using a different
            // SharedPreference.Editor here, they modify the same backend

closing parenthesis is missing and "."

::setTitle() (okay)

TorNetworkBridgeEnabledPreference
::onCreate()

                        // Only launch Fragment is we're enabling bridges.

s/is/if/

::onViewCreated()

// when Preferences the layout is created across multiple

Something is missing here.

s/the Change buttom/the Change button/g

::setBridgeChangeOnClickHandler() - okay

::isBridgeProvided()

//   If the bridgesEnabled switch is off

s/bridgesEnabled/bridgeEnabled/, given that you are using bridgeEnabled in the following code, no? I am debating whether it would clearer to do as/bridgeEnabled/bridgesEnabled/g, given that this belongs to PREFS_BRIDGES_ENABLED which is plural. There is a similar tension between bridgeType/bridgesType for PREFS_BRIDGES_TYPE.

TorNetworkBridgeSelectPreference
::onCreate() - okay
::onViewCreated() - okay
::addBridgeTypeRadioGroupOnCheckedHandler()

// This onl operates on

s/onl/only/

::getBridgeTypeRadioGroup() - okay
::getBridgeTypes() - okay
::getViewSeparator() - not needed right now (together with the two lager TODO blocks)
::populateList()

// The preferences are adding

s/adding/added/

::addBridgeTypeRadioButtons() - okay

TorNetworkBridgeProvidePreference
::onCreate() - okay
::setBridgeProvideText() -- okay
::onViewCreated() -- okay

TorBridgeProvideSaverOnClickListener
::TorBridgeProvideSaverOnClickListener() - okay
::onClick() - okay

Last edited 7 months ago by gk (previous) (diff)

comment:41 Changed 7 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving remaining tickets to March.

comment:42 Changed 7 months ago by gk

Keywords: tbb-8.5 added

Tickets on our radar for 8.5

comment:43 Changed 7 months ago by gk

Another bug I encountered: the gear icon on the log panel behaves differently than the one on the main start screen: For one, tapping it does not restore the connect button on the start screen and does not stop tor. Secondly, once you start to bootstrap all the changes made via the gear icon on the log panel do not seem to take any effect which is confusing.

comment:44 Changed 7 months ago by gk

I tried #28802 on top of this bug and obs3 and obfs4 are working out of the box. However meek not. I suspect that's because we use meek in the prefs file as transport name, compared to meek_lite in Orbot. However, I don't think we should have meek_lite as the transport name on our preferences view as this is confusing and misleading as meek_lite is in fact just meek. So, we might need a small additional patch replacing "meek_lite" with "meek" on our side.

comment:45 in reply to:  44 Changed 7 months ago by gk

Replying to gk:

I tried #28802 on top of this bug and obs3 and obfs4 are working out of the box. However meek not. I suspect that's because we use meek in the prefs file as transport name, compared to meek_lite in Orbot. However, I don't think we should have meek_lite as the transport name on our preferences view as this is confusing and misleading as meek_lite is in fact just meek. So, we might need a small additional patch replacing "meek_lite" with "meek" on our side.

So, yes, replacing meek in the prefs line with meek_lite makes this work. And looking at the code again we can easily show the label we want by replacing the meek-related part of the pref key to, say, meek (instead of meek-azure as it is right now). But I guess following what we have on desktop is the right thing to do...

(Oh, and all that said if we need to take this into account as well if we get away from our own prefs parsing and use a more Android-y way of dealing with those preferences instead)

comment:46 Changed 7 months ago by gk

Two additional bugs I found and was able to reproduce:

1) Say you want to use custom bridges. BridgeDB gives usually three of those out. So, the natural thing to do is to select, copy and paste those three lines. However, they are pasted into the _one_ line we have on the custom bridge view. The hint that one needs to have one bridge per line is a good one. But there is no way to get there with the three bridges copied and pasted. What we need to have here is a multiline textbox instead, I think.

2) Assuming you are running with the one custom bridge you configured during 1) and select the gear icon on the start view you'll get informed that you are running Tor Browser with a custom bridge with the link to change that. That's good. However, that does not help me if I want to fall back to a built-in bridge because clicking the change-link only gets me to the custom bridge input field. So, if I want to use a built-in bridge instead I have to first disable using bridges altogether just to reenable them later on and change the custom bridge type I want. That's a bit cumbersome...

comment:47 Changed 7 months ago by sysrqb

Status: needs_revisionneeds_review

anto, gk, thanks for the reviews! I believe I corrected all of the comments. Anto, do you have an opinion on how we should handle GeKo's comments in comment:46? I haven't tried this new branch with Gk's patch for #28802 nor with sisbell's new backend. That is next on my list of priorities.

The new branch is 28329_13 in my repo, and i uploaded an apk for testing:
https://people.torproject.org/~sysrqb/fennec-60.5.1.en-US.android-arm_28329_13.apk

I'm curious if the animation still results in a crash. I deleting some of the frames, so it should not require as much memory - if this is indeed what was causing the OOM.

I think it's safe to begin reviewing this branch, but as there may be some more small changes needed for integrating with other patches maybe wait another day.

In addition, as an aside, the UI looks nice on a smaller-screen device like a phone, it does not look as nice on a larger-screen like a tablet.

comment:48 Changed 7 months ago by gk

Just to give quick feedback (I'll look closer tomorrow): Very nice and the app is not crashing for me anymore.

comment:49 in reply to:  48 Changed 7 months ago by sysrqb

Replying to gk:

Just to give quick feedback (I'll look closer tomorrow): Very nice and the app is not crashing for me anymore.

Great news, thanks!

Three known issues with this branch:

  1. Delete unused animation frames
  2. Revert package name change (used for testing)
  3. Delete the "Change" and "Recommended" strings

The differences I saw between the app and the mockup are:

  1. When a built-in bridge is selected, the text says "built-in bridge" instead of "obfs4 bridge" (or another PT name).
  2. Similarly, the text only says "Internet is censored here" instead of switching between "Internet is censored here" and Tor is censored here" depending on the bridge.
  3. The background color of the textbox for providing a bridge is a different shade of gray
  4. I didn't use an SVG for the settings icon in this version

comment:50 in reply to:  47 ; Changed 7 months ago by gk

Replying to sysrqb:

anto, gk, thanks for the reviews! I believe I corrected all of the comments. Anto, do you have an opinion on how we should handle GeKo's comments in comment:46? I haven't tried this new branch with Gk's patch for #28802

One thing I noticed both with your older and newest branch for this bug is that it seems Tor is happily bootstrapping for a while before switching to bridges. I have not looked deeper if that's actually an issue but it makes me suspicious to see, with your older branch, Bootstrapped 5% and 10% messages before getting the warnings that e.g. some obfs3 bridges are not reachable and then bootstrapping continues with Bootstrapped 15% and 20% messages before I see notices that new bridge descriptors got fetched (for those two bridges that _are_ actually working).

comment:51 Changed 7 months ago by antonela

This build is crashing for me. Seems the same animation issue. Can we do a different implementation there instead of a frame to frame animation? I can provide you a .svg animation which should be lighter.

Also, icons and margins are in good shape now. I see the gear icon pixelated; we may want to have a .svg there to contemplate the different densities with one source.

comment:52 Changed 7 months ago by gk

Status: needs_reviewneeds_revision

comment:53 in reply to:  50 ; Changed 7 months ago by sysrqb

Replying to gk:

Replying to sysrqb:

anto, gk, thanks for the reviews! I believe I corrected all of the comments. Anto, do you have an opinion on how we should handle GeKo's comments in comment:46? I haven't tried this new branch with Gk's patch for #28802

One thing I noticed both with your older and newest branch for this bug is that it seems Tor is happily bootstrapping for a while before switching to bridges. I have not looked deeper if that's actually an issue but it makes me suspicious to see, with your older branch, Bootstrapped 5% and 10% messages before getting the warnings that e.g. some obfs3 bridges are not reachable and then bootstrapping continues with Bootstrapped 15% and 20% messages before I see notices that new bridge descriptors got fetched (for those two bridges that _are_ actually working).

I noticed soemthing similar on rare occasions, too. I have trouble reproducing it, though. It seems related to the background TorService stopping Tor. Maybe there is a race condition where the user presses the connect button in tor browser, tor browser begins the bootstrapping process and sends the "start tor" signal to TorService, TorService starts the Tor process and waits for the control port. If the user presses the settings icon before TorService successful opens a controller connection, then the "stop tor" signal is ignored.

This seems a little different than the bug you described because Tor shouldn't begin bootstrapping without bridges and then suddenly switch to using a bridge. However, I haven't tested this branch with working bridges yet, so maybe these bugs are related. I'll try reproducing this bug today.

comment:54 in reply to:  51 ; Changed 7 months ago by sysrqb

Replying to antonela:

This build is crashing for me. Seems the same animation issue. Can we do a different implementation there instead of a frame to frame animation? I can provide you a .svg animation which should be lighter.

:( sadness. We can try svg. The one problem with using an SVG is Android support. Currently, TBA is supported on API versions 16+, but SVG support (or VectorDrawable on Android) was only added in version 21. I think we can try our best here, and we can use use the vectorized animation when it is supported and fallback on the rasterized version when it is not supported.

https://developer.android.com/reference/android/graphics/drawable/AnimatedVectorDrawable

Also, icons and margins are in good shape now. I see the gear icon pixelated; we may want to have a .svg there to contemplate the different densities with one source.

Hrm. yeah. This is a similar problem as above. We can add higher-density icons or we can use the vectorized image when it is supported and fallback on the png when VectorDrawable isn't available.

https://developer.android.com/reference/android/graphics/drawable/VectorDrawable

comment:55 in reply to:  46 Changed 7 months ago by sysrqb

Replying to gk:

Two additional bugs I found and was able to reproduce:

1) Say you want to use custom bridges. BridgeDB gives usually three of those out. So, the natural thing to do is to select, copy and paste those three lines. However, they are pasted into the _one_ line we have on the custom bridge view. The hint that one needs to have one bridge per line is a good one. But there is no way to get there with the three bridges copied and pasted. What we need to have here is a multiline textbox instead, I think.

Oh. I see. This is a bug I'll fix. I just tried this and when multiple lines are pasted into the text box they are concatenated into a single line...that's not what I expected would happen.

comment:56 in reply to:  53 Changed 7 months ago by gk

Replying to sysrqb:

Replying to gk:

Replying to sysrqb:

anto, gk, thanks for the reviews! I believe I corrected all of the comments. Anto, do you have an opinion on how we should handle GeKo's comments in comment:46? I haven't tried this new branch with Gk's patch for #28802

One thing I noticed both with your older and newest branch for this bug is that it seems Tor is happily bootstrapping for a while before switching to bridges. I have not looked deeper if that's actually an issue but it makes me suspicious to see, with your older branch, Bootstrapped 5% and 10% messages before getting the warnings that e.g. some obfs3 bridges are not reachable and then bootstrapping continues with Bootstrapped 15% and 20% messages before I see notices that new bridge descriptors got fetched (for those two bridges that _are_ actually working).

I noticed soemthing similar on rare occasions, too. I have trouble reproducing it, though. It seems related to the background TorService stopping Tor. Maybe there is a race condition where the user presses the connect button in tor browser, tor browser begins the bootstrapping process and sends the "start tor" signal to TorService, TorService starts the Tor process and waits for the control port. If the user presses the settings icon before TorService successful opens a controller connection, then the "stop tor" signal is ignored.

This seems a little different than the bug you described because Tor shouldn't begin bootstrapping without bridges and then suddenly switch to using a bridge. However, I haven't tested this branch with working bridges yet, so maybe these bugs are related. I'll try reproducing this bug today.

I actually think that's a general Tor issue has I see it on desktop as well (I guess I could have tested that earlier :( ):

Mar 13 14:11:38.000 [notice] Bootstrapped 5%: Connecting to directory server
Mar 13 14:11:38.000 [notice] Bootstrapped 10%: Finishing handshake with directory server
Mar 13 14:11:38.000 [warn] Proxy Client: unable to connect to 2001:470:b381:bfff:216:3eff:fe23:d6c3:443 ("general SOCKS server failure")
Mar 13 14:11:38.000 [notice] Bootstrapped 15%: Establishing an encrypted directory connection
Mar 13 14:11:38.000 [notice] Bootstrapped 20%: Asking for networkstatus consensus
Mar 13 14:11:38.000 [notice] new bridge descriptor 'ndnop3' (fresh): $8DFCD8FB3285E855F5A55EDDA35696C743ABFC4E~ndnop3 at 109.105.109.165

That's weird, though...

Last edited 7 months ago by gk (previous) (diff)

comment:57 Changed 7 months ago by sysrqb

Oh, another bug. During bootstrapping the log text should be selectable (and copyable).

One other UX issue I noticed while testing this is I swipe between the main bootstrapping screen and the log screen - in particular when Tor takes a long time to bootstrap - and after tor finishes bootstrapping and the browser is shown I still try swiping to the left so I can see the tor logs. I wonder if this is a feature we want in the future? I don't know if users need easy access to the tor logs like this and I'm a little worried this could interfere with some website functionality if we aren't careful, but maybe this could be used for displaying the site-specific tor circuit and tor logs(?). Just a thought.

comment:58 in reply to:  54 Changed 7 months ago by sysrqb

Replying to sysrqb:

:( sadness. We can try svg. The one problem with using an SVG is Android support. Currently, TBA is supported on API versions 16+, but SVG support (or VectorDrawable on Android) was only added in version 21. I think we can try our best here, and we can use use the vectorized animation when it is supported and fallback on the rasterized version when it is not supported.
..
Hrm. yeah. This is a similar problem as above. We can add higher-density icons or we can use the vectorized image when it is supported and fallback on the png when VectorDrawable isn't available.

Okay, these were completely non-issues. I have an vector "settings gear" working without a problem. Android provided a backwards-compatible method for supporting vector images and it works transparently at run-time (it's actually quite nice). The button is a little smaller now, but we can iterate on that.

I just need the spinning onion as an SVG now.

comment:59 Changed 7 months ago by sysrqb

Okay, i have another branch I think is closer. 28329_15.

comment:60 in reply to:  57 Changed 7 months ago by gk

Replying to sysrqb:

Oh, another bug. During bootstrapping the log text should be selectable (and copyable).

One other UX issue I noticed while testing this is I swipe between the main bootstrapping screen and the log screen - in particular when Tor takes a long time to bootstrap - and after tor finishes bootstrapping and the browser is shown I still try swiping to the left so I can see the tor logs. I

Yes, I know that feeling. :)

wonder if this is a feature we want in the future? I don't know if users need easy access to the tor logs like this and I'm a little worried this could interfere with some website functionality if we aren't careful, but maybe this could be used for displaying the site-specific tor circuit and tor logs(?). Just a thought.

Hm. I think we need to come up with some instructions/ideas for users giving us log information to debug problems, yes. For desktop we have the option to tell Tor Launcher/Torbutton to do so and output the result at least in the browser console on all three platforms. This is not possible for mobile. What is the general model for Android apps in that regard (I assume other apps have similar requirements)? We probably want to follow that then. I don't mind making the site-specific circuit available in the log or easily accessible. We can think about that. But that should not replace our plan in #25764.

comment:61 Changed 7 months ago by gk

Okay, I finally managed to reproduce another bug I was hunting. (I still can't paste the log which makes the report a bit tricky, bear with me :) )

Steps to reproduce:

1) Use an .apk based on 28329_15
2) Once started click on the gear icon and enter a (working) vanilla bridge in the bridge details
3) Go back to the bootstrapping view and hit the Connect button
4) Once you see the "SUCCESS connected to Tor control port." message on the log, click again on the gear icon
5) Disable the proxy, go back to the bootstrapping view and tap Connect
6) You'll see a "Unable to start Tor" message after a bit mentioning a NullPointerException when invoking org.torproject.android.control.TorControlConnection.setEventHandler(org.torproject.android.control.EveentHandler)
7) Tor stops bootstrapping as a result

comment:62 Changed 7 months ago by gk

Here is the review of 28329_15. Unless mentioned here all the issues mentioned in comment:39 are solved.

General: often if (null != $foo) and if ($foo != null) are mixed, please stick to the latter.

res/drawable/ic_baseline_settings_20px.xml - license?
res/drawable/list_section_divider_material.xml - android license; where did the XML stuff get borrowed from or is that an original Android file?
res/drawable/tor_spinning_onion.xml - license?
For readability (and consistency across files) newlines between XML header, license, and the meat of the files would be good.

"ViewPager containing for our bootstrapping pages"

s/containing for/for containting/ ?

"stop bootstrapping animation"

s/stop/stop the/

nit: "being used. There" <- one whitespace too much :) (in TorPreferences.java)

"clicks on the Change link" <- missing "." at the end
"-1 == changeStart" -> "changeStart == -1"

"if meek-azure if chosen" -> s/if chosen/is chosen/

Other remaining issues I had in previous comments (just to have all in one comment) are mentioned in comment:61 comment:46 part 1).

A new one I saw while testing: the switch for enabling/disabling bridges itself is jumping a bit during the transition, probably depending on the text. I think the correct behavior would be that the switch stayed where it is and just the text "moves".

comment:63 Changed 7 months ago by gk

Oh, one thing I forgot: there is no need to show a separator at the end of the custom bridge screen.

comment:64 Changed 7 months ago by gk

// This implements TorNetworkBridgePopulateList so it can receive the list
// of bridges asynchronously from Gecko.

can go as well.

comment:65 Changed 7 months ago by gk

// Request the list of built-in bridges after the View is created

, too.

comment:66 in reply to:  46 Changed 7 months ago by antonela

Replying to gk:

2) Assuming you are running with the one custom bridge you configured during 1) and select the gear icon on the start view you'll get informed that you are running Tor Browser with a custom bridge with the link to change that. That's good. However, that does not help me if I want to fall back to a built-in bridge because clicking the change-link only gets me to the custom bridge input field. So, if I want to use a built-in bridge instead I have to first disable using bridges altogether just to reenable them later on and change the custom bridge type I want. That's a bit cumbersome...

I see. Also there is not any clue at the Select Bridge screen that allow users to know that that the known bridge provided was successfully saved.

We can fix both issues having a toggle menu at that list.
https://marvelapp.com/4fcjj0e/screen/54462993

So:

  • when users tap on Select a Bridge, the bridge options get displayed.
  • when users tap on Provide a Bridge I Know, we move users to the second screen. When they back to Select Bridge screen, they can see the description updated like:

https://marvelapp.com/4fcjj0e/screen/54462995

Then, when users got back to the Network Settings screen, we maintain the current description update.

Changed 7 months ago by antonela

comment:67 in reply to:  46 Changed 7 months ago by antonela

Replying to gk:

1) Say you want to use custom bridges. BridgeDB gives usually three of those out. So, the natural thing to do is to select, copy and paste those three lines. However, they are pasted into the _one_ line we have on the custom bridge view. The hint that one needs to have one bridge per line is a good one. But there is no way to get there with the three bridges copied and pasted. What we need to have here is a multiline textbox instead, I think.

Do you expect something like this?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/28329/TBA%20Settings%20Network%20-%20Bridge%20-%20known.png

On MD docs, the multi-line text field gets expanded right after the user writes/copypaste.

https://material.io/design/components/text-fields.html#input-types

Maybe It can get solved by changing the text field type, not the height.

comment:68 Changed 7 months ago by sysrqb

Okay, I have a new branch and I think I solved most of the bugs. 28329_17.

comment:69 Changed 7 months ago by gk

Keywords: tbb-parity added

tbb-parity items.

Changed 7 months ago by antonela

Attachment: icon1x1.svg added

Changed 7 months ago by antonela

Attachment: icon.svg added

comment:70 Changed 7 months ago by antonela

Keywords: tbb-parity removed

Testing
tor-browser-8.5a8-android-armv7-multi-qa_28329.apk

Bootstrapping screen

Select a Bridge

  • Select a bridge and Provide a bridge I know should have the same padding/margin around.

Provide a Bridge

  • The keyboard has a . We should have the Enter/Confirm icon there back.
  • When going back to the previous screen, the keyboard keeps open.
Last edited 7 months ago by antonela (previous) (diff)

comment:71 Changed 7 months ago by gk

Here is what is still left over after looking at 28329_17

res/drawable/tor_spinning_onion.xml - license?

For readability (and consistency across files) newlines between XML header, license, and the meat of the files would be good.

"ViewPager containing for our bootstrapping pages" s/containing for/for containting/ ?

"stop bootstrapping animation" s/stop/stop the/

nit: "being used. There" <- one whitespace too much :) (in TorPreferences.java)

"clicks on the Change link" <- missing "." at the end

"if meek-azure if chosen" -> s/if chosen/is chosen/

+    // This implements TorNetworkBridgePopulateList so it can receive the list
+    // of bridges asynchronously from Gecko.

can go

// Request the list of built-in bridges after the View is created

can go, too

'substitute it with "meek_lite"'. s/meek_lite/meek/

The switch for enabling/disabling bridges itself is jumping a bit during the transition, probably depending on the text. I think the correct behavior would be that the switch stayed where it is and just the text "moves".

comment:61 is still an issue.

comment:72 Changed 7 months ago by gk

Oh, I forgot one thing from the list

// Runnable for changing the alpha of the Onion image every two seconds.

yet private long mOnionAlphaChangerSleepInterval = 1000; so did you mean "every second"?

comment:73 in reply to:  71 Changed 7 months ago by sysrqb

Replying to gk:

Here is what is still left over after looking at 28329_17

res/drawable/tor_spinning_onion.xml - license?

For readability (and consistency across files) newlines between XML header, license, and the meat of the files would be good.

"ViewPager containing for our bootstrapping pages" s/containing for/for containting/ ?

"stop bootstrapping animation" s/stop/stop the/

nit: "being used. There" <- one whitespace too much :) (in TorPreferences.java)

"clicks on the Change link" <- missing "." at the end

"if meek-azure if chosen" -> s/if chosen/is chosen/

+    // This implements TorNetworkBridgePopulateList so it can receive the list
+    // of bridges asynchronously from Gecko.

can go

// Request the list of built-in bridges after the View is created

can go, too

'substitute it with "meek_lite"'. s/meek_lite/meek/

All addressed.

The switch for enabling/disabling bridges itself is jumping a bit during the transition, probably depending on the text. I think the correct behavior would be that the switch stayed where it is and just the text "moves".

This is a little difficult, unfortunately. I won't solve this today.

comment:61 is still an issue.

Same here, this may be a bug in Orbot. It needs more investigation.

comment:74 in reply to:  72 Changed 7 months ago by sysrqb

Replying to gk:

Oh, I forgot one thing from the list

// Runnable for changing the alpha of the Onion image every two seconds.

yet private long mOnionAlphaChangerSleepInterval = 1000; so did you mean "every second"?

Corrected. Now it is slightly different, too.

comment:75 Changed 7 months ago by sysrqb

Status: needs_revisionneeds_review

Okay, I pushed a new branch. 28329_19

comment:76 Changed 7 months ago by sysrqb

The bootstrap-onion is still too wide, i'll need some more time for finding how to solve this.

comment:77 Changed 7 months ago by sysrqb

Okay, i pushed another branch, 28329_20 with some deleted newlines around licenses. I cleaned up part 1 files, but the xml files in part 4 weren't consistent. This is now corrected, too.

comment:78 Changed 7 months ago by gk

Status: needs_reviewneeds_revision

Looking over 28329_20 I got some new nits:

all the expected preferences are is <- s/are is/are/

// If bridgesLine1 was not null, then append a newline. <- bridgesLine1 can be null and we still can land there with bridgeLine2 being not null (which can happen if a user for some reason leaves the first line out but starts to populate the second (and third line) instead; so maybe "If bridgesLine1 or bridgesLine2 was not null"

Log.i(LOGTAG, "provideBridge is empty. Disabling."); I feel we should only emit his log line or a similar one if we are actually disabling bridges, but that's not clear at that point. Thus, please move this line to a place where disabling bridges is essentially certainly to happen (and maybe do a "provideBridge is" -> "custom bridges are").

Otherwise this looks okay. I won't push fixup commits to tor-browser-60.6.esr-8.5-1 as there a probably major revisions coming taking that into account.

For posterity, the patches in 28329_19 got applied to tor-browser-60.5.1esr-8.5-1 as commits 0db4c9243320d2c516329949ee17e8b0a5b13624
a6e87093856101ec93bbeaf93eed22ac77031f73
fbf54c71e26a64859f953e18a34e5c9e8831f071
e4d2df5e776b6270b5eb98f4b718ea6eb5ea5094
87d6c829225d271ba94fa21672b157085dbf1ad9

and should get shipped in 8.5a9.

comment:79 in reply to:  78 Changed 7 months ago by sysrqb

Replying to gk:

Looking over 28329_20 I got some new nits:

all the expected preferences are is <- s/are is/are/

TorNetworkPreferenceFragment:disableBridges

// If bridgesLine1 was not null, then append a newline. <- bridgesLine1 can be null and we still can land there with bridgeLine2 being not null (which can happen if a user for some reason leaves the first line out but starts to populate the second (and third line) instead; so maybe "If bridgesLine1 or bridgesLine2 was not null"

Yes, I'll move this comment so it is clearer. This comment is specifically explaining why we add the "\n" + string. Moving it within the if-block above the string assignment should help.

Log.i(LOGTAG, "provideBridge is empty. Disabling."); I feel we should only emit his log line or a similar one if we are actually disabling bridges, but that's not clear at that point. Thus, please move this line to a place where disabling bridges is essentially certainly to happen (and maybe do a "provideBridge is" -> "custom bridges are").

Agreed.

New branch coming soon.

comment:80 Changed 7 months ago by gk

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

Marking blockers for Tor Browser 8.5.

comment:81 Changed 7 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:82 Changed 6 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:83 Changed 6 months ago by sysrqb

Adding a note here, unfortunately, we can't add the spinning onion animation using the third-party lottie library. This is due to an incompatibility between our current dependencies and the library's dependencies. Tor Browser for Android currently requires the Android Support Library version 23.4.0 but Lottie requires (at a minimum) version 25.0.1. I looked at all released versions of Lottie, and the oldest released version (v1.0.0) depends on Support Library 25.0.1 - and Lottie v1.0.0 was released over two years ago (so this would require serious consideration anyway).

This isn't disqualifying us from using lottie in the future, possibly after we move to 68esr where Firefox for Android uses Android Support Library 26.1.0 (same as Lottie), but we simply can't use it at this moment.

https://mvnrepository.com/artifact/com.airbnb.android/lottie

comment:84 Changed 6 months ago by sysrqb

Suprisingly, #29757 may be needed for supporting API level 16. More recent versions of Android do not throw this SecurityException (they simply log an error and continue), but Orbot fails during its startup process as a result of this exception being thrown, so the app won't bootstrap.

E/Orbot   ( 1935): error onBind
E/Orbot   ( 1935): java.lang.SecurityException: Permission Denial: opening provider org.torproject.torbrowser.ui.hiddenservices.providers.HSContentProvider from ProcessRecord{a76ef718 1935:org.torproject.torbrowser_alpha/u0a45} (pid=1935, uid=10045) that is not exported from uid 10044
E/Orbot   ( 1935):      at android.os.Parcel.readException(Parcel.java:1425)
E/Orbot   ( 1935):      at android.os.Parcel.readException(Parcel.java:1379)
E/Orbot   ( 1935):      at android.app.ActivityManagerProxy.getContentProvider(ActivityManagerNative.java:2354)
E/Orbot   ( 1935):      at android.app.ActivityThread.acquireProvider(ActivityThread.java:4219)
E/Orbot   ( 1935):      at android.app.ContextImpl$ApplicationContentResolver.acquireUnstableProvider(ContextImpl.java:1703)
E/Orbot   ( 1935):      at android.content.ContentResolver.acquireUnstableProvider(ContentResolver.java:1099)
E/Orbot   ( 1935):      at android.content.ContentResolver.query(ContentResolver.java:354)
E/Orbot   ( 1935):      at android.content.ContentResolver.query(ContentResolver.java:313)
E/Orbot   ( 1935):      at org.torproject.android.service.TorService.processSettingsImpl(TorService.java:1659)
E/Orbot   ( 1935):      at org.torproject.android.service.TorService.updateTorConfigFile(TorService.java:703)
E/Orbot   ( 1935):      at org.torproject.android.service.TorService.torUpgradeAndConfig(TorService.java:625)
E/Orbot   ( 1935):      at org.torproject.android.service.TorService.access$800(TorService.java:94)
E/Orbot   ( 1935):      at org.torproject.android.service.TorService$2.run(TorService.java:571)
E/Orbot   ( 1935):      at java.lang.Thread.run(Thread.java:856)
W/ActivityManager( 1409): Permission denied: checkComponentPermission() owningUid=10044
W/ActivityManager( 1409): Permission denied: checkComponentPermission() owningUid=10044
W/ActivityManager( 1409): Permission Denial: opening provider org.torproject.torbrowser.ui.hiddenservices.providers.HSContentProvider from ProcessRecord{a76ef718 1935:org.torproject.torbrowser_alpha/u0a45} (pid=1935, uid=10045) that is not exported from uid 10044

For example, API level 19 simply logs:

E/ActivityThread( 3446): Failed to find provider info for org.torproject.torbrowser.ui.hiddenservices.providers                                                                                                   
E/ActivityThread( 3446): Failed to find provider info for org.torproject.torbrowser.ui.hiddenservices.providers.cookie

comment:85 Changed 6 months ago by sysrqb

Status: needs_revisionneeds_review

I pushed branch 28329_28. I deleted some of the animation-specific code we don't need currently. The main significant additional code is adjusting the onion graphic's width and height such that they aren't stretched. Android was forcing square dimensions with the SVG.

comment:86 in reply to:  85 ; Changed 6 months ago by gk

Status: needs_reviewneeds_revision

Replying to sysrqb:

I pushed branch 28329_28. I deleted some of the animation-specific code we don't need currently. The main significant additional code is adjusting the onion graphic's width and height such that they aren't stretched. Android was forcing square dimensions with the SVG.

Thanks! So, without looking at the code changes in detail yet, two issues:

1) The onion is too big on the bootstrap panel (at least for my device) it gets cropped on the right side (just some pixels but one sees that that part of it is "not round" anymore).

2) After finishing the onboarding the about:tor page is loaded but there is no space between the text (Tor Browser Explore. Privately.) and the URL bar which looks a bit weird. I think the layout of that site should be as for the subsequent starts.

comment:87 in reply to:  86 ; Changed 6 months ago by gk

Replying to gk:

Replying to sysrqb:

I pushed branch 28329_28. I deleted some of the animation-specific code we don't need currently. The main significant additional code is adjusting the onion graphic's width and height such that they aren't stretched. Android was forcing square dimensions with the SVG.

[snip]

2) After finishing the onboarding the about:tor page is loaded but there is no space between the text (Tor Browser Explore. Privately.) and the URL bar which looks a bit weird. I think the layout of that site should be as for the subsequent starts.

Regarding that issue, I am not sure exactly why this is happening as the fixup commit looks good to me. However, double-checking with 8.5a8 shows it's a regression.

Review so far:

50729a60c1761e18a5586f0728cb68e9db0aff45 -- good
ea2c582e0af381a2230d0288d457f92bfb401765 -- good
db7b31d9a234c1f03a5c5508538f31d820aa525e -- good

re: 1ee605c225fdf42631db9bd80790823eb700702d is that one we want to keep (just having it called fixup! makes me actually wonder. I suppose if it's not just a debugging help it was meant to be a fixup to the parent)?

comment:88 Changed 6 months ago by gk

364e234d66ec101995e1fae1254e632fd23eb69b -- not okay

currentTop does not really seem to be needed.

        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

You already calculated the ratios and saved them in variables, no need to
calculate them again here (and elsewhere!).

// Add a classback for I guess you meant "callback"?

+        //     Adjust the width when the current width is greater than the expected
+        //     width.
+        //   - The opposite is likely true when the device in in landscape mode with
+        //     respect to the height and width. Adjust the height when it is greater
+        //     than the expected height.
+        //
+        // Subtract 1 from the expected value as a way of accounting for rounding
+        // error.
+        if (currentWidth < (expectedWidth - 1)) {
+            newHeight = expectedHeight;
+        } else if (currentHeight < (expectedHeight - 1)) {
+            newWidth = expectedWidth;
+        }

That part is a bit dense. I was wondering for a while where the width gets actually adjusted when it is greater than expected as neither of your two if-clauses is actually checking that as your comment suggests. So, ideally your comment would at least explain why this scenario falls into the currentHeight < (expectedHeight -1) check.

s/in in/is in/

comment:89 in reply to:  86 Changed 6 months ago by sysrqb

Replying to gk:

Replying to sysrqb:

I pushed branch 28329_28. I deleted some of the animation-specific code we don't need currently. The main significant additional code is adjusting the onion graphic's width and height such that they aren't stretched. Android was forcing square dimensions with the SVG.

Thanks! So, without looking at the code changes in detail yet, two issues:

1) The onion is too big on the bootstrap panel (at least for my device) it gets cropped on the right side (just some pixels but one sees that that part of it is "not round" anymore).

Yes, I found that was introduced in 8.5a9 when we moved to the vectorized onion image. It seems the hard-coded viewport within the image wasn't correct, so it always cropped the right-side of the image by a few pixels. Adjusting this resolves the issue.

comment:90 in reply to:  87 ; Changed 6 months ago by sysrqb

Replying to gk:

2) After finishing the onboarding the about:tor page is loaded but there is no space between the text (Tor Browser Explore. Privately.) and the URL bar which looks a bit weird. I think the layout of that site should be as for the subsequent starts.

Regarding that issue, I am not sure exactly why this is happening as the fixup commit looks good to me. However, double-checking with 8.5a8 shows it's a regression.

Hrm. I'm not sure I see this. I'll upload a screenshot of about:tor for comparison.

re: 1ee605c225fdf42631db9bd80790823eb700702d is that one we want to keep (just having it called fixup! makes me actually wonder. I suppose if it's not just a debugging help it was meant to be a fixup to the parent)?

Yep, that was a mistake. I forgot to merge that into the previous commit before updating the ticket.

Changed 6 months ago by sysrqb

comment:91 in reply to:  88 ; Changed 6 months ago by sysrqb

Replying to gk:

364e234d66ec101995e1fae1254e632fd23eb69b -- not okay

currentTop does not really seem to be needed.

Deleted now.

        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

You already calculated the ratios and saved them in variables, no need to
calculate them again here (and elsewhere!).

I deleted the ratio variables because I found the syntax confusing due to additional type casting and needing more nested parentheses. This was included in the accidental-fixup commit, but I can revert this change if you think the ratio variables are more readable.

// Add a classback for I guess you meant "callback"?

I sure do.

+        //     Adjust the width when the current width is greater than the expected
+        //     width.
+        //   - The opposite is likely true when the device in in landscape mode with
+        //     respect to the height and width. Adjust the height when it is greater
+        //     than the expected height.
+        //
+        // Subtract 1 from the expected value as a way of accounting for rounding
+        // error.
+        if (currentWidth < (expectedWidth - 1)) {
+            newHeight = expectedHeight;
+        } else if (currentHeight < (expectedHeight - 1)) {
+            newWidth = expectedWidth;
+        }

That part is a bit dense. I was wondering for a while where the width gets actually adjusted when it is greater than expected as neither of your two if-clauses is actually checking that as your comment suggests. So, ideally your comment would at least explain why this scenario falls into the currentHeight < (expectedHeight -1) check.

I'll add more details in the comment.

s/in in/is in/

ack.

Changed 6 months ago by gk

Attachment: about_tor_no_space.png added

Changed 6 months ago by gk

Attachment: about_tor_space.png added

comment:92 Changed 6 months ago by gk

about_tor_no_space.png shows the issue I only have after running the onboarding. about_tor_space.png is how it should be and how it is for subsequent runs (yes, I know the screenshot quality has room for ímprovement :) ).

Last edited 6 months ago by gk (previous) (diff)

comment:93 in reply to:  90 Changed 6 months ago by gk

Replying to sysrqb:

Replying to gk:

2) After finishing the onboarding the about:tor page is loaded but there is no space between the text (Tor Browser Explore. Privately.) and the URL bar which looks a bit weird. I think the layout of that site should be as for the subsequent starts.

Regarding that issue, I am not sure exactly why this is happening as the fixup commit looks good to me. However, double-checking with 8.5a8 shows it's a regression.

Hrm. I'm not sure I see this. I'll upload a screenshot of about:tor for comparison.

re: 1ee605c225fdf42631db9bd80790823eb700702d is that one we want to keep (just having it called fixup! makes me actually wonder. I suppose if it's not just a debugging help it was meant to be a fixup to the parent)?

Yep, that was a mistake. I forgot to merge that into the previous commit before updating the ticket.

Okay, the commit looks good to me, though (module the commit message)

comment:94 in reply to:  91 ; Changed 6 months ago by gk

Replying to sysrqb:

Replying to gk:

[snip]

        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

You already calculated the ratios and saved them in variables, no need to
calculate them again here (and elsewhere!).

I deleted the ratio variables because I found the syntax confusing due to additional type casting and needing more nested parentheses. This was included in the accidental-fixup commit, but I can revert this change if you think the ratio variables are more readable.

Works for me. In that case please remove

        final double imgDimensionRatioHeightWidth = imgHeight/imgWidth;
        final double imgDimensionRatioWidthHeight = imgWidth/imgHeight;

.

[snip]

comment:95 in reply to:  94 ; Changed 6 months ago by sysrqb

Status: needs_revisionneeds_review

Replying to gk:

Replying to sysrqb:

Replying to gk:

[snip]

        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

You already calculated the ratios and saved them in variables, no need to
calculate them again here (and elsewhere!).

I deleted the ratio variables because I found the syntax confusing due to additional type casting and needing more nested parentheses. This was included in the accidental-fixup commit, but I can revert this change if you think the ratio variables are more readable.

Works for me. In that case please remove

        final double imgDimensionRatioHeightWidth = imgHeight/imgWidth;
        final double imgDimensionRatioWidthHeight = imgWidth/imgHeight;

Those should be deleted in the fixup! commit.

-        final int expectedHeight = (int) (((double) currentWidth)*imgDimensionRatioHeightWidth);
-        final int expectedWidth = (int) (((double) currentHeight)*imgDimensionRatioWidthHeight);
+        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
+        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

In any case, I pushed a new branch with these corrections, and I modified the dense comment above the width/height conditional so it is clearer (I hope). Branch 28329_30.

For the onion size, I adding a little more logic for choosing the size. If the image's width is greater than 600dp then set the max width at 600dp and scale the height accordingly. However, if the the current width is already near 600dp (+/- 100dp), then set the width at 400dp and scale the height accordingly. I'm hoping this allows for a better experience on lower-density/lower-resolution screens.

As for the space between the about:tor text and the url bar, I don't know what it causing that. I haven't successfully reproduced it. We added some code for preventing this by reloading the page when the chrome (url bar) is rendered after bootstrapping completes. I noticed we reload the page and allow Gecko to use the cache. I wonder if this is the bug, where it is using the cached version instead of re-rendering it, but I don't know why I can't reproduce it. In this new branch I forced bypass-cache with reload, so I'm curious if this solves the problem.

In addition, I now have a spinning onion animation file we may be able to use. I don't know if we'll be can get it into this release.

comment:96 Changed 6 months ago by sysrqb

Okay, last branch with a small addition (unregister the global ViewTreeLayoutListener) - this prevents spamming the log with debug messages regarding layout updates after bootstrapping completes. I pushed 28329_31.

comment:97 in reply to:  95 ; Changed 6 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

Replying to gk:

[snip]

Works for me. In that case please remove

        final double imgDimensionRatioHeightWidth = imgHeight/imgWidth;
        final double imgDimensionRatioWidthHeight = imgWidth/imgHeight;

Those should be deleted in the fixup! commit.

-        final int expectedHeight = (int) (((double) currentWidth)*imgDimensionRatioHeightWidth);
-        final int expectedWidth = (int) (((double) currentHeight)*imgDimensionRatioWidthHeight);
+        final int expectedHeight = (int) (currentWidth*imgHeight/imgWidth);
+        final int expectedWidth = (int) (currentHeight*imgWidth/imgHeight);

In any case, I pushed a new branch with these corrections, and I modified the dense comment above the width/height conditional so it is clearer (I hope). Branch 28329_30.

For the onion size, I adding a little more logic for choosing the size. If the image's width is greater than 600dp then set the max width at 600dp and scale the height accordingly. However, if the the current width is already near 600dp (+/- 100dp), then set the width at 400dp and scale the height accordingly. I'm hoping this allows for a better experience on lower-density/lower-resolution screens.

Works and looks indeed better on my phone, thanks!

As for the space between the about:tor text and the url bar, I don't know what it causing that. I haven't successfully reproduced it. We added some code for preventing this by reloading the page when the chrome (url bar) is rendered after bootstrapping completes. I noticed we reload the page and allow Gecko to use the cache. I wonder if this is the bug, where it is using the cached version instead of re-rendering it, but I don't know why I can't reproduce it. In this new branch I forced bypass-cache with reload, so I'm curious if this solves the problem.

It does!

In addition, I now have a spinning onion animation file we may be able to use. I don't know if we'll be can get it into this release.

We'll see. Let's do this in a follow-up bug.

Anyway, I've closed the child tickets and referenced the respective commits there. Additional bug fixes from 28329_31 made it into commit be91c6f0a5cba14a7ba0e17682b158ada97667be and 7b5f5ee887cd6fd865e86784ad536b4c0136ce83 on tor-browser-60.6.1esr-8.5-1

I'll file follow-up tickets for the following issues (please double-check that I did not miss any):

  1. The spinning onion
  2. The custom bridge line solution (we'd like to have a multi-line form here)
  3. The "switch-jump-issue" if one enables/disables bridges (see comment:71)
  4. The potentially orbot-related null pointer exception (see comment:61)

comment:98 in reply to:  97 Changed 6 months ago by gk

Replying to gk:

[snip]

I'll file follow-up tickets for the following issues (please double-check that I did not miss any):

  1. The spinning onion

#30129

  1. The custom bridge line solution (we'd like to have a multi-line form here)

#30130

  1. The "switch-jump-issue" if one enables/disables bridges (see comment:71)

#30131 (I took the freedom to add another issue with the switch to the ticket which I found which should be higher prio than just the jumping, maybe we can solve both in that ticket, though)

  1. The potentially orbot-related null pointer exception (see comment:61)

#30132

I put all of them on the tbb-8.5 radar but I am not convinced yet that any of them is tbb-8.5-must.

comment:99 Changed 5 months ago by gk

FWIW, I pushed a fix-up commit that replaces a bunch of Orbot instances with tor-android-service and makes the comment about possible bridge pref states a bit clearer in case built-in bridges are used. The changes are on tor-browser-60.6.1esr-8.5-1 (commit 937038361e7529779f5bfce42b7e2e4007542503)

Last edited 5 months ago by gk (previous) (diff)
Note: See TracTickets for help on using tickets.