Moving these to legacy/trac#5709 (moved), 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)
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).
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 6d81ef66, 1666b018, 1bce86ec, and 41b5dac4).
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.
Trac: Status: needs_review to needs_revision Keywords: TorBrowserTeam201906R deleted, TorBrowserTeam201906 added
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.
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 6d81ef66, 1666b018, 1bce86ec, and 41b5dac4).
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.
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.
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.
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.
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 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.
+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 legacy/trac#10399 (moved). 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.
+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 legacy/trac#10399 (moved). 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?
legacy/trac#25660 (moved) might be relevant here. Where we had been thinking about "New Private Window" vs. "New Window".
legacy/trac#25660 (moved) 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 legacy/trac#25660 (moved), 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.
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.