Closed Bug 628238 Opened 9 years ago Closed 8 years ago

Arrow panels mispositioned when page has a different zoom level

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: jk1700, Assigned: enndeakin)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 4 obsolete files)

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
Version: unspecified → Trunk
Blocks: 616607
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Mounir?
Component: XP Toolkit/Widgets: XUL → Layout: Form Controls
QA Contact: xptoolkit.xul → layout.form-controls
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?
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?
(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.
In that case, chances are they're using the chrome's "css to device pixel" conversion factor, which is wrong in this case...
Component: Layout: Form Controls → XUL
QA Contact: layout.form-controls → xptoolkit.widgets
(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
Component: XUL → XUL Widgets
Product: Core → Toolkit
QA Contact: xptoolkit.widgets → xul.widgets
Summary: XUL panels alignment method should take into account page zoom level → Arrow panels mispositioned when page has a different zoom level
Attachment #506425 - Flags: feedback?(mounir.lamouri)
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.
Attachment #506425 - Flags: feedback?(mounir.lamouri) → feedback+
(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.
Can you describe the additional problem in more detail? Or is it unrelated to this bug?
(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.
(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.
(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.
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.
The spec (http://www.w3.org/TR/cssom-view/#the-getclientrects-and-getboundingclient) and MDN say getBoundingClientRect is the border box.
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.
OK, rounding sounds like the issue then. We should try to use consistent rounding at least.
Attached patch patch with test (obsolete) — Splinter Review
Attachment #506425 - Attachment is obsolete: true
Attachment #506826 - Flags: review?(neil)
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?
(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 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;
Blocks: 629002
Attached patch updated patch (obsolete) — Splinter Review
I enabled the test on Linux and I also needed to enlarge the chrome test frame.
Assignee: nobody → enndeakin
Attachment #506826 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #507839 - Flags: review?(neil)
Attachment #506826 - Flags: review?(neil)
Attachment #507839 - Flags: review?(neil) → review+
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #507839 - Attachment is obsolete: true
Attachment #508506 - Flags: approval2.0?
Comment on attachment 508506 [details] [diff] [review]
Updated patch

The test was enabled on Linux so I'll fix this up now.
Attachment #508506 - Flags: approval2.0?
Attachment #508506 - Attachment is obsolete: true
Attachment #509407 - Flags: approval2.0?
Attachment #509407 - Flags: approval2.0? → approval2.0+
No longer blocks: 629002
Depends on: 629002
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?
This has had approval for most of a week - is it landing soon, or missing FF4?
Attachment #509407 - Flags: approval2.0+
Duplicate of this bug: 641664
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.
Whiteboard: not-ready
http://hg.mozilla.org/mozilla-central/rev/7eda7df522d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Flags: in-testsuite+
Whiteboard: not-ready
You need to log in before you can comment on or make changes to this bug.