#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)
Change History (32)
comment:1 Changed 7 weeks ago by
Keywords: | tbb-9.0-issues tbb-9.0.1-can added |
---|---|
Priority: | High → Medium |
Severity: | Major → Normal |
comment:2 Changed 7 weeks ago by
comment:3 Changed 7 weeks ago by
Keywords: | ux-team added |
---|
comment:4 Changed 7 weeks ago by
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 follow-up: 6 Changed 7 weeks ago by
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 Changed 7 weeks ago by
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
Changed 7 weeks ago by
Attachment: | Captura de pantalla 2018-08-15 a la(s) 3.13.54 PM.png added |
---|
Changed 7 weeks ago by
Attachment: | Captura de pantalla 2018-08-15 a la(s) 3.14.18 PM.png added |
---|
Changed 7 weeks ago by
Attachment: | Captura de pantalla 2018-08-15 a la(s) 3.16.11 PM.png added |
---|
Changed 7 weeks ago by
Attachment: | Captura de pantalla 2018-08-15 a la(s) 3.15.05 PM.png added |
---|
comment:7 Changed 7 weeks ago by
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 follow-up: 9 Changed 7 weeks ago by
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 Changed 7 weeks ago by
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
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
Or worse they don't know such a preference exists and keep using an older Tor Browser version.
comment:12 Changed 6 weeks ago by
Keywords: | TorBrowserTeam201910R added |
---|---|
Status: | new → needs_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
Default Theme, letterboxed page
Dark Theme, no letterbox
Dark Theme, letterboxed
comment:13 Changed 6 weeks ago by
Keywords: | TorBrowserTeam201910 added; TorBrowserTeam201910R removed |
---|---|
Status: | needs_review → needs_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
Keywords: | TorBrowserTeam201911 added; TorBrowserTeam201910 removed |
---|
Moving tickets to November 2019
comment:15 Changed 5 weeks ago by
Points: | → 2 |
---|
Changed 5 weeks ago by
Attachment: | abouttor letterboxed.png added |
---|
Changed 5 weeks ago by
Attachment: | abouttor none.png added |
---|
comment:16 follow-up: 17 Changed 5 weeks ago by
Actual Points: | → 5 |
---|---|
Keywords: | TorBrowserTeam201911R added; TorBrowserTeam201911 removed |
Status: | needs_revision → needs_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.
comment:17 Changed 5 weeks ago by
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 follow-up: 20 Changed 5 weeks ago by
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.
comment:20 follow-up: 21 Changed 5 weeks ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
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 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.
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?
comment:21 Changed 5 weeks ago by
Keywords: | tbb-backport added |
---|
comment:22 Changed 2 weeks ago by
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.
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