TBA has tabs and private tabs, we only want private tabs

[sysrqb]

Having both tabs and private tabs is confusing. We should remove all instances of non-private tabs.

[sysrqb]

Moving these to #5709, these aren't blockers anymore. We'll merge Orfox patches first, then audit everything as we move to m-c and work on TBA. (We should create more tickets as we find more items that need investigating/fixing)

[sysrqb]

(It's almost June, but I'll be optimistic with the keyword :) )

Setting this for review, this is only for alpha - it's in my repo bug24920_01.

[gk]

Moving reviews over to June.

[gk]

Two things I am unsure about:

1) It's a bit sad to see the look of the browsing experience differ now from what we had before as it seems the private mode is differently styled (dark grey URL bar e.g.). This is not the case on desktop it seems? We get the same look regardless whether we have permanent private browsing mode on or off. I have not checked why that's the case, but I feel if possible then it is worth aligning the mobile and desktop experience here (and by that I mean that we'd ideally not make a visual difference indicating which mode we are on to not leak that information).

2) I spent a bit time trying to figure out why you do

+    let isPrivate = (("isPrivate" in aParams) && aParams.isPrivate) || Services.prefs.getBoolPref("browser.privatebrowsing.autostart")

Is that for defense-in-depth? Or is there an actual need for that? Ideally, I think the check for that pref (or, better: for whether we are in private browsing mode or not) should be done somewhere else if it is missing and not at this place (but that's a bit hard to say as I am not sure why it is needed), but it could get at least a comment explaining why it is there.

I am leaving this ticket open for addressing 2) and moved along with the changes meanwhile to get 9.0a2 built. I cherry-picked them to tor-browser-60.7.0esr-9.0-1 (commits 6d81ef66c55d5b88f39ee6527f8218b2f2e15cd8, 1666b0186346ab2769614d3b19bc2058fd6f13f0, 1bce86ec5cfff504baf6e61b9d356ff74c62b297, and 41b5dac4943dcf65582f2638fe6d1b5c80127c0a).

Could you please based your commits next time on whatever the current HEAD of the current tor-browser dev branch points to? I had to debug first why my testbuild was failing and it turned out that the Torbutton submodule commit you had on your branch did not point to any of the Torbutton commits in the repository but to one on a branch in my public Torbutton repo. Thus, the build was failing as the submodule update failed.

[sysrqb]

Replying to gk:

Two things I am unsure about:

1) It's a bit sad to see the look of the browsing experience differ now from what we had before as it seems the private mode is differently styled (dark grey URL bar e.g.). This is not the case on desktop it seems? We get the same look regardless whether we have permanent private browsing mode on or off. I have not checked why that's the case, but I feel if possible then it is worth aligning the mobile and desktop experience here (and by that I mean that we'd ideally not make a visual difference indicating which mode we are on to not leak that information).

This is an interesting point. On desktop, the only visual indication of using a normal or private window is in the window's title bar (at least on Linux). The theme doesn't change, but on Android the equivalent of "Dark theme" is enabled when private tabs are used. Honestly, I'm not sure what we should do here. Following Mozilla's lead on this and using the dark theme with private tabs, like Fennec does, is the easy solution and tells the user "you are currently using private tabs". But, this should be the default in Tor Browser, so maybe that visual indicator isn't needed. As an alternative, we can use the "normal tab" colors with private tabs like tor browser on desktop, and if autostart is disabled we can return to using the two different color schemes.

2) I spent a bit time trying to figure out why you do

+    let isPrivate = (("isPrivate" in aParams) && aParams.isPrivate) || Services.prefs.getBoolPref("browser.privatebrowsing.autostart")

Is that for defense-in-depth? Or is there an actual need for that? Ideally, I think the check for that pref (or, better: for whether we are in private browsing mode or not) should be done somewhere else if it is missing and not at this place (but that's a bit hard to say as I am not sure why it is needed), but it could get at least a comment explaining why it is there.

Ah, yes, I can add a comment on this. It is needed for when extensions open their preferences page. These pages are opened entirely within the chrome javascript, so the Java part of this patch is never reached. By default, extensions open normal tabs for their preferences.

I am leaving this ticket open for addressing 2) and moved along with the changes meanwhile to get 9.0a2 built. I cherry-picked them to tor-browser-60.7.0esr-9.0-1 (commits 6d81ef66c55d5b88f39ee6527f8218b2f2e15cd8, 1666b0186346ab2769614d3b19bc2058fd6f13f0, 1bce86ec5cfff504baf6e61b9d356ff74c62b297, and 41b5dac4943dcf65582f2638fe6d1b5c80127c0a).

Could you please based your commits next time on whatever the current HEAD of the current tor-browser dev branch points to? I had to debug first why my testbuild was failing and it turned out that the Torbutton submodule commit you had on your branch did not point to any of the Torbutton commits in the repository but to one on a branch in my public Torbutton repo. Thus, the build was failing as the submodule update failed.

Sorry for this, yes, I'll be more careful about this in the future.

[sysrqb]

For comparison.

[sysrqb]

Replying to sysrqb:

This is an interesting point. On desktop, the only visual indication of using a normal or private window is in the window's title bar (at least on Linux).

Okay, that isn't entirely true. Private windows have a purple/white mask icon in the upper-right corner, too. There's limited space in the chrome on most Android phones, so I don't think adding another image like that is best option, either.

[gk]

Replying to sysrqb:

Replying to sysrqb:

This is an interesting point. On desktop, the only visual indication of using a normal or private window is in the window's title bar (at least on Linux).

