Ctrl+Tab panel is not centered on the right screen of a multiple monitor environment

VERIFIED FIXED in Firefox 3.7a1

Status

()

defect
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: sylvain.pasche, Assigned: tnikkel)

Tracking

(Blocks 1 bug)

Trunk
Firefox 3.7a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed by bug 445765], )

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

11 years ago
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.
Reporter

Comment 2

11 years ago
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.
Reporter

Updated

11 years ago
Depends on: 445765

Updated

11 years ago
Duplicate of this bug: 445961
OS: Linux → All
Hardware: PC → All

Updated

11 years ago
Duplicate of this bug: 449024

Comment 5

11 years ago
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).

Updated

11 years ago
Duplicate of this bug: 472107
Duplicate of this bug: 501562
Duplicate of this bug: 501562
Blocks: 505404
Assignee

Comment 9

10 years ago
Posted patch patch (obsolete) — Splinter Review
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)
Assignee

Comment 10

10 years ago
Make the coordinates passed to openPopupAtScreen relative to the current screen.
Attachment #396097 - Flags: review?(dao)
Assignee

Updated

10 years ago
Blocks: 393186
Attachment #396097 - Flags: review?(dao) → review-
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.
Assignee

Comment 13

10 years ago
(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.
Assignee

Comment 14

10 years ago
This addresses the issue with a taskbar on the left.
Attachment #396097 - Attachment is obsolete: true
Attachment #396128 - Flags: review?(dao)
(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
Assignee

Comment 16

10 years ago
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 19

10 years ago
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+
Assignee

Comment 20

10 years ago
Posted patch patch v2Splinter 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
Assignee

Comment 21

10 years ago
Fixed the one nit.
Attachment #396128 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Keywords: checkin-needed
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.
Assignee

Comment 23

10 years ago
(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.
http://hg.mozilla.org/mozilla-central/rev/83ba2c6e25eb
http://hg.mozilla.org/mozilla-central/rev/b652e12fc7e3
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Yes.
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.