474149 - improve popup moveTo code

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: patch, also fixes the longstanding margin uninitialized warnings

The code to move popups does some special handling for widgets. Instead, we can now just assign screen coordinates, use SetPopupPosition and things should just work.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't we still update the attributes?

Comment 2

[Neil Deakin]

Thinking about it, we probably should, although I think this should only happen if the attributes are already set. Otherwise, the attributes will override the position when the popup is opened again.

Comment 3

[Neil Deakin]

Attachment: update attributes is they are already set

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  {
+    nsAutoString left, top;
+    left.AppendInt(aLeft);
+    top.AppendInt(aTop);
+ 
+    nsWeakFrame weakFrame(this);
+    mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::left, left, PR_FALSE);
+    if (!weakFrame.IsAlive()) {
+      return;
+    }
+    mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::top, top, PR_FALSE);
+    if (!weakFrame.IsAlive()) {
+      return;
+    }

Can't you avoid the weakframe stuff by moving this to the end of the function and using a saved local reference to mContent?

Comment 5

[Neil Deakin]

Attachment: with changes

Comment 7

[Neil Deakin]

This was fixed a while ago. I had disabled a test for this, but have now fixed it as part of bug 487631.

Comment 8

[Heather Arthur [:harth]]

Comment on attachment 361601 [details] [diff] [review]
with changes

It would be great to have this for extension developers like me and the original commenters on bug 423791 and bug 457600 as a popup is the only way to get anything to appear over the browser content. Without this fix the flickering involved with moveTo looks really bad.

Comment 9

[Neil Deakin]

Comment on attachment 361601 [details] [diff] [review]
with changes

(In reply to comment #8)
> (From update of attachment 361601 [details] [diff] [review])
> It would be great to have this for extension developers like me and the
> original commenters on bug 423791 and bug 457600 as a popup is the only way to
> get anything to appear over the browser content. Without this fix the
> flickering involved with moveTo looks really bad.

moveTo shouldn't be required to do that. You should be able to just call openPopupAtScreen. 

Anyway, this patch relies on significant popup coordinate calculation code changes from bug 279539 which is too risky for 1.9.1.

Comment 10

[Wladimir Palant]

I've just added a note to https://developer.mozilla.org/en/XUL/Method/moveTo about moveTo(-1,-1) feature which was added here. Neil, is additional documentation needed maybe? From what I can tell, you at the very least fixed essential moveTo() brokenness which I can observe in 1.9.0/1.9.1 - it used to subtract the screen coordinates of the parent view from the screen coordinates passed in, this resulted in badly wrong positioning at least on Windows.