Status

()

defect
RESOLVED FIXED
11 years ago
6 months ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Attachment #357506 - Flags: superreview?(roc)
Attachment #357506 - Flags: review?(roc)
Shouldn't we still update the attributes?
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.
Attachment #357506 - Attachment is obsolete: true
Attachment #357815 - Flags: superreview?(roc)
Attachment #357815 - Flags: review?(roc)
Attachment #357506 - Flags: superreview?(roc)
Attachment #357506 - Flags: review?(roc)
+  {
+    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?
Posted patch with changesSplinter Review
Attachment #357815 - Attachment is obsolete: true
Attachment #361601 - Flags: superreview?(roc)
Attachment #361601 - Flags: review?(roc)
Attachment #357815 - Flags: superreview?(roc)
Attachment #357815 - Flags: review?(roc)
Attachment #361601 - Flags: superreview?(roc)
Attachment #361601 - Flags: superreview+
Attachment #361601 - Flags: review?(roc)
Attachment #361601 - Flags: review+
Duplicate of this bug: 478544
This was fixed a while ago. I had disabled a test for this, but have now fixed it as part of bug 487631.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Attachment #361601 - Flags: approval1.9.1?
Attachment #361601 - Flags: approval1.9.1?
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.
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.
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.