Closed Bug 445749 Opened 13 years ago Closed 12 years ago
Ctrl+Tab panel is not centered on the right screen of a multiple monitor environment
On the right screen, the panel left border touches the screen left border. It looks like the screen.availLeft property is not correct for the right screen (its value is 0 but it should be 1280). I'll file a separate bug for this. Could someone with a dual head monitor environment on Windows or Mac report if the same issue happens there?
Are you sure screen.availLeft is wrong? I think we're actually passing correct numbers to openPopupAtScreen.
Yes, I'm pretty sure it is wrong. I'm writing a testcase for the Core bug that causes this. I guess that the panel still appears on the right screen because the window manager forces popups to appear on the same screen as the active window.
This issue is even worse when the second monitor is located above the primary one. In that case the panel is bottom-aligned and remains so when resizing (although that only applies to the All Tabs panel, cf. bug 436304 comment #41).
According to the header http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/public/nsIPopupBoxObject.idl#146 and the implementation prior to bug 393186 landing, the coordinates passed to openPopupAtScreen should be given relative to the current screen. Before bug 393186 we would get the screen from the device context for the prescontext instead of determining the screen from the passed in screen coordinates, so it didn't cause a problem. So adjust the screen coordinates from being relative to the current screen, to being in absolute screen space.
Assignee: nobody → tnikkel
Attachment #396096 - Flags: review?(enndeakin)
Make the coordinates passed to openPopupAtScreen relative to the current screen.
Attachment #396097 - Flags: review?(dao)
Comment on attachment 396097 [details] [diff] [review] fixup ctrl+tab openPopupAtScreen usage Without screen.availLeft, the popup isn't centered within the available area if the task bar is on the left of the screen. So that code needs to stay as is, I think.
Component: Tabbed Browser → General
QA Contact: tabbed.browser → general
(In reply to comment #10) > Created an attachment (id=396097) [details] > fixup ctrl+tab openPopupAtScreen usage > > Make the coordinates passed to openPopupAtScreen relative to the current > screen. Shouldn't screen.availLeft be relative to the current screen already? MDC says "If you work with two screens this property evaluated on the right screen returns width of the left one in pixels", but I'm not sure this makes sense.
(In reply to comment #12) > Shouldn't screen.availLeft be relative to the current screen already? MDC says > "If you work with two screens this property evaluated on the right screen > returns width of the left one in pixels", but I'm not sure this makes sense. I believe MDC is correct. screen.availLeft should be in the absolute screen space. In a multimonitor situation you can identify every point on every monitor with a single pair of signed ints. In the situation MDC describes the top left of the left screen is at (0,0) and the top left of the right screen is at (width of left screen,0). The two monitors combine to form one seamless surface.
This addresses the issue with a taskbar on the left.
(In reply to comment #13) > (In reply to comment #12) > I believe MDC is correct. screen.availLeft should be in the absolute screen > space. Why should it be in the absolute screen space? Why would a consumer want to deal with that? Aren't all other window.screen properties about the current screen? This documentation supports my theory that the screen.avail* properties are expected to expose which area on the current screen can be used: http://code.google.com/p/doctype/wiki/ScreenAvailLeftProperty
MDC correctly documents what Firefox currently does, and has done for a long time (at least pre FF 3.0). It is only due to bug 445765 that toplevel windows always seemed to see screen.* as relative to a monitor with top left at (0,0). I don't know how much I trust that doctype page, it doesn't mention the multimonitor situation at all.
(In reply to comment #16) > I don't know how much I trust that doctype page, it doesn't mention the > multimonitor situation at all. Yeah, but I assumed that it doesn't have to mention it, because screen.availLeft isn't about multiple monitors. The point of availLeft is that objects on the screen can reduce the available size, which MDC doesn't mention at all, so I don't have a lot of trust in that either. http://code.google.com/p/doctype/wiki/ScreenLeftProperty does mention the multiple-monitor scenario. The way screen.left and screen.availLeft are documented there makes a lot of sense to me...
Comment on attachment 396128 [details] [diff] [review] fixup ctrl+tab openPopupAtScreen usage v2 ... but since Gecko seems to include screen.left in screen.availLeft (although I can't test it), I guess this is fine. Would be nice if we had a complete documentation that backs this up, though. > var estimateHeight = this.canvasHeight * 1.25 + 75; >- this.panel.openPopupAtScreen(screen.availLeft + (screen.availWidth - this.panel.width) / 2, >- screen.availTop + (screen.availHeight - estimateHeight) / 2, >- false); >+ this.panel.openPopupAtScreen( >+ screen.availLeft + (screen.availWidth - this.panel.width) / 2 - screen.left, nit: I'd prefer (screen.availLeft - screen.left) + (screen.availWidth - this.panel.width) / 2
Attachment #396128 - Flags: review?(dao) → review+
Comment on attachment 396096 [details] [diff] [review] patch Probably a bit hard to simplify this so that the screen rectangle is only retrieved once?
Attachment #396096 - Flags: review?(enndeakin) → review+
(In reply to comment #19) > Probably a bit hard to simplify this so that the screen rectangle is only > retrieved once? Yeah, I think so, as it could be two different screens. This patch just changes some names since there was a declaration of rootScreenRect that hid the previous one, which could have been confusing.
Attachment #396096 - Attachment is obsolete: true
Fixed the one nit.
Attachment #396128 - Attachment is obsolete: true
patch v2 doesn't seem to apply. By the way, if you'd add r=... to the commit messages, your attachments could be directly imported.
(In reply to comment #22) > patch v2 doesn't seem to apply. I just refreshed the patch on my machine and it was identical. So I don't know what the problem could be. > By the way, if you'd add r=... to the commit messages, your attachments could > be directly imported. Ok, I will add the r=... to any patches I upload after they've got review+.
It's possibly that my tree was busted. In fact I just removed and re-cloned the whole thing.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(In reply to comment #25) > http://hg.mozilla.org/mozilla-central/rev/83ba2c6e25eb > http://hg.mozilla.org/mozilla-central/rev/b652e12fc7e3 backed out because of bug 514891
Whiteboard: [fixed by bug 445765]
The backout was: http://hg.mozilla.org/mozilla-central/rev/845b66a2eb91 http://hg.mozilla.org/mozilla-central/rev/fd38dbd0730b http://hg.mozilla.org/mozilla-central/rev/ce62745a7c9c http://hg.mozilla.org/mozilla-central/rev/28930fd5b4f6 Are you saying this doesn't need to be reopened because bug 445765 also fixed it, and that fix is still in?
Verified Fixed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090915 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090915052755
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.