Change letterboxing color when dark theme is enabled
Change letterboxing color when dark theme is enabled
It is very white and should be changed to a dark color
Designs
Activity
added TorBrowserTeam201911R in Legacy / Trac actualpoints::5 in Legacy / Trac component::applications/tor browser in Legacy / Trac owner::tbb-team in Legacy / Trac points::2 in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tbb-9.0-issues in Legacy / Trac tbb-9.0.1-can in Legacy / Trac type::defect in Legacy / Trac ux-team in Legacy / Trac labels
For a work-around, see https://trac.torproject.org/projects/tor/ticket/30812#comment:3
However, it's not something that is high priority upstream, ask Tom
Should obviously be changed upstream.
However if anyone decides to do this in Tor browser then I suggest that you also add some kind of soft pattern to the letterboxing space to make the separation from the page content clear at a glance. Right now it's hard to distinguish on pages that use white background, so on most pages.
The pattern could be even something Tor-related, such as little onion watermarks. This will also more clearly signal to users that this space around the page content is very much intentional and is part of Tor browser's mission to fight against fingerprinting. Right now there's a lot of users confused by the letterboxing, mistaking it for a bug.
When I tried doing this before, it seemed like it was going to be really difficult. But as we've refactored a bunch of XBL stuff I think it might have gotten easier.
If use the browser toolbox to select the with class 'browserStack' that has the window, and manipulate that element's bgcolor - that's what I would try to get working but I can't dig into this for the next several months.
-
Replying to tom:
... 'browserStack' that has the window, and manipulate that element's bgcolor ...
Ugh. https://github.com/ghacksuserjs/ghacks-user.js/issues/747#issuecomment-518841908 .. I would use #tabbrowser-tabpanels
-
Replying to cypherpunks:
The border works well and would be improve the situation in the default theme as well.
Agreed, as long as it doesn't impact the sizing, which it might (I have not confirmed this -> https://github.com/ghacksuserjs/ghacks-user.js/issues/747#issuecomment-503784101 ).
I personally wouldn't want any big difference in coloring: just a slightly darker border. Not bright purple :)
A pattern would be too jarring and clash with the page content. The separation should ideally be very minimalist yet very noticeable.
my 2cents: totally prefer a solid color: clean/efficient
This and other features relating to letterboxing should be made a bigger priority. Most people don't know what letterboxing is and many think it is a bug introduced in the new browser version. The current letterboxing affects the browsing experience more than you would expect and will make users search for the preference to turn off the feature completely which is not a good outcome.
Ok, I spent today working out an improved UX with letterboxing and present the following prototypes.
This change gives the appearance of extending down the browser chrome to the content area, and the content itself being nested within it. A border is added that is the same color as the border currently used to separate the toolbars and the content area. It's currently 1px wide to match the rest of the chrome borders, but it can be tweaked.
The patch to do this unfortunately does require a bit of fiddling with the actual fingerprinting code. The letterbox size calculation needed to be updated to take border width into account (and can be further tweaked to also take into account margin if we wish). I also discovered (and hopefully fixed/avoided) a neat little race condition that would result in over-vigorous letter-boxing when opening a new-tab.
tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220
== Default Theme, builtin page 
== Default Theme, letterboxed page 
== Dark Theme, no letterbox 
== Dark Theme, letterboxed 
Trac:
Status: new to needs_review
Keywords: N/A deleted, TorBrowserTeam201910R added
I like the idea, thanks! Two things I realized while testing which should get fixed:
-
Suddenly letterboxing is applied even to my default screen size which is properly rounded. That should not happen. Users should only see letterboxing in effect if the windows is a) not properly rounded on start-up (which is happening without your patch on my system) or b) getting resized.
-
For some reason the color next to the scrollbar is still the old one on the
about:torpage (that is white as before) in non-dark mode while all the other parts do get the chrome color. Can you see that as well on your system?
It makes me a bit nervous to see the CSS changed unconditionally while Letterboxing itself is bound to a pref. It seems to work as far as I can tell but we should think harder about an upstreamable solution as I suspect Mozilla would not take the approach you have chosen.
Trac:
Keywords: TorBrowserTeam201910R deleted, TorBrowserTeam201910 added
Status: needs_review to needs_revision-
-
gk: Letterboxing now only occurs when necessary and there are no more unconditional CSS changes (each rule is now behind a 'letterbox' class). I haven't seen the scrollbar color issue you've described with the most recent patch.
tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2
The browser content is now anchored to the toolbar, but the toolbar's horizontal border must remain. If we end up not liking that horizontal line, we can go further in and modify the global styles a bit.
Trac:
Status: needs_revision to needs_review
Actualpoints: N/A to 5
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added -
Replying to pospeselr:
gk: Letterboxing now only occurs when necessary and there are no more unconditional CSS changes (each rule is now behind a 'letterbox' class). I haven't seen the scrollbar color issue you've described with the most recent patch.
I am inclined to think that's been a local glitch in my setup. Sorry for the noise.
Regarding unconditional CSS changes
+ background-color: var(--toolbar-bgcolor); + background-image: var(--toolbar-bgimage);seem to me still not letterbox dependent. However, I could think Mozilla picking up that change, so I am fine leaving it.
tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2
The browser content is now anchored to the toolbar, but the toolbar's horizontal border must remain. If we end up not liking that horizontal line, we can go further in and modify the global styles a bit.
Thanks! Nice work. I am not sure whether we like that but let's give it a try. I tested the code on Linux, Windows, and macOS and it is working for me as expected.
Some code questions:
// One cannot (easily) control the color of a margin unfortunately. // An initial attempt to use a border instead of a margin resulted // in offset event dispatching; so for now we use a colorless margin. - aBrowser.style.margin = `${margins.height}px ${margins.width}px`; + aBrowser.classList.add("letterboxing");- Is that commit still accurate? I mean we do now deal with the margin color thanks to your patch.
- Why are you calling
aBrowser.classList.add("letterboxing");here and in other places after you calledthis._resolveBorderDimensions(aBrowser);which already addsletterboxing? Wouldn't it be enough to just do it once? If not could you add a comment as to why this is needed (again)? Maybe that's for the case that someone is flipping the pref in the browser without reloading a page or restarting the browser?
-
Ok I've updated the patch with a fixup commit which makes the tabpanel color change conditional to letterboxing being enabled, and I've removed the redundant addition of "letterboxing" to the browser element class list and am now only doing it on one place.
And here's a followup/optional commit which adds about:tor to the set of pages that do not need letterboxing (like about:blank). It's our built-in page, so I don't see any reason why it needs to be letterboxed, but I left it out of the fixup commit just in case we do want it letterboxed.
-
Replying to pospeselr:
Ok I've updated the patch with a fixup commit which makes the tabpanel color change conditional to letterboxing being enabled, and I've removed the redundant addition of "letterboxing" to the browser element class list and am now only doing it on one place.
Thanks this looks good now. The fixup idea is good (and confused me at first because I was not used to it from you ;) ) as it makes reviewing eas(y)(ier). I squashed it (please do that the next time as this saves some time on the reviewer's/merger's side) and applied it to
tor-browser-68.2.0esr-9.5-1(commit ff808390).And here's a followup/optional commit which adds about:tor to the set of pages that do not need letterboxing (like about:blank). It's our built-in page, so I don't see any reason why it needs to be letterboxed, but I left it out of the fixup commit just in case we do want it letterboxed.
I am a bit reluctant here and think we should discussing how we want to deal with privileged pages and
about:torin particular. Here is my concern I had:18:46 <+GeKo> for the about:tor page exemption 18:47 <+GeKo> i had been thinking about mentioning that in my review 18:47 <+GeKo> but then thought it might be confusing to users when the window is suddenly starting "to do" things 18:47 <+GeKo> while they just tried to visit a page 18:48 <+GeKo> users don't have the concept of priviledged vs. non-priviledged pagespospeselr: could you open a ticket for that discussion including your idea to generally apply letterboxing to privileged sites, too?
Trac:
Status: needs_review to closed
Resolution: N/A to fixed -
Replying to gk:
[snip]
pospeselr: could you open a ticket for that discussion including your idea to generally apply letterboxing to privileged sites, too?
legacy/trac#32411 (moved) it is.
Marking the fix for a possible backport.
Trac:
Keywords: N/A deleted, tbb-backport added 
Just wanted to mention here that setting
ui.systemUsesDarkTheme: 1andbrowser.in-content.dark-mode: trueinabout:configwill change the latest Tor Browser to use a dark letterbox border (as well as dark Preferences page, Add-ons page, etc). Since these settings may also enable the CSS prefers-color-scheme feature, these settings may make you fingerprintable.
-
Replying to opara:
Since these settings may also enable the CSS prefers-color-scheme feature, these settings may make you fingerprintable.
RFP covers prefers-color-scheme in FF67+ , so we are good here: https://bugzilla.mozilla.org/show_bug.cgi?id=1540726
mentioned in issue legacy/trac#32308 (moved)
moved from legacy/trac#32220 (moved)
mentioned in issue #40767 (closed)
mentioned in merge request !242 (closed)
