Change letterboxing color when dark theme is enabled

[cypherpunks]

Change letterboxing color when dark theme is enabled

It is very white and should be changed to a dark color

[Thorin]

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

[cypherpunks]

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.

[tom]

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 <stack> 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.

[Thorin]

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

[antonela]

I made some mockups in the past. I hope they are helpful now. Check the attachments.

+1 on having a dark background on dark themes.

-1 on having an onion pattern.

+1 on explaining to users why this feature is useful and allow them to enable/disable easy.

[cypherpunks]

The border works well and would be improve the situation in the default theme as well.

A pattern would be too jarring and clash with the page content. The separation should ideally be very minimalist yet very noticeable.

[Thorin]

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

[cypherpunks]

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.

[cypherpunks]

Or worse they don't know such a preference exists and keep using an older Tor Browser version.

[pospeselr]

default theme no letterboxing

[pospeselr]

default theme with letterboxing

[pospeselr]

dark theme no letterboxing

[pospeselr]

dark theme with letterboxing

[pospeselr]

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

[gk]

I like the idea, thanks! Two things I realized while testing which should get fixed:

1) 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.

2) For some reason the color next to the scrollbar is still the old one on the about:tor page (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.

[pili]

Moving tickets to November 2019

[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.

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.

[gk]

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");

1) Is that commit still accurate? I mean we *do* now deal with the margin color thanks to your patch.
2) Why are you calling aBrowser.classList.add("letterboxing"); here and in other places *after* you called this._resolveBorderDimensions(aBrowser); which already adds letterboxing? 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?

[tom]

​https://bugzilla.mozilla.org/show_bug.cgi?id=1594455

[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.

tor-browser: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2&id=b0bd271086e6d727d74d63dddaf13cc73f456ecb

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.

tor-browser: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2&id=733e4014dc0a10c375c70070ec7ee9f468ea73e2

[gk]

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.

tor-browser: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2&id=b0bd271086e6d727d74d63dddaf13cc73f456ecb

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 ff8083901a19421e9a3f0dba5346bd6873fee956).

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.

tor-browser: ​https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_32220_v2&id=733e4014dc0a10c375c70070ec7ee9f468ea73e2

I am a bit reluctant here and think we should discussing how we want to deal with privileged pages and about:tor in 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 pages

pospeselr: could you open a ticket for that discussion including your idea to generally apply letterboxing to privileged sites, too?

[gk]

Replying to gk:

[snip]

pospeselr: could you open a ticket for that discussion including your idea to generally apply letterboxing to privileged sites, too?

#32411 it is.

Marking the fix for a possible backport.

[gk]

This change is a large one so that letting it bake another 4 weeks does not sound unreasonable. In particular as the fix has only seen testing two weeks in the alpha series so far. Additionally, we might be able to pick up the fix for 1594455 directly that way.