Opened 2 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#26884 closed defect (fixed)

Update preferences.xul to make it work on mobile

Reported by: igt0 Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, tbb-mobile, TorBrowserTeam201808
Cc: sysrqb, gk, arthuredelstein Actual Points:
Parent ID: #26531 Points:
Reviewer: Sponsor:

Description

TorButton preferences.xul doesn't work on mobile. It is simpler to make it work on mobile than updating the tor-browser-settings extension.

Child Tickets

Attachments (1)

screenshot_slider.png (55.7 KB) - added by igt0 7 weeks ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 2 months ago by gk

Component: Applications/TorbuttonApplications/Tor Browser
Keywords: tbb-torbutton TorBrowserTeam201807 added
Owner: set to tbb-team
Parent ID: #26531
Priority: MediumVery High

comment:2 Changed 2 months ago by igt0

XUL components are not well supported on mobile, e.g. the slider doesn't render. I am migrating the XUL markup to XHTML.

comment:3 Changed 7 weeks ago by igt0

Status: newneeds_review

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

You can see the patches here:
https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/6dfbf8bc311208a14f146f80fb285dcd5efc3f45

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Changed 7 weeks ago by igt0

Attachment: screenshot_slider.png added

comment:4 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201807 removed

comment:5 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201807R removed

comment:6 Changed 7 weeks ago by gk

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

comment:7 in reply to:  6 ; Changed 7 weeks ago by igt0

Replying to gk:

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

It is not integrated in the build process. You need to copy the xpi to the distribution directory(mobile/android/torbrowser/assets/distribution/extensions).

Besides that you will need to update the extension preferences info:
extensions.enabledAddons: Update the torbutton version to 2.0.1

extensions.legacy.enabled: It needs to be true

I wonder if we can integrate it in the work made by sisbell.

comment:8 in reply to:  7 Changed 7 weeks ago by sysrqb

Replying to igt0:

Replying to gk:

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

It is not integrated in the build process. You need to copy the xpi to the distribution directory(mobile/android/torbrowser/assets/distribution/extensions).

For this

  1. export TB_BUILD_WITH_DISTRIBUTION=1 (or set this env variable however you like) so the distribution is included by .mozconfig-android
  2. create mobile/android/torbrowser/assets/distribution/extensions, as igt0 said, if that directory structure doesn't already exist, and copy the xpi into the extensions directory.

comment:9 in reply to:  3 ; Changed 5 weeks ago by sysrqb

Cc: arthuredelstein added
Status: needs_reviewneeds_revision

Replying to igt0:

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

The original XUL implementation still works on Desktop, correct? Only mobile uses XHTML?

You can see the patches here:
https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Seems okay - but Arthur, maybe you want to skim through one?


Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/6dfbf8bc311208a14f146f80fb285dcd5efc3f45

Missing (optional) semi-colon - but all the other lines end with a semi-colon:

