628238 - Arrow panels mispositioned when page has a different zoom level

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[gadjo]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
Build Identifier: 

Invalid input arrow panel is not properly aligned to the input field when the page zoom level is != 1


Reproducible: Always

Steps to Reproduce:
1. Open the URL
2. Press Ctrl--
3. Submit the form

Actual Results:  
Arrow panel is not corretly aligned to the input field


Expected Results:  
Arrow panel should be aligned to the input field


Try different zoom levels (<1 and >1) - every time the panel is misaligned in a different way

Comment 1

[gadjo]

Attachment: screenshots from different zoom levels

Comment 2

[Boris Zbarsky [:bzbarsky]]

Mounir?

Comment 3

[Mounir Lamouri (:mounir)]

Why wouldn't that be a XUL panel bug? It seems that XUL panels should take into account zoom when setting their position? or the issue is somewhere else?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well... What units are you specifying the panel position in?  Or are you not specifying it at all (just setting the anchor point)?

Do these panels live in the content page, or are they chrome overlaid on the content?

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #4)
> Well... What units are you specifying the panel position in?  Or are you not
> specifying it at all (just setting the anchor point)?

I'm specifying the anchor point and an offset (in pixel). I'm also passing in parameter where to anchor (like "center", "top", ...) but this is translated to position by the panel.
The only thing that might be wrong in what I pass in the offset but it would not behave so badly if it was [only] that.

> Do these panels live in the content page, or are they chrome overlaid on the
> content?

AFAIUI, they are chrome overlaid on the content.

Comment 6

[Boris Zbarsky [:bzbarsky]]

In that case, chances are they're using the chrome's "css to device pixel" conversion factor, which is wrong in this case...

Comment 7

[gadjo]

