re-implement desktop onboarding for ESR 68

added BugSmashFund TorBrowserTeam201909R actualpoints::5 component::applications/tor browser ff68-esr owner::tbb-team parent::30429 points::4 priority::medium resolution::fixed severity::normal status::closed type::defect labels

Following mcs suggestion, I resurrected the onboarding system addon. Then I cherry picked the original Tor onboarding commits and finally converted the bootstrapped extension into a webextension + experimental API, which is how it was done for other extensions (e.g. formautofill).

Implemented in 6 last commits of https://github.com/acatarineu/tor-browser/commits/28822 (based on current 30429 branch).

Trac:
Keywords: ff68-esr deleted, ff68-esr TorBrowserTeam201906R added
Status: new to needs_review

No reviews in June 2019 anymore, moving them.

Trac:
Keywords: ff68-esr TorBrowserTeam201906R deleted, TorBrowserTeam201907R, ff68-esr added

No July any longer.

Trac:
Keywords: TorBrowserTeam201907R deleted, TorBrowserTeam201908R added

Adding Sponsor 44 to ESR68 tickets

Trac:
Sponsor: N/A to Sponsor44-can

Trac:
Parent: N/A to #30429 (moved)

Okay, I took the commits from your 30429+9 and here are my comments:

ea5f8c2c9ccc0130944f05a6e31ebb2a9322041c - I think it's mostly good. Two things:

+const {Services}

I think we settled for space between name and braces in Torbutton? Would be good here as well.

+	@$(MAKE) -C ../extensions/onboarding/locales chrome AB_CD=$*

It's really just one level up here, right? While all the other items are two levels up?

1c9eb3993c5b505c0894b13634b09690bfb97791 - not okay (not sure about the changed onboarding-overlay-button but we'll see I guess while testing)

The images are the wrong ones. We want to have those from #30560 (moved).

6f05a139b387c072a63bfae3a086aee2cee95875 - okay e19e128476f48278911656db735739f0526f12ce - not okay

-/* The primary button gets the same color as the customize button. */

in browser/themes/shared/UITour.inc.css is missing

742fccfcbb7a19ba9daee44335e9962639773d13 - not okay

-  OnboardingTelemetry: "re[/onboarding/modules/OnboardingTelemetry.jsm",](/onboarding/modules/OnboardingTelemetry.jsm",)

in browser/extensions/onboarding/bootstrap.js is missing

aeb0b6678e61fd282825610ca29a225eb0991281 - I think this is okay, but are we affected by https://bugzilla.mozilla.org/show_bug.cgi?id=1498378?

I'll take the patches, though, for tor-browser-68.1.0esr-9.0-1, so please provide fixups for the issues above based on that one. (I am about to rebase and push it to tor-browser).

Trac:
Keywords: TorBrowserTeam201908R deleted, TorBrowserTeam201908 added
Status: needs_review to needs_revision

Oh, and while rebasing I saw we have

# UITour
# DuckDuckGo .onion (used for circuit display onboarding).
origin	uitour	1	https://3g2upl4pq6kufc4m.onion
origin	uitour	1	about:home
origin	uitour	1	about:newtab
origin	uitour	1	about:tor

I guess about:home and about:newtab can go now (at least that's my expression after re-looking at the webextension converting commit?

Additionally, upon further testing it seems to me that the details part of the circuit level and the security settings is not working as expected. While clicking on the former opens the DDG .onion, no tour through the display shows up trying to make the menu behind the security settings button visible by clicking on the option does not do anything either.

Moving tickets to September

Trac:
Keywords: TorBrowserTeam201908 deleted, TorBrowserTeam201909 added

Trac:
Points: N/A to 4

Addressed comments in https://github.com/acatarineu/torbutton/commit/28822 and https://github.com/acatarineu/tor-browser/commits/28822+1 (last 5 commits).

Replying to gk:

Okay, I took the commits from your 30429+9 and here are my comments:

ea5f8c2c9ccc0130944f05a6e31ebb2a9322041c - I think it's mostly good. Two things:
+const {Services}
}}}
I think we settled for space between name and braces in Torbutton? Would be good
here as well.

Fixed.

{{{
   @$(MAKE) -C ../extensions/onboarding/locales chrome AB_CD=$*
}}} It's really just one level up here, right? While all the other items are two levels up?

We could change it to

