Closed Bug 1363055 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Nika, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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?)
(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.
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?
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
(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.
Flags: needinfo?(dothayer)
See Also: → 1363410
Yeah, unfortunately I can't speak much to this - it's definitely not specific to the update dialogs.
Flags: needinfo?(dothayer)
Do we really intend to show multiple doorhangers? Shouldn't we just show 1 at a time, on the focused/topmost/last-used window?
Flags: needinfo?(dothayer)
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.
Component: General → Application Update
Product: Firefox → Toolkit
(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?
(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.
Flags: needinfo?(dothayer)
(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.
(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... :-(
Blocks: 389365
Component: Application Update → XUL
Product: Toolkit → Core
Jonathan, any chance your adventures with multiple screens and hidpi mean you might be able to help here (cf. your comments in bug 959682)? :-)
Flags: needinfo?(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.
Flags: needinfo?(kchen)
Flags: needinfo?(jfkthame)
Flags: needinfo?(VYV03354)
> 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.
Flags: needinfo?(VYV03354)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0d5492cc78ca8ee0df3e25c7a3aa133009a06ad
Could you test if this build fixes the issue?
Flags: needinfo?(michael)
(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 :)
Flags: needinfo?(michael)
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|
Attachment #8868746 - Flags: review?(kchen) → review+
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?
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/6508b37240e4
Find a nearest screen if no screen overlaps. r=kanru
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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 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 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.
https://hg.mozilla.org/mozilla-central/rev/6508b37240e4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(kchen)
You need to log in before you can comment on or make changes to this bug.