+// Set the desired slider value and update UI.
+function torbutton_set_slider(sliderValue) {
+  state.slider = sliderValue

(preferences.js is missing a semicolon on this line, too)


Unnecessary semi-colon after the closing curly bracket of a few functions. It seems like those may be copied from preferences.js.


descNames and linkNames are reversed in preferences-mobile.js (compared to preferences.js). Can you add a comment about this?


SECURITY_PREFERENFES_URI should be _PREFERENCES_?


Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Seems okay.

Do we need to add fennec as a new target application in install.rdf?

+        <!-- fennec -->
+        <em:targetApplication>
+            <Description>
+                <em:id>{aa3c5121-dab2-40e2-81ca-7ea25febc110}</em:id>
+                <em:minVersion>60.0</em:minVersion>
+                <em:maxVersion>10000.0</em:maxVersion>
+            </Description>
+        </em:targetApplication>

comment:10 Changed 5 weeks ago by sysrqb

Also, how did you bypass the addon signiture restriction?

Gecko : 1534486863961 addons.xpi WARN Invalid XPI: signature is required but missing

loadManifest() calls mustSign()

comment:11 Changed 5 weeks ago by sysrqb

Oh, right. Okay, i got this working by toggling xpinstall.signatures.required. Then I forced installation by going to file:///data/data/org.torproject.torbrowser/distribution/extensions/ and tapping on torbutton@torproject.org.xpi. After restarting the app, torbutton runs and I see "Security Settings" in the menu. igt0, nice job.

When I open the Security Settings, I see the slider. However, tapping the actual label "Standard" and "Safest" result in the position jumping to the other location (tapping on "Standard" causes the location indicator to jump to "Safest", and vice versa). I think the noscript settings are reversed, as well.

comment:12 in reply to:  9 Changed 5 weeks ago by gk

Replying to sysrqb:

Replying to igt0:

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

The original XUL implementation still works on Desktop, correct? Only mobile uses XHTML?

You can see the patches here:
https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Seems okay - but Arthur, maybe you want to skim through one?

I have some nits:
a) Please keep the comment which was available above torbutton_show_torbrowser_manual().
b)

// torbutton_show_torbrowser_manual() returns true.

needs to get reworded.
c) If there is an error in get_general_useragent_locale() we should have some way of showing it (e.g. by emitting a message to the console), even if the usual logging functionality is not available in the module. I'd like to avoid omitting any kind of notification in such a case.

comment:13 in reply to:  9 Changed 5 weeks ago by igt0

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Seems okay.

Do we need to add fennec as a new target application in install.rdf?

+        <!-- fennec -->
+        <em:targetApplication>
+            <Description>
+                <em:id>{aa3c5121-dab2-40e2-81ca-7ea25febc110}</em:id>
+                <em:minVersion>60.0</em:minVersion>
+                <em:maxVersion>10000.0</em:maxVersion>
+            </Description>
+        </em:targetApplication>

I am not sure, because legacy extensions are not supported by default in Fennec and I believe the targetApplication is a way to protect the user of installing it in unsupported versions. So even if the user tries to install just the torbutton, Fennec will not allow it.

comment:14 Changed 5 weeks ago by igt0

Status: needs_revisionneeds_review

Updated code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/d69a48bbaa5fbed41e11448ed5800f9f75cfd124

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/67b54c3d6a767dfa0146967a8e3675d8dee1e8ff

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/742843fe8fd181cf7c8632779069517814d6110b

comment:15 in reply to:  14 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewneeds_revision

Replying to igt0:

Updated code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/d69a48bbaa5fbed41e11448ed5800f9f75cfd124

Let's keep the loglevel to 4 as we had it in the original code. And probably add err to the log string to give some clue about what went wrong? I guess just the "Error while getting locale" + err would do it.

Last edited 5 weeks ago by gk (previous) (diff)

comment:16 Changed 5 weeks ago by igt0

Status: needs_revisionneeds_review

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/056b82ca7e77c1d3848b149a74b644a2ef003519

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/b6392b49c8eb7f586cbc7dcfb07be991a80f0e2b

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/8bd10b44346f26e5d8e881fd33e3458e80b9ee91

comment:17 Changed 5 weeks ago by gk

Status: needs_reviewneeds_revision

Thanks, looks good. We are close here. Could you do a final rebase of your patches against master? There are a bunch of conflicts due to the desktop onboarding/about:tor-changes.

Last edited 5 weeks ago by gk (previous) (diff)

comment:18 in reply to:  17 Changed 5 weeks ago by igt0

Rebased code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/9757d18579fd63340cd2877ea0b26da9cb61eb91

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/9b194a0a6e8ea659d59024796f5df33f4b0c327c

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/cc90adc868bfaebe298db09155a9412334e1dfa3

Here is the branch:
https://github.com/igortoliveira/torbutton/commits/26884-v4

Replying to gk:

Thanks, looks good. We are close here. Could you do a final rebase of your patches against master? There are a bunch of conflicts due to the desktop onboarding/about:tor-changes.