(In reply to comment #5)
> > Do these panels live in the content page, or are they chrome overlaid on the
> > content?
> 
> AFAIUI, they are chrome overlaid on the content.

See bug 628020, I guess if they are live in page they would scroll when the page is scrolled

Comment 8

[Neil Deakin]

Attachment: account for zoom pixel difference

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 506425 [details] [diff] [review]
account for zoom pixel difference

This is solving the arrow positioning issue but the panel isn't positioned exactly where it should be. Though, this issue isn't a big deal.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> This is solving the arrow positioning issue but the panel isn't positioned
> exactly where it should be. Though, this issue isn't a big deal.

I will open another bug about that if this is not fix in this bug later.

Comment 11

[Neil Deakin]

Can you describe the additional problem in more detail? Or is it unrelated to this bug?

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> Can you describe the additional problem in more detail? Or is it unrelated to
> this bug?

Hmmm, there is no issue actually. I've double-checked that and it is just more obvious that a checkbox has a frame much bigger than it's content when zoomed so you see the arrow beginning a dozen of pixel beyond.

However, go to http://oldworld.fr/mozilla/form-ltr-rtl.html, zoom max and try to have the popups showing up. Some of them will have no arrow.

Comment 13

[Neil Deakin]

(In reply to comment #12)
> However, go to http://oldworld.fr/mozilla/form-ltr-rtl.html, zoom max and try
> to have the popups showing up. Some of them will have no arrow.

I noticed that, but I think it may be because of the larger border or outline. We may want to use the border rectangle.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> I noticed that, but I think it may be because of the larger border or outline.
> We may want to use the border rectangle.

Likely indeed. When it happens, popup.xml sees the popup 1px inside the anchor. I guess nsMenuFramePopup.cpp should be more careful when positionning the popup? I will see what I can do. But I guess this could be part of another bug.

Comment 15

[Neil Deakin]

nsMenuPopupFrame.cpp used the frame rectangle which includes padding and border.

However, popup.xml uses getBoundingClientRect which I think is the content rectangle (excluding padding and border). It should be a matter of just adding the padding and border to it.

Comment 16

[Timothy Nikkel (:tnikkel)]

The spec (http://www.w3.org/TR/cssom-view/#the-getclientrects-and-getboundingclient) and MDN say getBoundingClientRect is the border box.

Comment 17

[Mounir Lamouri (:mounir)]

I've investigated the issue a bit and I think that's a round issue. The issue appear when border width equals 0.6666 for example. I would guess that anchorFoo has a value floored... I will write a patch.

Comment 18

[Neil Deakin]

OK, rounding sounds like the issue then. We should try to use consistent rounding at least.

Comment 19

[Neil Deakin]

Attachment: patch with test

Comment 20

[[e-mail]]

Comment on attachment 506826 [details] [diff] [review]
patch with test

>+  setScale(frames[0], 1.5);
>+  let utils = frames[0].QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                        getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  zoomFactor = utils.screenPixelsPerCSSPixel;
How does the zoom factor differ from the scale you just set?

Is is possible for the chrome's screenPixelsPerCSSPixel not to equal 1?

>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
Isn't this a mochichrome test now?

Comment 21

[Neil Deakin]

(In reply to comment #20)

> How does the zoom factor differ from the scale you just set?

Don't know. I don't think it matters to check even if it is always the same.

> Is is possible for the chrome's screenPixelsPerCSSPixel not to equal 1?

Possible I guess, but it seems like an edge case not worth worrying about.
 
> >+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> Isn't this a mochichrome test now?

Will remove this.

Comment 22

[[e-mail]]

Comment on attachment 506826 [details] [diff] [review]
patch with test

>+ifneq (gtk2,$(MOZ_WIDGET_TOOLKIT))
Hmm, I think I saw dao check in Linux arrowpanels again?

>+  setScale(frames[0], 1.5);
>+  let utils = frames[0].QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                        getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  zoomFactor = utils.screenPixelsPerCSSPixel;
Well, I still think it looks like an odd way to write zoomFactor = 1.5;

Comment 23

[Neil Deakin]

Attachment: updated patch

I enabled the test on Linux and I also needed to enlarge the chrome test frame.

Comment 24

[Neil Deakin]

Attachment: Updated patch

Disabled the test on Linux again as the different margins on the popup was causing a failure. This can fixed up in another bug.

An accessibility test was also causing a hang due to the enlarged chrome frame, so I added a fix to that test to scroll the content into view after it finished.

Comment 25

[Neil Deakin]

Comment on attachment 508506 [details] [diff] [review]
Updated patch

The test was enabled on Linux so I'll fix this up now.

Comment 26

[Neil Deakin]

Attachment: adjusted the label position to fix up the test for linux

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 509407 [details] [diff] [review]
adjusted the label position to fix up the test for linux

Can we get this landed soon, please?

Comment 28

[Johnathan Nightingale [:johnath]]

This has had approval for most of a week - is it landing soon, or missing FF4?

Comment 30

[Mounir Lamouri (:mounir)]

Comment on attachment 509407 [details] [diff] [review]
adjusted the label position to fix up the test for linux

>diff --git a/toolkit/content/widgets/popup.xml b/toolkit/content/widgets/popup.xml
>+        try {
>+          let anchorWindow = anchor.ownerDocument.defaultView;
>+          if (anchorWindow != window) {
>+            let utils = anchorWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                                     getInterface(Components.interfaces.nsIDOMWindowUtils);
>+            let spp = utils.screenPixelsPerCSSPixel;
>+            anchorLeft = Math.round(anchorLeft * spp);
>+            anchorRight = Math.round(anchorRight * spp);
>+            anchorTop = Math.round(anchorTop * spp);
>+            anchorBottom = Math.round(anchorBottom * spp);
>+          }
>+        } catch(ex) { }
>+

Please, Neil, could you change the anchorFoo = Math.round(anchorFoo * spp) lines to anchorFoo *= spp?
Otherwise, you are going to accidentally recreate what caused bug 629002.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/7eda7df522d1