Opened 3 years ago

Closed 3 years ago

#18144 closed defect (fixed)

about:tor update arrow position is wrong (Retina and zoom)

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201602R
Cc: brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In the candidate builds of Tor Browser 5.5 and 6.0a1, the update arrow does not point to the Torbutton toolbar item on a 2x (Retina) display or when content zoom is used.

I am not sure when this regressed, but it happens because the about:tor page receives a spoofed value for window.devicePixelRatio (it is always 1.0).

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by mcs

Because window.devicePixelRatio always returns 1.0, we need to find another way to determine the correct ratio.

Kathy and I discovered that accounting for the device pixel ratio is not too difficult (e.g., if a retina Mac Book Pro is in use) since the that ratio is the same for the chrome area that contains the Torbutton toolbar item and the page itself (e.g., 2.0). However, it is much more difficult to precisely account for the zoom level (Ctrl/Cmd+/-). We thought we could just use a call like this to get the correct factor:

let zoomFactor = ZoomManager.getZoomForBrowser(gBrowser);

But the actual ratio that is used for layout is slightly different, which causes the about:tor arrow to not quite be centered below the Torbutton toolbar item at some zoom levels. This is not very noticeable when the toolbar item is in its default position near the left side of the window, and since the APIs to retrieve the correct value appear to be only accessible from C++ we we may just have to live with this small annoyance.

comment:3 Changed 3 years ago by gk

Keywords: TorBrowserTeam201601R added

comment:4 Changed 3 years ago by gk

Status: needs_reviewneeds_information

I am a bit confused by the reference to #2875 given that we started to take account for retina displays with #10640, if I understand that correctly, and the latter got much later fixed. So, the retina fix was basically non-existent since 3.5.2? Could you verify that testing such an old bundle? Or what is going on here?

comment:5 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:6 in reply to:  4 Changed 3 years ago by mcs

Status: needs_informationneeds_revision

Replying to gk:

I am a bit confused by the reference to #2875 given that we started to take account for retina displays with #10640, if I understand that correctly, and the latter got much later fixed. So, the retina fix was basically non-existent since 3.5.2? Could you verify that testing such an old bundle? Or what is going on here?

You are right; it is not #2875. Sorry for the confusion. The fix that caused this to break is the one for #13875 (TB 4.5 time frame). But ticket:13875#comment:22 provides an alternative way to get the device pixel ratio that should be more accurate than what we did for this ticket. Kathy and I will try that and if it works, we will post an improved patch.

comment:8 Changed 3 years ago by gk

Status: needs_reviewneeds_information

Looking at the diff between the first and the second patch and at the comment that changed I am wondering why

tbXpos *= window.devicePixelRatio;

slipped in. I mean if window.devicePixelRatio is alaways 1.0 for non-Chrome windows (causing the bug we have here) why do we need to multiply tbXpos with it in the first place? What am I missing?

comment:9 in reply to:  8 Changed 3 years ago by mcs

Replying to gk:

Looking at the diff between the first and the second patch and at the comment that changed I am wondering why

tbXpos *= window.devicePixelRatio;

slipped in. I mean if window.devicePixelRatio is alaways 1.0 for non-Chrome windows (causing the bug we have here) why do we need to multiply tbXpos with it in the first place? What am I missing?

Actually, window.devicePixelRatio will be 2.0 on a Retina display because 'window' is the chrome window that contains the Torbutton toolbar item. And then dividing by the screenPixelsPerCSSPixel value from the content window converts the offset into device-independent units (aka CSS pixels).

Another way to look at it: the new code is similar to the old code except we removed the "if ("devicePixelRatio" in window)" checks (FF 18 was a long time ago now) and instead of dividing by devicePixelRatio inside aboutTor.xhtml (the content window) we get the screenPixelsPerCSSPixel value inside torbutton.js and do the division there. This is necessary because screenPixelsPerCSSPixel is only available from privileged code (e.g., inside the Torbutton overlay).

comment:10 Changed 3 years ago by gk

Resolution: fixed
Status: needs_informationclosed

Thanks, this got applied to master (b195514c21e5809f9afc8ec606e716d0e67865cc) and maint-1.9.4 (3ccbfe38c767fb5d85ea37168ae08c297997b5ac).

Note: See TracTickets for help on using tickets.