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?

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.

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.

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.

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.

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.

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.

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?

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 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?

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.

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.

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:

The device doesn't have internet connectivity

The network is censoring Tor connections (and the user needs a bridge)

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.

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.

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.

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.

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)? :)

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.

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? :)

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.

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)? :)

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.

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?

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?

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.

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:

Automatic selection

Select from list

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?

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

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:

Automatic selection

Select from list

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.

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

The (spinning) onion image isn't as large as it's shown in the mockup

There is a up button (back button) in the upper-lefthand corner of the settings screen and it does nothing

The mockup shows a dividing line between radio button bridge types, I failed getting that working

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.

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

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!

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.

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

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.

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.

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?

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.

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.

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.

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)

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...

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.

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).

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.

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.

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.

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.

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 :( ):

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.

:( 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.

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.

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

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.

Other remaining issues I had in previous comments (just to have all in one comment) are mentioned in comment:61comment: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".

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.

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?

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

+ // 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".

+ // 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".

// 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.

// 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").

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.

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.

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.

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.

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)?

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.

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.

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.

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.

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 :) ).

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.

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.

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.

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.

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.

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):

The spinning onion

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

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

FWIW, I pushe 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)