Closed
Bug 1363055
Opened 8 years ago
Closed 7 years ago
Placement of the new "Restart to update Nightly" dialogs are very buggy on linux
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: nika, Assigned: emk)
References
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?)
Reporter | ||
Comment 1•8 years 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.
Comment 2•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
Comment 5•8 years ago
|
||
Yeah, unfortunately I can't speak much to this - it's definitely not specific to the update dialogs.
Flags: needinfo?(dothayer)
Comment 6•8 years 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•8 years 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•8 years 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?
Comment 9•8 years ago
|
||
(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•8 years 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•8 years 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... :-(
Comment 12•8 years 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)
Comment 13•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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•8 years 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•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment 22•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Flags: needinfo?(kchen)
You need to log in
before you can comment on or make changes to this bug.
Description
•