Okay, that isn't entirely true. Private windows have a purple/white mask icon in the upper-right corner, too. There's limited space in the chrome on most Android phones, so I don't think adding another image like that is best option, either.

Interesting, that's not what I see. I don't see any sign of private browsing mode on desktop at all. If I open my normal Firefox ESR and do open a new private window with, say, Ctrl+Shift+P then I get the result you describe but not in my Tor browser. Interestingly, if I open the menubar and go to File then the "New Private Window" option has the Ctrl+N shortcut which is traditionally only for opening normal Windows.

[Thorin]

FYI: In FF (desktop), if you start in normal mode, then PB windows get the purple mask top right. If you start in PB Mode then all windows will be PB Mode (despite the menu item called New Window = effectively just a new PB window) and the purple mask is never shown. It's been this way since forever (that I've known).

Edit: that's the pref browser.privatebrowsing.autostart which is true for TB but default false for FF. Toggle that and restart the browser and redo your tests.

[pili]

I wonder if we need some UX input here, what are the timelines for this? 9.0 stable?

[gk]

Replying to pili:

I wonder if we need some UX input here, what are the timelines for this? 9.0 stable?

If we care about that it would be 9.0 stable, yes. I am still on the fence about how much effort we should spend here, though, given that Fenix is coming sooner than later...

[sysrqb]

Replying to gk:

Replying to pili:

I wonder if we need some UX input here, what are the timelines for this? 9.0 stable?

If we care about that it would be 9.0 stable, yes. I am still on the fence about how much effort we should spend here, though, given that Fenix is coming sooner than later...

I think we should decide what UI/UX we want here, because I expect we'll carry this decision over to fenix, as well.

[antonela]

Some comments:

+1 on keeping consistency between desktop and mobile. If the light theme is the default in desktop, we want to have the same default in mobile. Which of those themes (or even a custom one) are the default is a long discussion happening at #10399. So, for the TBA release could we have the same UX you got in the latest nightly but with the light/default theme?

Following Mozilla's lead on this and using the dark theme with private tabs, like Fennec does, is the easy solution and tells the user "you are currently using private tabs". But, this should be the default in Tor Browser, so maybe that visual indicator isn't needed.

I agree. The default in Tor Browser is a private tab + a lot of other things. Yesterday we talked about having or not the word "private" at the label menu items. Maybe, is redundant (and not accurate) to have New Private Tab in the Tor Browser. Just New Tab seems better. Could re-name New Private Tab to New Tab on the kebab menu?

Could we replace the ​mask icon with an onion icon? Would you let me know the sizes you need for that icon? Seems like the regular 16, 32, 64, 128 assets could do the job.

[gk]

Replying to antonela:

Some comments:

+1 on keeping consistency between desktop and mobile. If the light theme is the default in desktop, we want to have the same default in mobile. Which of those themes (or even a custom one) are the default is a long discussion happening at #10399. So, for the TBA release could we have the same UX you got in the latest nightly but with the light/default theme?

Following Mozilla's lead on this and using the dark theme with private tabs, like Fennec does, is the easy solution and tells the user "you are currently using private tabs". But, this should be the default in Tor Browser, so maybe that visual indicator isn't needed.

I agree. The default in Tor Browser is a private tab + a lot of other things. Yesterday we talked about having or not the word "private" at the label menu items. Maybe, is redundant (and not accurate) to have New Private Tab in the Tor Browser. Just New Tab seems better. Could re-name New Private Tab to New Tab on the kebab menu?

#25660 might be relevant here. Where we had been thinking about "New Private Window" vs. "New Window".

[sysrqb]

Replying to gk:

#25660 might be relevant here. Where we had been thinking about "New Private Window" vs. "New Window".

I think we should leave the questions about UI changes for #25660, and decide if the current implementation of this in Tor Browser Alpha on Android is working correctly. This is going stable within the next few weeks, and I want this to be a change we are happy about shipping.

I'll push a fixup commit for the comment in comment:10, and I'll spend a couple hours on modifying the dark theme (as mentioned in comment:9). I'll leave that for another ticket if I don't finish in that time frame.

[sysrqb]

I pushed bug24920_05 with the added comment, but I want to test it first before setting this for review because I made one small code change, as well.

[sysrqb]

Okay, marking this for review - bug24920_05.

I noticed in the commit-diff from the 68esr rebase (1d4697df174ff83b6efa1f568b2ee154cb20b93e) I accidentally changed the syntax on one of the lines.

     if (isPrivate) {
-      attrs.privateBrowsingId = 1;
+      attrs['privateBrowsingId'] = 1;
     }

This fixup changes it back, too.

[gk]

Looks good. Cherry-picked onto tor-browser-68.1.0esr-9.0-2 (commit a07356ac7693bec4ed1b206bdbeb64287cb2a7f6). Resetting state as there are still things unresolved in this bug.

[sysrqb]

Replying to gk:

Resetting state as there are still things unresolved in this bug.

I opened #31856 for the remaining discussion about the theme. #25660 can be used for both Window and Tab. We're done with this one.

[cypherpunks]

defect implemented?

[sysrqb]

Replying to cypherpunks:

defect implemented?

Details, details. You're so picky! :)

(But, I hope we didn't implement a defect.)

I suppose this is either defect fixed if we take the position that not respecting privatebrowsing.autostart on Android was a defect. Or, this is a new feature we implemented. I'll go with the latter because it's easier than re-opening this ticket and closing it again with a different resolution.