{{{

@
(MAKE) -C ../../browser/extensions/onboarding/locales chrome AB_CD=
*

but I think it's the same as the current one, since the file is already in browser `browser/locales/Makefile.in`.

> 1c9eb3993c5b505c0894b13634b09690bfb97791 - not okay (not sure about the changed `onboarding-overlay-button` but we'll see I guess while testing)

I changed the css to match the old patch, since the `onboarding-overlay-attention-dot` was not showing properly.

> The images are the wrong ones. We want to have those from #30560.

Fixed.

> 6f05a139b387c072a63bfae3a086aee2cee95875 - okay
> e19e128476f48278911656db735739f0526f12ce - not okay
> {{{
> -/* The primary button gets the same color as the customize button. */
> }}}
> in `browser/themes/shared/UITour.inc.css` is missing

Fixed.
 


> 742fccfcbb7a19ba9daee44335e9962639773d13 - not okay
> {{{
> -  OnboardingTelemetry: "re[/onboarding/modules/OnboardingTelemetry.jsm",](/onboarding/modules/OnboardingTelemetry.jsm",)
> }}}
> in `browser/extensions/onboarding/bootstrap.js` is missing

Fixed.

> aeb0b6678e61fd282825610ca29a225eb0991281 - I think this is okay, but are we affected by https://bugzilla.mozilla.org/show_bug.cgi?id=1498378?

We are, nice catch. Reverted that one too.

> Additionally, upon further testing it seems to me that the details part of the circuit level and the security settings is not working as expected. While clicking on the former opens the DDG .onion, no tour through the display shows up trying to make the menu behind the security settings button visible by clicking on the option does not do anything either.

It seems there were several recent patches which did not like the onboarding. The security settings not opening is a regression due to #31322 and then #31251. The first one prevents UITour-lib.js from loading, and the second one makes `window.document.getElementById("security-level-button").doCommand()` not open the security level anymore (not sure why).

The circuit path problem seems to be a regression caused by the FPI permissions patches. I tried reverting `Bug 1330467 - part 1. Don't strip first party domain from permissions key; r=johannh,Ehsan` and it works. It also works if you disable `firstparty.isolate`.

The problem is that the permission check in [UITourChild.jsm](https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/browser/components/uitour/UITourChild.jsm#91) returns `false` after that patch is applied. I think this is because the `uitour` permission for the DDG onion page is not being loaded in the content process (where this is called). These are loaded in [ContentParent.cpp](https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/dom/ipc/ContentParent.cpp#5450) (for the principal). Before the FPI permission patch (or when disabling `firstparty.isolate`, this is called with key `https://3g2upl4pq6kufc4m.onion`, which loads the permission properly. After the patch it's called with key `https://3g2upl4pq6kufc4m.onion^firstPartyDomain=3g2upl4pq6kufc4m.onion`, which apparently does not work. This issue does not occur with `about:tor` `uitour` permission: this is always loaded because `nsPermissionManager::GetKeyForOrigin` returns empty keys for origins not starting with `http://`, `https://` or `ftp://`. The convention is that permissions with empty keys are default permissions and always preloaded (see [ContentParent.cpp](https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/dom/ipc/ContentParent.cpp#1340)). So I fixed this by adding `uitour` to the permissions that are always preloaded.



**Trac**:  
**Keywords**: TorBrowserTeam201909 **deleted**,  TorBrowserTeam201909R **added**  
**Status**: needs_revision **to** needs_review

Thanks for the digging and the fixups. Very nice work! I cherry-picked the Torbutton commit onto master (commit d796dd27d12850bd176a85e160002755bfccfab2) and the other ones onto tor-browser-68.1.0esr-9.0-2 (commits b4e8db514df16856b567be1e13cb9af11a7de6e5 535d41fd820d5b94caf43f3e403dc74b3566c837 915d521a2fae009509a229bdaeb1d3a95a17b8c1 e1ad91f3031dd08a0591d0f9d472cb49e015ad75 5d0ffb6cea1e3eca56884f087120f2b40aa2926b).

I've opened a new ticket to do The Right Thing with respect to our onboarding that is we need a new one fitting to the new Firefox reality instead of backing out patches and hacking around the breakage: #31660 (moved).

FWIW: I think doCommand() is not working anymore due to the following change:

-               oncommand="SecurityLevelButton.onCommand(this, event);"
+               onmousedown="SecurityLevelButton.onCommand();"

There is no oncommand handler anymore but an onmousedown one which presumably fires, too, if one clicks.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

hi acat, could you set the "Actual Points" field with (more or less) how long it took you to complete this, that way we can see whether our estimates are accurate.

Thanks!

I don't remember exactly, since the first patch was done 3 months ago. If we include the regressions and addressing the review comments that had to be done later I would say something like 5.

Trac:
Actualpoints: N/A to 5

BugSmashFund can be used for the ESR work done so far

Trac:
Keywords: N/A deleted, BugSmashFund added

Sponsor 44 only covered PM and Team Lead work

Trac:
Sponsor: Sponsor44-can to N/A

closed

changed time estimate to 32h

added 40h of time spent

mentioned in issue #29768 (moved)

mentioned in issue #30429 (moved)

mentioned in issue #31660 (moved)

moved to tpo/applications/tor-browser#28822 (closed)

mentioned in issue tpo/applications/tor-browser#31660 (closed)

re-implement desktop onboarding for ESR 68

Child items ...

Activity