1773589 - Don't close panel views before hidePopup animation is finished

Status: NEW

(Firefox :: Toolbars and Customization, task, P3)

Description

[Shane Hughes [:aminomancer]]

I should have done this as part of bug 1724622. It didn't occur to me at the time because that was focused on a particular popup that wouldn't be affected. But in order to make panels fade more consistently, we need to wait for popuphidden before closing views. Thankfully this is an easy fix because popuphidden already closes views. So we just need to return after this call.

Comment 1

[Shane Hughes [:aminomancer]]

Actually I guess it's possible there may be timing issues if we just return before closing the views. In that 180ms when the panel is closing, another panel might be trying to open the same view that was already open in the previous panel, in which case we need the views to be released. Ugh.

Gijs can you help me with this? I think we can't really let other callers pass animate = true until resolving this, because of the possibility that, for example, user has the app menu open to the history view, and then clicks the history toolbar button. Then the history view needs to instantly disappear from the app menu while it's still animating, or else the history panel will appear empty for several frames.

I guess it's possible to eliminate that conflict by checking here whether the open view is in an animating panel, and then, if it is, release the view immediately. But maybe there's a better way, and like you said before there are a lot of callers that might want to be animating, so it's potentially a pretty big decision.

Edit: Actually, on third thought... I don't think it's possible that we're calling this method as a result of user clicking a button that opens a panel. When we do that, I'm pretty sure it just calls panel.hidePopup(true) and not PanelMultiView.hidePopup(true). So it's not closing all the views in the first place.

I am testing it right now (what I just described about the history view in app menu/history panel) and it seems like we currently resolve this by not opening the history panel until the app menu has fully closed (awaiting popuphidden event I guess). So maybe this is not a big deal at all, and I can just add return after the popup.hidePopup(true) call. But maybe you have some insights on how I can make sure that same behavior will be applied to PanelMultiView.hidePopup and PanelView.prototype.hidePopup

Comment 2

[:Gijs (back August 24; he/him)]

(In reply to Shane Hughes [:aminomancer] from comment #1)

Actually I guess it's possible there may be timing issues if we just return before closing the views. In that 180ms when the panel is closing, another panel might be trying to open the same view that was already open in the previous panel, in which case we need the views to be released. Ugh.

As you discovered, I think this part should be OK.
<snip>

Edit: Actually, on third thought... I don't think it's possible that we're calling this method as a result of user clicking a button that opens a panel. When we do that, I'm pretty sure it just calls panel.hidePopup(true) and not PanelMultiView.hidePopup(true). So it's not closing all the views in the first place.

I am testing it right now (what I just described about the history view in app menu/history panel) and it seems like we currently resolve this by not opening the history panel until the app menu has fully closed (awaiting popuphidden event I guess). So maybe this is not a big deal at all, and I can just add return after the popup.hidePopup(true) call. But maybe you have some insights on how I can make sure that same behavior will be applied to PanelMultiView.hidePopup and PanelView.prototype.hidePopup

In response to comment 0 though, my question is whether the code comment here: https://searchfox.org/mozilla-central/rev/552bfc6334b797d92fb2ce8e93a6ace5f47cd56d/browser/components/customizableui/PanelMultiView.jsm#622-624

// We close all the views synchronously, so that they are ready to be opened
// in other PanelMultiView instances. The "popuphidden" handler may also
// call this function, but the second time openViews will be empty.

Is incorrect - is openViews not empty? It sounds like in some cases when called from popuphidden, it is empty, which would mean that the effect is different. Perhaps we don't reliably fire viewhiding listeners? That should be testable... Can you check?

Comment 3

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 4

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:Gijs, since the bug has recent activity, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 5

[Shane Hughes [:aminomancer]]

I'll look into it this week before I forget

Comment 6

[:Gijs (back August 24; he/him)]

(The bot is misbehaving, sorry for the churn!)