Some of the "hacks" in the popup code were to prevent mis aligning of the popup on Windows. On windows, when you type "window." then press "a" and then BACK_SPACE, the popup is no longer attached to the input area, rather, it floats on top. There are various other ways to bring about a similar effect.

Girish, can you please post a screenshot / screencast? Maybe Timothy can help us with a fix.

Following the exact STR from desription.

Thanks Girish! Timothy, can you please check if this bug would be easy to fix in the platform code? It's one of those old painful xul popup bugs for us - it does affect the autocomplete popup we use in devtools for several years. Thank you very much!

Do I need to back out the hacks you have in place in order to see this bug? Or is it visible in vanilla Firefox? Also, is it at all specific to a platform (ie Windows only)?

Right now, in the latest Nightly, and only on Windows, this can be seen on a vanilla Nightly.

Which hacks are you referring to? I couldn't find anything specific and you don't seem to be moving the popup yourself. What's happening here is that the popup is getting resized, and nsMenuPopupFrame::SetPopupPosition is adjusting the position due to the new size properly. We call nsIWidget::Resize with the new position and size. Then, we get a widget layer callback indicating that the popup was moved, followed by a resize callback. But because this panel has level="top", nsXULPopupManager::PopupMoved calls MoveTo thinking that you're trying to manually move the popup, disconnecting it from the anchor. Ideally, we should just make the move/resize callbacks just understand that the position and size that we asked for within SetPopupPosition is what we want, and we only want to do anything when something external to this changes the position and size. There's other bugs about this as well.

(In reply to Neil Deakin from comment #6) > But because this panel has level="top", nsXULPopupManager::PopupMoved calls > MoveTo thinking that you're trying to manually move the popup, disconnecting > it from the anchor. Why is the check in nsXULPopupManager::PopupMoved that the position is the same (the one that starts "aPnt.x != currentPnt.x ...") not working? ie it should decide we are in the same position and not do anything.

(In reply to Neil Deakin from comment #6) > Which hacks are you referring to? I couldn't find anything specific and you > don't seem to be moving the popup yourself. In bug 961431 we removed a bunch of ugly hacks from our autocomplete-popup.js file. We were working around XUL popup bugs - problems with positioning and sizing. These hacks were removed in an attempt to fix that new bug. It wasnt sufficient. Thanks to Timothy's work now popups no longer slide down, but we still have problems with positioning on Windows as pointed out in comment #2.

(In reply to Timothy Nikkel (:tn) from comment #7) > Why is the check in nsXULPopupManager::PopupMoved that the position is the > same (the one that starts "aPnt.x != currentPnt.x ...") not working? ie it > should decide we are in the same position and not do anything. It should, but currentPnt is (-1, -1) as the popup is anchored and not screen positioned. I think the check should really just be comparing the actual requested screen position with the aPnt passed to PopupMoved.

(silently moves this to CORE::XUL)

(In reply to Neil Deakin from comment #9) > I think the check should really just be comparing the actual requested > screen position with the aPnt passed to PopupMoved. I agree! This patch does that and it fixes the bug for me. In fact we already made the same change for the popup size in http://hg.mozilla.org/mozilla-central/rev/3253b8b2b75e

Comment on attachment 8401806 [details] [diff] [review] patch Looks good, and works with various panels I tests on Windows and Mac.

Woot! (finally)

Girish, can you please confirm this bug is fixed? Thanks!

Yup it is.

Timothy, is this okay to be uplifted ? Ideally, we would want this to be uplifted till 29 as the first fix for bug 961431 is in 29 (which exposed this bug again).

I'm a little weary of uplifting this fix to beta. In the past regressions from popup fixes like this have appeared quite late and I'd be more comfortable with a full beta cycle. Is there another way to proceed for beta?

(In reply to Timothy Nikkel (:tn) from comment #19) > I'm a little weary of uplifting this fix to beta. In the past regressions > from popup fixes like this have appeared quite late and I'd be more > comfortable with a full beta cycle. > > Is there another way to proceed for beta? Unfortunately, I dont know of a different way to proceed. We can probably leave it as-is... Can you please ask for aurora approval? I find that more important. Thanks a lot for your fixes - they were very much needed.

Comment on attachment 8401806 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): the underlying platform bug has existed for a very long time, some more recent changes in devtools has made it more prominent there User impact if declined: misplaced popup panels in some cases Testing completed (on m-c, etc.): been on m-c for a few days now, Neil tested localy with some popup positioned tests Risk to taking this patch (and alternatives if risky): popups can have some unexpected issues, but I think getting more testing exposure for this by getting it on aurora will only help that (a full beta cycle would be ideal though) String or IDL/UUID changes made by this patch: none

Comment on attachment 8401806 [details] [diff] [review] patch Aurora sure, for more eyes, but yes let's make sure this gets an entire beta cycle.