2012305 - Popups no longer update position

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Shane Hughes [:aminomancer]]

Popups used to constantly update their position when moving the window they're attached to. They still emit popuppositioned events, but the popup does not actually position. For most of the main chrome UI popups, they at least update position when you stop dragging - not ideal, but at least the popup isn't left permanently hanging. But for OMC's feature callout panels, they never update position. They used to work just fine, and smoothly follow the anchor around the screen. The only things that are special about these panels are the xul attributes and the use of a popuppositioned event listener (which shouldn't do anything except set an attribute on the panel, which is only used by CSS to move the custom arrow).

So the part most confusing to me is distinguishing the feature callout panels from other panels - what makes them different. These popups stay open even when you click outside them. For other popups, you would need to use the disable_autohide pref. But this can still matter for any popup, as the anchor can occasionally move without the user needing to click outside the popup. Anyway, popup movement is rare for the usual popups, but common for feature callouts because autohide is disabled. But I tried removing the noautohide attribute, and it still doesn't update position. behavior Another difference might be that our popup is not created by an event, so there is no triggerEvent param to pass to openPopup. Other popups tend to pass this, so maybe that's significant.

Pushlog

I suspect this was regressed by bug 1933181, but I'm not certain, because the bisection fell apart at the end:

1:40.37 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=860e9251e02806437cda3650e62d5747aaa45895&tochange=69063d81c4e1f54731d4a8698c63c3a703e83966

1:42.64 INFO: ************* Switching to autoland by process of elimination (no branch detected in commit message)
1:43.84 ERROR: Unable to exploit the merge commit. Origin branch is mozilla-central, and the commit message for 69063d81 was:
Bug 1994157 - Partially revert the previous patch to land what emilio intended.

Testing

Our feature callout panels can be easily tested by going to about:config and enabling browser.newtabpage.activity-stream.asrouter.devtoolsEnabled, then going to about:asrouter, searching for "TEST_FEATURE_TOUR", and hitting the blue Show button under it. That will open a feature callout anchored to the app menu button, so it should follow the button around. But if you move the window, it doesn't - at least on Windows. If anyone can test this on another OS that would be great.

---

I'm CCing @emcminn since she could probably easily test this on macOS, and @emilio since he knows about how the popup code has changed lately.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1933181

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2012305 - Fix popup moves on window moves. r=tnikkel,#layout

We don't unconditionally move-resize native windows every frame anymore,
but we still need to schedule a native move resize in cases where the
window moves.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

So adding an automated test for this should be reasonably easy. Is there an example test that tests this mechanism? It should be possible to move the window with window.moveBy(2, 2) or so and checking that the popup's getOuterScreenRect() also changes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8f3cd839279e
https://hg.mozilla.org/integration/autoland/rev/a0a76de7b645
Fix popup moves on window moves. r=tnikkel,layout-reviewers

Comment 5

[Shane Hughes [:aminomancer]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)

So adding an automated test for this should be reasonably easy. Is there an example test that tests this mechanism? It should be possible to move the window with window.moveBy(2, 2) or so and checking that the popup's getOuterScreenRect() also changes.

Not in the feature callout tests. It's just assumed that the panel works as usual. I can file a followup to add a test, though. Seems straightforward enough. The right spot would probably be around here.

Alternatively, it seems like a basic enough mechanism to have its own test like these. test_popup_tracksWindowMoves or something like that.

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a0a76de7b645