Opened 7 weeks ago

Closed 5 weeks ago

Last modified 2 weeks ago

#32220 closed defect (fixed)

Change letterboxing color when dark theme is enabled

Reported by: cypherpunks Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-9.0-issues, tbb-9.0.1-can, ux-team, TorBrowserTeam201911R, tbb-backport
Cc: Actual Points: 5
Parent ID: Points: 2
Reviewer: Sponsor:

Description

Change letterboxing color when dark theme is enabled

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

Child Tickets

Attachments (10)

Captura de pantalla 2018-08-15 a la(s) 3.13.54 PM.png (1.2 MB) - added by antonela 7 weeks ago.
Captura de pantalla 2018-08-15 a la(s) 3.14.18 PM.png (1.2 MB) - added by antonela 7 weeks ago.
Captura de pantalla 2018-08-15 a la(s) 3.16.11 PM.png (1.2 MB) - added by antonela 7 weeks ago.
Captura de pantalla 2018-08-15 a la(s) 3.15.05 PM.png (1.2 MB) - added by antonela 7 weeks ago.
default.png (193.8 KB) - added by pospeselr 6 weeks ago.
default theme no letterboxing
default-letterbox.png (163.3 KB) - added by pospeselr 6 weeks ago.
default theme with letterboxing
dark.png (190.0 KB) - added by pospeselr 6 weeks ago.
dark theme no letterboxing
dark-letterbox.png (2.0 MB) - added by pospeselr 6 weeks ago.
dark theme with letterboxing
abouttor letterboxed.png (318.5 KB) - added by pospeselr 5 weeks ago.
abouttor none.png (320.6 KB) - added by pospeselr 5 weeks ago.

Change History (32)

comment:1 Changed 7 weeks ago by gk

Keywords: tbb-9.0-issues tbb-9.0.1-can added
Priority: HighMedium
Severity: MajorNormal

comment:2 Changed 7 weeks ago by 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

comment:3 Changed 7 weeks ago by antonela

Keywords: ux-team added

comment:4 Changed 7 weeks ago by 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.

comment:5 Changed 7 weeks ago by 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.

comment:6 in reply to:  5 Changed 7 weeks ago by 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

comment:7 Changed 7 weeks ago by 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.

comment:8 Changed 7 weeks ago by 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.

comment:9 in reply to:  8 Changed 7 weeks ago by 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

comment:10 Changed 7 weeks ago by 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.

comment:11 Changed 7 weeks ago by cypherpunks

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

Changed 6 weeks ago by pospeselr

Attachment: default.png added

default theme no letterboxing

Changed 6 weeks ago by pospeselr

Attachment: default-letterbox.png added

default theme with letterboxing

Changed 6 weeks ago by pospeselr

Attachment: dark.png added

dark theme no letterboxing

Changed 6 weeks ago by pospeselr

Attachment: dark-letterbox.png added

dark theme with letterboxing

comment:12 Changed 6 weeks ago by pospeselr

Keywords: TorBrowserTeam201910R added
Status: newneeds_review

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

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/default.png

Default Theme, letterboxed page

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/default-letterbox.png

Dark Theme, no letterbox

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/dark.png

Dark Theme, letterboxed

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/dark-letterbox.png

comment:13 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

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.

comment:14 Changed 5 weeks ago by pili

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201910 removed

Moving tickets to November 2019

comment:15 Changed 5 weeks ago by pili

Points: 2

Changed 5 weeks ago by pospeselr

Attachment: abouttor letterboxed.png added

Changed 5 weeks ago by pospeselr

Attachment: abouttor none.png added

comment:16 Changed 5 weeks ago by pospeselr

Actual Points: 5
Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

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.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/abouttor%20letterboxed.png

https://trac.torproject.org/projects/tor/raw-attachment/ticket/32220/abouttor%20none.png

comment:17 in reply to:  16 Changed 5 weeks ago by 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?

comment:19 Changed 5 weeks ago by 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

Last edited 5 weeks ago by pospeselr (previous) (diff)

comment:20 in reply to:  19 ; Changed 5 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

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?

Last edited 5 weeks ago by gk (previous) (diff)

comment:21 in reply to:  20 Changed 5 weeks ago by gk

Keywords: tbb-backport added

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.

comment:22 Changed 2 weeks ago by 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.

Note: See TracTickets for help on using tickets.