#24920 closed enhancement (implemented)
TBA has tabs and private tabs, we only want private tabs
Reported by: | sysrqb | Owned by: | tbb-team |
---|---|---|---|
Priority: | Medium | Milestone: | |
Component: | Applications/Tor Browser | Version: | |
Severity: | Normal | Keywords: | tbb-mobile, TorBrowserTeam201909, ux-team |
Cc: | igt0, antonela | Actual Points: | |
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
Having both tabs and private tabs is confusing. We should remove all instances of non-private tabs.
Child Tickets
Attachments (1)
Change History (26)
comment:1 Changed 23 months ago by
Keywords: | tbb-mobile added |
---|
comment:2 Changed 22 months ago by
Parent ID: | → 19675 |
---|
comment:3 Changed 22 months ago by
Parent ID: | 19675 → #19675 |
---|
comment:4 Changed 22 months ago by
Parent ID: | #19675 → #5709 |
---|
comment:5 Changed 14 months ago by
Summary: | Orfox has tabs and private tabs, we only want private tabs → TBA has tabs and private tabs, we only want private tabs |
---|
comment:6 Changed 7 months ago by
Parent ID: | #5709 |
---|
comment:7 Changed 6 months ago by
Keywords: | TorBrowserTeam201905R added |
---|---|
Status: | new → needs_review |
(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
.
comment:8 Changed 6 months ago by
Keywords: | TorBrowserTeam201906R added; TorBrowserTeam201905R removed |
---|
Moving reviews over to June.
comment:9 follow-up: 10 Changed 6 months ago by
Keywords: | TorBrowserTeam201906 added; TorBrowserTeam201906R removed |
---|---|
Status: | needs_review → needs_revision |
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.
comment:10 follow-up: 11 Changed 6 months ago by
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.
comment:11 follow-up: 12 Changed 6 months ago by
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.
comment:12 Changed 6 months ago by
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.
comment:13 Changed 6 months ago by
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.
comment:14 follow-up: 15 Changed 6 months ago by
Cc: | antonela added |
---|---|
Keywords: | ux-team added |
I wonder if we need some UX input here, what are the timelines for this? 9.0 stable?
comment:15 follow-up: 16 Changed 6 months ago by
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...
comment:16 Changed 6 months ago by
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.
comment:17 follow-up: 18 Changed 6 months ago by
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.
comment:18 follow-up: 19 Changed 6 months ago by
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. JustNew Tab
seems better. Could re-nameNew Private Tab
toNew Tab
on the kebab menu?
#25660 might be relevant here. Where we had been thinking about "New Private Window" vs. "New Window".
comment:19 Changed 3 months ago by
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.
comment:20 Changed 3 months ago by
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.
comment:21 Changed 2 months ago by
Keywords: | TorBrowserTeam201909R added; TorBrowserTeam201906 removed |
---|---|
Status: | needs_revision → needs_review |
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.
comment:22 follow-up: 23 Changed 2 months ago by
Keywords: | TorBrowserTeam201909 added; TorBrowserTeam201909R removed |
---|---|
Status: | needs_review → new |
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.
comment:23 Changed 2 months ago by
Resolution: | → implemented |
---|---|
Status: | new → closed |
comment:25 Changed 2 months ago by
Type: | defect → enhancement |
---|
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.
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)