1689682 - Long tooltips open on wrong screen

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Sam Johnson]

Background: This is a 15" 2018 MacBook Pro, running macOS 11.1, with the internal monitor set to the default setting ("looks like 1680x1050"). I have two external 1920x1080 displays connected to the machine. The arrangement of displays is: internal display | external display | external display. The display configured as primary is the external display in the middle. I do not have multiple Spaces configured.

STR:

1. In a Firefox window on the MBP retina display, which is the leftmost monitor, go to a page with a very long URL and no <title> tag. See the URL field in this bug for an example URL that reproduces every time.
2. The tab title should be very long because it is just the long URL. Hover the tab until the tab tooltip appears. It will show up on the non-retina screen to the immediate right.

Expected result:
Tooltip appears on the same screen as the mouse cursor.

Actual result:
Tooltip does not appear on the correct screen at all, instead it appears on a different screen and very large.

Comment 1

[Sam Johnson]

Attachment: about:support

Comment 2

[Haik Aftandilian [:haik]]

@Sam, could you also include what you have for the setting "Displays have separate Spaces" in the Mission Control settings? Checked or unchecked.

Comment 3

[Sam Johnson]

(In reply to Haik Aftandilian [:haik] from comment #2)

@Sam, could you also include what you have for the setting "Displays have separate Spaces" in the Mission Control settings? Checked or unchecked.

It is checked.

Comment 4

[Haik Aftandilian [:haik]]

I have been able to reproduce this on Release 85 and the latest Nightly, but not 100% reliably. That is with a 3 monitor setup similar to comment 0.

Comment 5

[Haik Aftandilian [:haik]]

@Sam, I have an in-progress fix for this problem available here if you would like to test it to confirm that it addresses the problem you're hitting.

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/afI0QWfNTneslASFZjCM5Q/runs/0/artifacts/public/build/target.dmg

These builds are not signed with our production certs and as a result you'll have to launch the browser using right-click and then Open after extracting the app from the dmg.

Comment 6

[Sam Johnson]

@Haik: Looking good! I am not able to reproduce the issue in the linked build.

Comment 7

[Haik Aftandilian [:haik]]

I need some help on the layout side with this bug.

I reproduced Sam's issue using a non-Retina display attached to a MacBook Pro with Retina display turned down to the lowest resolution and arranged to the left of the non-Retina external display. And then attempting to trigger a tooltip on the left Retina display. In that case, the problem causing the tooltip to display on the wrong screen is that the minimum size of the widget mSizeConstraints is so large that it confuses FitRectToVisibleAreaForScreen() called from nsCocoaWindow::DoResize().

Specifically, when attempting to show a tooltip on the left retina display, even though the (x,y) position of the tooltip widget is within the bounds of the left screen, when the mSizeConstraints are applied in nsCocoaWindow::DoResize(), the widget ends up spanning more of the right non-retina display so FitRectToVisibleAreaForScreen() constraints the widget to that screen. I addressed this in the comment 5 patch by constraining the widget to the screen the (x, y) resides on. In this case, the nsCocoaWindow is initially created with all zero position and size values and then the resize method is called with the correct (x, y) position.

However, on another configuration I ran into a case where when the tooltip is re-displayed, the nsCocoaWindow is created with an (x,y) position putting it on the wrong screen and nsCocoaWindow::Resize() is not called before the widget is displayed. We can't fix this case in widget code because we don't know on which screen to display the widget. Unless we can infer this from the aNativeParent argument to nsCocoaWindow::Create().

Comments in the code indicate the aParent argument to nsCocoaWindow::Create() is meant to be the parent widget, but it is not set by the caller. If popups had a reliable parent widget (which is presumably the window where the right click or tooltip hover originated) widget code could use the same screen as the parent widget.

Comment 8

[Stephen A Pohl [:spohl]]

Did you mean to need-info someone? Markus might be able to lead you in the right direction here.

Comment 9

[Haik Aftandilian [:haik]]

(In reply to Stephen A Pohl [:spohl] from comment #8)

Did you mean to need-info someone? Markus might be able to lead you in the right direction here.

Thanks. I've been working with :tnikkel today on this today.

Comment 10

[Haik Aftandilian [:haik]]

After some more debugging, the root of this problem is that nsCocoaWindow::Create() interprets its aRect.{x,y} parameter as an absolute position instead of an offset from the parent windows origin causing it to sometimes place popup windows on the wrong screen causing the popup window to have an unexpected scaling factor. Specifically, for popup windows, the (x, y) component of aRect is an offset from the parent window's top left position. The ::Create() method uses the (x, y) position as absolute resulting in it sometimes choosing the wrong screen to place the window on. Later, during layout, the window is resized and on single screen configs, the resize moves the window to the appropriate place. On multiscreen configs where the screens have different scaling factors, if the window is initially placed on the wrong screen, its scaling factor will not match the scaling factor of the parent causing an incorrect minimum size constraint to be applied resulting in the logic in nsCocoaWindow::Resize() to choose a different screen again.

The new fix initially positions the popup window on the same screen as the parent window to ensure it will have the same scaling factor as the parent. As a result, its minimum size is correct and the window is not erroneously moved to the wrong window in ::Create() or ::Resize().

There is a case where the popup window should not have the same scaling factor as the parent: when the parent window spans multiple screens of different scaling factors, and the popup should be displayed on the screen showing the least of the window. That case is already completely broken and results in popups appearing on the wrong screen or wrong area of the screen. I will file a new bug to cover this problem.

Comment 11

[Haik Aftandilian [:haik]]

Attachment: Bug 1689682 - Patch 1 - Make nsCocoaWindow::Create() treat arguments correctly for parented windows r=#mac-reviewers

When creating popup windows, initialize the NSWindow with the parent window's
dimensions confined to the parent window's screen.

Comment 12

[Markus Stange [:mstange]]

Great debugging work! I'm going to pick apart your description and say what I would like to happen at each point in an "ideal" world. We don't necessarily have to make that all happen in this bug, but maybe it'll be a useful guide to find the right fix.

I would also like the type system to catch more of these bugs for us. That means choosing the right C++ type for each coordinate variable, and maybe adding new types for cases that are currently not covered. See this list in Units.h for the existing coordinate types.

(In reply to Haik Aftandilian [:haik] from comment #10)

After some more debugging, the root of this problem is that nsCocoaWindow::Create() interprets its aRect.{x,y} parameter as an absolute position instead of an offset from the parent windows origin

Can you link me to the code that expects it to be treated as an offset from the parent?

causing it to sometimes place popup windows on the wrong screen causing the popup window to have an unexpected scaling factor.

Ok... ideally, a wrong scaling factor shouldn't make a difference, but more on that later.

Specifically, for popup windows, the (x, y) component of aRect is an offset from the parent window's top left position.

Treating coordinates differently based on a separate field (here: window type) sounds fragile. Ideally, we'd have a distinction at the type level. If this were Rust, I would ask for a RelativeOrAbsoluteDesktopRect enum, with enum variants RelativeToParentWindow(DesktopIntRect) and Absolute(DesktopIntRect). But since this is C++, I'm not sure what the cleanest way to handle this is.
Alternatively, we could say "these coordinates are always absolute coordinates; if you want a relative offset to the parent, the caller needs to make that calculation".

The ::Create() method uses the (x, y) position as absolute resulting in it sometimes choosing the wrong screen to place the window on.

Ideally: Even if the window is initially placed on the wrong screen, what happens afterwards should handle this gracefully.

On multiscreen configs where the screens have different scaling factors, if the window is initially placed on the wrong screen, its scaling factor will not match the scaling factor of the parent causing an incorrect minimum size constraint to be applied

Why does the scaling factor cause an incorrect minimum size constraint? (Honest question, I haven't looked at the code.) Ideally, the size constraint should be in desktop pixels, and should be independent of the window's current scale factor.

resulting in the logic in nsCocoaWindow::Resize() to choose a different screen again.

Which logic exactly? The FitRectToVisibleAreaForScreen part?

The new fix initially positions the popup window on the same screen as the parent window to ensure it will have the same scaling factor as the parent. As a result, its minimum size is correct and the window is not erroneously moved to the wrong window in ::Create() or ::Resize().

Hmm, I'm not a huge fan of this; it seems more like a workaround for other brokenness rather than the right fix.

Comment 13

[Haik Aftandilian [:haik]]

We discussed this patch on the #macdev channel and the conclusion was we'd try to do a more thorough fix and address more of the issues in nsCocoaWindow. I'm working on that.

(In reply to Haik Aftandilian [:haik] from comment #10)

After some more debugging, the root of this problem is that nsCocoaWindow::Create() interprets its aRect.{x,y} parameter as an absolute position instead of an offset from the parent windows origin

Can you link me to the code that expects it to be treated as an offset from the parent?

This is how the nsIWidget::Create() methods are documented. See https://searchfox.org/mozilla-central/rev/362676fcadac37f9f585141a244a9a640948794a/widget/nsIWidget.h#434-485

causing it to sometimes place popup windows on the wrong screen causing the popup window to have an unexpected scaling factor.

Ok... ideally, a wrong scaling factor shouldn't make a difference, but more on that later.

Specifically, for popup windows, the (x, y) component of aRect is an offset from the parent window's top left position.

Treating coordinates differently based on a separate field (here: window type) sounds fragile. Ideally, we'd have a distinction at the type level. If this were Rust, I would ask for a RelativeOrAbsoluteDesktopRect enum, with enum variants RelativeToParentWindow(DesktopIntRect) and Absolute(DesktopIntRect). But since this is C++, I'm not sure what the cleanest way to handle this is.
Alternatively, we could say "these coordinates are always absolute coordinates; if you want a relative offset to the parent, the caller needs to make that calculation".

The right way to do this may be to rely on one of aParent or aNativeParent being non-null instead of the widget type. Per the nsIWidget documentation: In practice at least one of aParent and aNativeParent will be null. If both are null the widget isn't parented (e.g. context menus or independent top level windows).

The ::Create() method uses the (x, y) position as absolute resulting in it sometimes choosing the wrong screen to place the window on.

Ideally: Even if the window is initially placed on the wrong screen, what happens afterwards should handle this gracefully.

On multiscreen configs where the screens have different scaling factors, if the window is initially placed on the wrong screen, its scaling factor will not match the scaling factor of the parent causing an incorrect minimum size constraint to be applied

Why does the scaling factor cause an incorrect minimum size constraint? (Honest question, I haven't looked at the code.) Ideally, the size constraint should be in desktop pixels, and should be independent of the window's current scale factor.

In nsContainerFrame::SetSizeConstraints() we set the widget size constraints based on the provided nsPresContext.

resulting in the logic in nsCocoaWindow::Resize() to choose a different screen again.

Which logic exactly? The FitRectToVisibleAreaForScreen part?

Yes, when the screen argument to FitRectToVisibleAreaForScreen is null, it chooses the screen which overlaps the most with the window which is not always the right screen.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93fd52dafca2
Long tooltips open on wrong screen r=mac-reviewers,mstange

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/93fd52dafca2

Comment 16

[Haik Aftandilian [:haik]]

Requesting a backout while we debug bug 1698262.

Comment 17

[Atila Butkovits]

Backed out as requested.

Backout link: https://hg.mozilla.org/integration/autoland/rev/20f79622ed40fe9e9a722a4ae416e794518088c6

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/20f79622ed40

Comment 19

[Haik Aftandilian [:haik]]

Attachment: Bug 1689682 - Don't recreate popups

Instead of recreating popup windows, make their NSWindow's be child windows
so that they are not moved to the "Assign To" desktop. Previously, only
popups with level ePopupLevelParent used child windows. Recreating popup
windows was added as a workaorund for a problem where popups would appear on
the wrong desktop when the macOS "Assign To" feature was used, but making
the windows be be child windows is a better fix.

Depends on D106351

Comment 20

[Haik Aftandilian [:haik]]

Attachment: Bug 1689682 - Remove nsIWidget::NeedsRecreateToReshow() method

Remove nsIWidget::NeedsRecreateToReshow() method because it is no longer used.

Depends on D109263

Comment 21

[Haik Aftandilian [:haik]]

Attachment: Bug 1689682 - Include the desktop-to-device scaling factor in SizeConstraints r=#mac-reviewers

Include the scaling factor in the SizeConstraints struct so that widgets can
correctly constrain their width and height when the widget's scaling factor
differs from the nsPresContext which can happen on Mac when 1) the parent window
is moved to a new screen and the child window widget has not yet been resized
and 2) after a window is created with a zero width.

Depends on D109264

Comment 22

[Haik Aftandilian [:haik]]

Attachment: Z-level order issue

The context menu appears between the addon popup and the main window instead of on top of the addon popup.

Comment 23

[Haik Aftandilian [:haik]]

These patches aren't ready for review yet. I'm running into a z-level ordering issue (see screenshot) with these changes when opening a context menu on a popup. This problem appears to be due to the incorrect parent widget being passed to the context menu's ::Create() call in this case, but more debugging is needed.

Comment 24

[Haik Aftandilian [:haik]]

The updated patches address 2 main issues:

Issue #1: nsCocoaWindow::Create() does not interpret its aRect argument correctly.

When the ::Create() method is called with a parent widget, the aRect argument is meant to be treated as an offset from the parent widget. i.e., it specifies a rectangle that is offset by (aRect.x, aRect.y) from the parent widget origin. aRect.{width, height} are the width and height. The current code treats aRect as an absolute position which can result in the NSWindow initially being placed on the wrong screen. For example, if the main window is on a screen with origin X coordinate of -1000 with width 1000, and the aRect X value is 100, the widget X coordinate should be -900, but the bug puts the widget X at 100 which is on a different screen.

And in nsCocoaWindow::Create(… LayoutDeviceIntRect aRect, …) the aRect argument is scaled to desktop coordinates using the widget’s backing scale factor. But at this point, the NSWindow has not been created and the widget uses a hardcoded 1.0 value which is incorrect for some screens. This results in the width and height being twice as large as they should be which can cause the window to end up on the wrong screen.

—

Issue #2: nsContainerFrame::SetSizeConstraints() sets the widget size constraints using a scale factor that doesn’t match the widget’s scale factor.

nsContainerFrame::SetSizeConstraints() uses the nsPresContext’s scaling which may not match the widget’s own scaling factor causing the widget to incorrectly scale the provided values in nsCocoaWindow::SetSizeConstraints(). The scaling factors can differ because some widgets are created with a zero width and height and in that case on macOS [NSWindow backingScaleFactor] returns a specific value and not the value of the screen where the (x, y) coordinate is.

Once the widget is resized, the scale factor is corrected and the BackingScaleFactorChanged() callback is called. Specifically, the call to [mWindow setFrame] in nsCocoaWindow::DoResize() triggers the backingScaleFactor to be correct for the given non-zero dimensions. But because ::DoResize() has already applied the constraints, the window can still end up wrong screen due to the call to FitRectToVisibleAreaForScreen() in ::DoResize(). BackingScaleFactorChanged() recomputes the SizeConstraints(), but the result is incorrect because old values were not computed using the old scaling factor.

The backingScaleFactor for zero-sized windows is a known problem documented here and also reported to Apple.
https://searchfox.org/mozilla-central/rev/1758450798ae14492ba28b695f48143840ad6c5b/widget/cocoa/nsCocoaWindow.mm#1723-1732

Comment 25

[Phabricator Automation]

Comment on attachment 9205253 [details]
Bug 1689682 - Patch 1 - Make nsCocoaWindow::Create() treat arguments correctly for parented windows r=#mac-reviewers

Revision D106351 was moved to bug 1701070. Setting attachment 9205253 [details] to obsolete.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63438368ddfc
Include the desktop-to-device scaling factor in SizeConstraints r=mac-reviewers,mstange

Comment 27

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/63438368ddfc

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:haik, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Comment 31

[Catalin Sasca, Desktop QA [:csasca]]

I was able to reproduce this issue on Firefox 87.0a1 (2021-01-29) under macOS 11.4 by following the STR from Comment 0 and by using a 3-display setup.

This issue is no longer reproducible on Firefox 89.0 and 90.0a1 (2021-05-30) on the same setup.