comment:19 Changed 5 weeks ago by igt0

Status: needs_revisionneeds_review

comment:20 Changed 5 weeks ago by gk

Status: needs_reviewneeds_revision

Looks good, one final nit: could you remove the comma in torOn: torbutton_tor_check_ok(),? We only have one item in the object.

comment:21 in reply to:  20 Changed 5 weeks ago by igt0

Status: needs_revisionneeds_review

Updated code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/17aae7176531a7fa5617b31a6c6dd22168f689fb

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/2bf6712165fde40317556eefbd9c33d223f9a010

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/b88b7493c6831697c7214ef6de93addf48237b03

Replying to gk:

Looks good, one final nit: could you remove the comma in torOn: torbutton_tor_check_ok(),? We only have one item in the object.

comment:22 Changed 5 weeks ago by gk

Status: needs_reviewneeds_information

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

comment:23 in reply to:  22 ; Changed 5 weeks ago by gk

Replying to gk:

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

I tested with the patch for #27220 and still got the same results. Here is the bundle I used:

https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk
https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk.asc

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

FWIW, that's #27220.

comment:24 Changed 5 weeks ago by gk

Status: needs_informationneeds_revision

While looking at #27221 I copied an .xpi with this fix over an older Torbutton version and the browser on desktop is not showing the about:tor page anymore. It's complaining about the torbutton logger not available yet (undefined reference error). Thus, we need a better fix here.

comment:25 in reply to:  23 Changed 5 weeks ago by igt0

I opened the apk and it doesn't have the noscript extension in the distribution directory. It is why you are seeing JS enabled.

Replying to gk:

Replying to gk:

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

I tested with the patch for #27220 and still got the same results. Here is the bundle I used:

https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk
https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk.asc

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

FWIW, that's #27220.

comment:26 Changed 5 weeks ago by igt0

Status: needs_revisionneeds_review

Arthur noticed we don't need the torbutton_get_general_useragent_locale because the getLocale does the exactly same thing and it is simpler. So I removed torbutton_get_general_useragent_locale.

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils
https://github.com/igortoliveira/torbutton/commit/5b5931efdf9d8d8bc018d5c02abb14d4e379e160

Bug 26884 - Part 2: Create mobile security slider
https://github.com/igortoliveira/torbutton/commit/690a3836779975202fd722579dfa5997953af61d

Bug 26884 - Part 3: Remove optionsURL from install.rdf
https://github.com/igortoliveira/torbutton/commit/86adebdcf40095b8ac449129f6c6458c03d7a747

comment:27 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, this looks good now, thanks! (And, indeed, the missing Noscript was the cause of the problem I encountered).

I merged your branch to master (commit 86adebdcf40095b8ac449129f6c6458c03d7a747) and tagged 2.0.3 we can use for the alpha.

comment:28 Changed 4 weeks ago by gk

Resolution: fixed
Status: closedreopened

Damn, I should have tested it again:

JavaScript error: chrome://torbutton/content/torbutton.js, line 2251: TypeError: show_torbrowser_manual is not a function

this happens on a desktop Tor Browser (8.0a10 Linux).

comment:29 in reply to:  28 Changed 4 weeks ago by gk

Resolution: fixed
Status: reopenedclosed

Replying to gk:

Damn, I should have tested it again:

JavaScript error: chrome://torbutton/content/torbutton.js, line 2251: TypeError: show_torbrowser_manual is not a function

this happens on a desktop Tor Browser (8.0a10 Linux).

Okay, I think what happened was me testing the commit before bumping the Torbutton version and then later, after tagging, I saw the error. Blowing the startupCache away manually "solves" this problem. igt0 verified that an actual version bump has the same effect. Thus, we are good here it seems and can proceed with 2.0.3.

comment:30 Changed 4 weeks ago by gk

Keywords: tbb-mobile added
Note: See TracTickets for help on using tickets.