1363055 - Placement of the new "Restart to update Nightly" dialogs are very buggy on linux

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Attachment: Screenshot from 2017-05-08 11-29-02.png

See attached screenshot. The Restart to update Nightly box above the window does not appear to have a window appears to be associated with a window on a different monitor which is placed slightly off screen (looks like some sort of incorrect default or overflow issue?)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

(In reply to Michael Layzell [:mystor] from comment #0)
> Created attachment 8865473 [details]
> Screenshot from 2017-05-08 11-29-02.png
> 
> See attached screenshot. The Restart to update Nightly box above the window
> does not appear to have a window appears to be associated with a window on a
> different monitor which is placed slightly off screen (looks like some sort
> of incorrect default or overflow issue?)

Well, that comment was incoherent. Let's try again.

The Restart to update Nightly box at the top appears to be associated with a window on a different monitor which is placed slightly off screen (looks like some sort of incorrect default or overflow issue?).

I have three monitors, this issue is on the left-most monitor, and the window which the top box seems to be associated with is on the rightmost monitor and about 100px of the window is off of the right of the screen.

The smaller box occurred after un-maximising the window.

Comment 2

[Johann Hofmann [:johannh]]

This is likely not the fault of that specific dialog but rather a general doorhanger problem. Neil, do you know more about this, is this a dupe by any chance?

Comment 3

[Neil Deakin]

The image implies the panel appears multiple times. I'm not sure if that is intentional.

After bug 1109868, the panels should update their positions to the location of whatever they are anchored to. It is a bit unclear from the description without knowing more specifics how to reproduce this.

Doug Thayer just worked on the update dialogs; you might ask him as well.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Neil Deakin from comment #3)
> The image implies the panel appears multiple times. I'm not sure if that is
> intentional.

The panel appears attached to every open browser window. I think that is intentional.

I have reliable STR for the floating doorhanger part of this bug now:

1. Run linux (I'm on Ubuntu 16.10) on a multiple monitor setup
2. Open a Firefox window, and move it to the rightmost side of the rightmost screen, so that the pancake menu is almost completely off of the screen, but still clickable.
3. Click the pancake menu. The doorhanger will open on the leftmost screen.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Yeah, unfortunately I can't speak much to this - it's definitely not specific to the update dialogs.

Comment 6

[:Gijs (he/him)]

Do we really intend to show multiple doorhangers? Shouldn't we just show 1 at a time, on the focused/topmost/last-used window?

Comment 7

[:Gijs (he/him)]

Moving over to update for now - if this is a generic platform panel problem, it should move to the appropriate component for that, but Fx::General is a dumping ground so let's not put it there.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs from comment #7)
> Moving over to update for now - if this is a generic platform panel problem,
> it should move to the appropriate component for that, but Fx::General is a
> dumping ground so let's not put it there.

This is a generic doorhanger problem on Linux as far as I know, so perhaps this should be moved to Widget: Gtk?

Comment 9

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs from comment #6)
> Do we really intend to show multiple doorhangers? Shouldn't we just show 1
> at a time, on the focused/topmost/last-used window?

Update doorhangers should only _show_ when a window is focused, but they won't hide when the window is unfocused if they were already shown. Our goal was for them to be more unobtrusive, and animating in and out whenever you change focus didn't seem to advance that goal.

In any case the change to actually make it behave this way only landed a few days ago with Bug 1358363, so before then it would be much more common to see doorhangers on multiple windows at the same time.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Doug Thayer [:dthayer] from comment #9)
> (In reply to :Gijs from comment #6)
> > Do we really intend to show multiple doorhangers? Shouldn't we just show 1
> > at a time, on the focused/topmost/last-used window?
> 
> Update doorhangers should only _show_ when a window is focused, but they
> won't hide when the window is unfocused if they were already shown. Our goal
> was for them to be more unobtrusive, and animating in and out whenever you
> change focus didn't seem to advance that goal.
> 
> In any case the change to actually make it behave this way only landed a few
> days ago with Bug 1358363, so before then it would be much more common to
> see doorhangers on multiple windows at the same time.

This will probably at least sidestep a lot of the problems with the doorhanger, such as the problems with it appearing above other windows. However, it still doesn't fix the main issue here, which is a problem with all doorhangers on linux as far as I can tell, which is that when the anchor point for the doorhanger is off of the right of the rightmost screen in a multiple monitor setup, we draw it on the leftmost monitor, despite that being the wrong location.

Comment 11

[:Gijs (he/him)]

(In reply to Michael Layzell [:mystor] from comment #10)
> (In reply to Doug Thayer [:dthayer] from comment #9)
> > (In reply to :Gijs from comment #6)
> > > Do we really intend to show multiple doorhangers? Shouldn't we just show 1
> > > at a time, on the focused/topmost/last-used window?
> > 
> > Update doorhangers should only _show_ when a window is focused, but they
> > won't hide when the window is unfocused if they were already shown. Our goal
> > was for them to be more unobtrusive, and animating in and out whenever you
> > change focus didn't seem to advance that goal.
> > 
> > In any case the change to actually make it behave this way only landed a few
> > days ago with Bug 1358363, so before then it would be much more common to
> > see doorhangers on multiple windows at the same time.
> 
> This will probably at least sidestep a lot of the problems with the
> doorhanger, such as the problems with it appearing above other windows.
> However, it still doesn't fix the main issue here, which is a problem with
> all doorhangers on linux as far as I can tell, which is that when the anchor
> point for the doorhanger is off of the right of the rightmost screen in a
> multiple monitor setup, we draw it on the leftmost monitor, despite that
> being the wrong location.

This seems like it's basically bug 389365, which judging by the bug number, might be tricky to fix. I don't have the setup to even reproduce this (don't run a Linux desktop outside a VM), never mind time to work out why it's broken in cpp code I'm not familiar with. Neil Deakin would normally be your best bet but he's out until August, AIUI. So to get traction step 0 would be finding someone who has a setup to reproduce this and time to fix it... :-(

Comment 12

[:Gijs (he/him)]

Jonathan, any chance your adventures with multiple screens and hidpi mean you might be able to help here (cf. your comments in bug 959682)? :-)

Comment 13

[Jonathan Kew [:jfkthame]]

I'm afraid I am fairly swamped in layout-land at the moment, and haven't looked at multiscreen stuff in a while. Offhand it sounds like when the anchor point is off-screen, we fall back to using the first/default/main screen instead of looking for the screen nearest to the desired position.

Might be worth asking :emk for any insights, or :kanru who did some recent refactoring of widget/screen code.

Comment 14

[Masatoshi Kimura [:emk]]

> we fall back to using the first/default/main screen instead of looking for the screen nearest to the desired position.

I think so, too. In Windows terms, nsIScreenManager.screenForRect behaves more like MONITOR_DEFAULTTOPRIMARY than MONITOR_DEFAULTTONEAREST. We should improve the algorithm to determine the most appropriate screen.

Comment 15

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0d5492cc78ca8ee0df3e25c7a3aa133009a06ad
Could you test if this build fixes the issue?

Comment 16

[Nika Layzell [:nika] (ni? for response)]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e0d5492cc78ca8ee0df3e25c7a3aa133009a06ad
> Could you test if this build fixes the issue?

Yes, this fixes the issue with the popup appearing on the wrong monitor for me! Thanks :)

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Bug 1363055 - Find a nearest screen if no screen overlaps.

Review commit: https://reviewboard.mozilla.org/r/140346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140346/

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

https://reviewboard.mozilla.org/r/140346/#review143732

::: widget/ScreenManager.cpp:164
(Diff revision 1)
> +  // If the window intersects one or more screen,
> +  // return the screen that has the largest intersection.

If the *rect* intersects [...]

::: widget/ScreenManager.cpp:172
(Diff revision 1)
> +  // If the window does not intersect a screen, find
> +  // a screen that is nearest to the window.

Ditto

::: widget/ScreenManager.cpp:180
(Diff revision 1)
> +    uint32_t distanceX = 0;
> +    if (aX > (x + width)) {
> +      distanceX = aX - (x + width);
> +    } else if ((aX + aWidth) < x) {
> +      distanceX = x - (aX + aWidth);
> +    }

Maybe put a debug warning when distanceX is not zero

::: widget/ScreenManager.cpp:187
(Diff revision 1)
> +    uint32_t distanceY = 0;
> +    if (aY > (y + height)) {
> +      distanceY = aY - (y + height);
> +    } else if ((aY + aHeight) < y) {
> +      distanceY = y - (aY + aHeight);
> +    }

Same for distanceY

::: widget/ScreenManager.cpp:198
(Diff revision 1)
> +      if (distance <= 0) {
> +        break;
> +      }

distance is a uint32_t so we only need to check if |distance == 0|

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

https://reviewboard.mozilla.org/r/140346/#review143732

> Maybe put a debug warning when distanceX is not zero

Why?

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140346/diff/1-2/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6508b37240e4
Find a nearest screen if no screen overlaps. r=kanru

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

https://reviewboard.mozilla.org/r/140346/#review143732

> Why?

Sorry. I meant when distanceX is zero. :-/

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

https://reviewboard.mozilla.org/r/140346/#review143732

> Sorry. I meant when distanceX is zero. :-/

distanceX/Y will be zero when a rect edge touches (but does not intersect) a screen edge. So it is normal.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8868746 [details]
Bug 1363055 - Find a nearest screen if no screen overlaps.

https://reviewboard.mozilla.org/r/140346/#review143732

> distanceX/Y will be zero when a rect edge touches (but does not intersect) a screen edge. So it is normal.

OK, this is fine then.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6508b37240e4