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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Sylvain Pasche]

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?

Comment 1

[Dão Gottwald [:dao]]

Are you sure screen.availLeft is wrong? I think we're actually passing correct numbers to openPopupAtScreen.

Comment 2

[Sylvain Pasche]

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.

Comment 5

[Simon Bünzli]

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).

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: fixup ctrl+tab openPopupAtScreen usage

Make the coordinates passed to openPopupAtScreen relative to the current screen.

Comment 11

[Dão Gottwald [: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.

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Timothy Nikkel (:tnikkel)]

(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.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: fixup ctrl+tab openPopupAtScreen usage v2

This addresses the issue with a taskbar on the left.

Comment 15

[Dão Gottwald [: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

Comment 16

[Timothy Nikkel (:tnikkel)]

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.

Comment 17

[Dão Gottwald [:dao]]

(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 18

[Dão Gottwald [:dao]]

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

Comment 19

[Neil Deakin]

Comment on attachment 396096 [details] [diff] [review]
patch

Probably a bit hard to simplify this so that the screen rectangle is only retrieved once?

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

(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.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: fixup ctrl+tab openPopupAtScreen usage v3

Fixed the one nit.

Comment 22

[Dão Gottwald [:dao]]

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.

Comment 23

[Timothy Nikkel (:tnikkel)]

(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+.

Comment 24

[Dão Gottwald [:dao]]

It's possibly that my tree was busted. In fact I just removed and re-cloned the whole thing.

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/83ba2c6e25eb
http://hg.mozilla.org/mozilla-central/rev/b652e12fc7e3

Comment 26

[Dão Gottwald [:dao]]

(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

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 28

[Dão Gottwald [:dao]]

Yes.

Comment 29

[Tanner M. Young [:tmyoung]]

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