Opened 13 days ago

Closed 11 days ago

Last modified 6 days ago

#31598 closed defect (fixed)

Properly enable letterboxing (again)

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201909R, tbb-9.0-must-alpha
Cc: Actual Points: 0.2
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

It seems while rebasing in #30429 we did not realize that the letterboxing patches made it into ESR 68 yet the pref to enabled the functionality needs still to get set.

Child Tickets

Change History (10)

comment:1 Changed 13 days ago by gk

Oh, and while we are at it: we should disable the notification bar in Torbutton that complains when the user tries to maximize their window.

comment:2 Changed 13 days ago by gk

Summary: Properly enabled letterboxing (again)Properly enable letterboxing (again)

comment:4 in reply to:  3 Changed 12 days ago by gk

Status: needs_reviewneeds_revision

Replying to acat:

Patches in https://github.com/acatarineu/tor-browser/commit/31598 and https://github.com/acatarineu/torbutton/commit/31598.

I think we want to bind the behavior to the letterboxing pref maybe? That is: if letterboxing is enabled don't show any notification if resizing is happening (but make sure that our window is still properly rounded when created) and if letterboxing is disabled (because folks don't like the new behavior) fall back to the current status quo)

comment:5 Changed 12 days ago by acat

Status: needs_revisionneeds_review

Yes, I think that's a good idea. Revised patch: https://github.com/acatarineu/torbutton/commit/31598+1

comment:6 Changed 12 days ago by gk

Status: needs_reviewneeds_revision

Hm, so this leaves is in a weird situation, kind of: disabling/enabling letterboxing happens instantly. However, showing/not showing the notification box depends on the window creation state, which gives surprising results. You'd e.g. get the notification bar if you just enabled letterboxing but you started without it being on and vice versa. I think we should find a better way binding letterboxing to the notification bar.

If that's to tricky I am fine just disabling that part entirely. However, in that case we could just rip the resizelistener etc. out.

comment:7 Changed 12 days ago by acat

Mmm, actually I thought about that, but not sure how I tested that I thought letterboxing was not being enabled instantly, just on a new window.

In any case, here is a revised patch: https://github.com/acatarineu/torbutton/commit/31598+2. Now the resize listener is always enabled and checking the pref when it's called. Being strict, this still has the issue that a user with the window already maximized changing letterboxing pref will not get any notification, since these are only shown on window resize right now. Do you think we should also handle those cases?

comment:8 Changed 12 days ago by acat

Status: needs_revisionneeds_review

comment:9 in reply to:  7 Changed 11 days ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Resolution: fixed
Status: needs_reviewclosed

Replying to acat:

Mmm, actually I thought about that, but not sure how I tested that I thought letterboxing was not being enabled instantly, just on a new window.

In any case, here is a revised patch: https://github.com/acatarineu/torbutton/commit/31598+2. Now the resize listener is always enabled and checking the pref when it's called. Being strict, this still has the issue that a user with the window already maximized changing letterboxing pref will not get any notification, since these are only shown on window resize right now. Do you think we should also handle those cases?

No, I think what we have now is fine. I merged your patches to torbutton (commit 1edfec57049a76052fda8eb17b6e3b8f281e3361) and tor-browser (commit f19b6946fcccb42b3b060c5ecf53ba8ec524b420), thanks!

comment:10 Changed 6 days ago by acat

Actual Points: 0.2
Note: See TracTickets for help on using tickets.