[tbb-bugs] #32220 [Applications/Tor Browser]: Change letterboxing color when dark theme is enabled

[Tor Bug Tracker & Wiki]

#32220: Change letterboxing color when dark theme is enabled
-------------------------------------------------+-------------------------
 Reporter:  cypherpunks                          |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-9.0-issues, tbb-9.0.1-can, ux-   |  Actual Points:  5
  team, TorBrowserTeam201911R                    |
Parent ID:                                       |         Points:  2
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by gk):

 Replying to [comment:16 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?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32220#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online