If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 55

Status

()

Core
XUL
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: mystor, Assigned: emk)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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?)
(Reporter)

Comment 1

4 months ago
(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)

Comment 3

4 months ago
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)
(Reporter)

Comment 4

4 months ago
(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)
(Reporter)

Updated

4 months ago
See Also: → bug 1363410
Yeah, unfortunately I can't speak much to this - it's definitely not specific to the update dialogs.
Flags: needinfo?(dothayer)

Comment 6

4 months ago
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)

Comment 7

4 months ago
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
(Reporter)

Comment 8

4 months ago
(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)
(Reporter)

Comment 10

4 months ago
(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

4 months ago
(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

Comment 12

4 months ago
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)
(Assignee)

Comment 14

4 months ago
> 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)
(Assignee)

Comment 15

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0d5492cc78ca8ee0df3e25c7a3aa133009a06ad
Could you test if this build fixes the issue?
Flags: needinfo?(michael)
(Reporter)

Comment 16

4 months ago
(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 hidden (mozreview-request)

Comment 18

4 months ago
mozreview-review
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+
(Assignee)

Comment 19

4 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 21

4 months ago
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)

Updated

4 months ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED

Comment 22

4 months ago
mozreview-review-reply
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. :-/
(Assignee)

Comment 23

4 months ago
mozreview-review-reply
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

4 months ago
mozreview-review-reply
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6508b37240e4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(kchen)
You need to log in before you can comment on or make changes to this bug.