1396414 - Application Menu jumps after clicking Customize (Intermittent)

Status: NEW

(Firefox :: Menus, defect, P3)

Description

[Aaron Benson]

See the attached video. After clicking Customize from the Application Menu, the application menu panel jumps up making it feel glitchy. It only happens sometimes but seems pretty easily reproduceable.

Comment 1

[Aaron Benson]

Video of behavior: https://cl.ly/2f3e0f312G23

Comment 3

[:Gijs (he/him)]

I've tried to look into this, but I don't understand what's going on. The customize toolbar button is linked to cmd_CustomizeToolbars, which calls gCustomizeMode.enter(). At the start of enter() (here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#213 ) the panel's "state" property is "closed". If not the layout changes caused by customize mode, I don't understand what would cause the panel to move like this. Neil?

Comment 4

[:Gijs (he/him)]

Trying to find a regression window for this, I think this started to be much more jarring / noticeable as a result of bug 1352075. It happened before that but was less noticeable because the panel shrunk as part of the hiding animation, and, it seems to me, the anchoring was always correct (never overlapped the navbar).

I think this is glitchy enough that we should ideally not ship 57 with it.

Comment 5

[:Gijs (he/him)]

Attachment: Pre-animation-change recording

The bizarre thing before the animation change is that it seems the panel goes big -> small -> big while animating out.

Either way, this seems broken. Given that the panel state is already 'closed' by the time our code runs, I don't know how we could wait for the panel to stop animating out or something (assuming that's triggering weirdness here). 

The other odd thing is that turning off cosmetic animations here doesn't seem to help, at least on current nightly (though it might make things less frequent, I can still reproduce after turning it off and restarting). I don't understand how/why panel layout would break that way.

Comment 6

[Julien Cristau [:jcristau]]

tracking per comment 4 as new regression

Comment 7

[Neil Deakin]

Bug 1352075 added a -70px transform that applies whenever the animate attribute on the panel is not 'open' or 'cancel'. Removing that and I don't see the jumping anymore. So I'm assuming that this has something to do with the animation.

Comment 8

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #7)
> Bug 1352075 added a -70px transform that applies whenever the animate
> attribute on the panel is not 'open' or 'cancel'. Removing that and I don't
> see the jumping anymore. So I'm assuming that this has something to do with
> the animation.

Sam, can you look into this? Can we maybe ensure that we don't apply that style while the panel is closing?

Comment 9

[Sam Foster [:sfoster] (he/him)]

I can take a look. The "resting" state of the panels is at -70px, and on opening there is a transform transition that moves them to 0px. When a panel is closed, we just transition opacity to 0 - i.e. there is no transform animation. 

(the panel animations are not currently covered by the cosmeticAnimations.enabled pref. They should be, but when I tried to do this in bug 1380065 (and default it to false in tests) I ran afoul of a gazillion test failures.)

Leaving ni? on myself for follow-up.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Sam Foster [:sfoster] from comment #9)

> Leaving ni? on myself for follow-up.

No you didn't. ;-)

Comment 11

[Sam Foster [:sfoster] (he/him)]

> > Leaving ni? on myself for follow-up.
> 
> No you didn't. ;-)

Apparently clearing and setting results in just clearing ni? Assigning instead which is probably what I meant anyhow.

Comment 12

[Sam Foster [:sfoster] (he/him)]

I'm not able to reproduce this on Windows. And we wont see it on Linux as the animations are disabled there. So marking this as OSX for now.

Comment 13

[Sam Foster [:sfoster] (he/him)]

WIP update: I don't see any particular problem with the CustomizeUI logic. I think this is in the popup/arrowpanel binding, the css and the nsPopupManager code. With some logging in the arrowpanel/popup bindings, When you click "Customize" on the open panel, I'm seeing this sequence: 

  appMenu-popup:hidePopup
  appMenu-popup:popuphiding
  appMenu-popup:popuphidden
  identity-popup:hidePopup
  appMenu-popup:hidePopup

The strange thing about this is that the hiding is supposed to be async, and should wait for the transition on the panel to end (it does a fade over 180ms.) The reason we would see the menu slide up is if we get to popuphidden which removes the animate attribute, while the popup is still visible. So I want to confirm this call to HasCurrentTransitions http://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp# is returning the expected value.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1396414 - Take the popup cancel path when dismissing the appMenu from CustomizableUI.

Review commit: https://reviewboard.mozilla.org/r/182902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182902/

Comment 15

[Sam Foster [:sfoster] (he/him)]

So, attachment 8911435 [details] appears to fix the problem for me on OSX. Where I'm having a harder time is explaining why this is so. I think part of it is the slightly confusing semantics of popup hiding. The "cancel" parameter has the effect that the popup hits this rule[1] which is what we want. This is the path taken if you place focus somewhere else when a popup/panel is open. But that is just "hiding" to me, whereas "cancelling" would be closing the panel abruptly without a transition. 

I'm happy to be schooled on this. Ni :enn for that :)

[1] http://searchfox.org/mozilla-central/source/toolkit/content/xul.css#475

Comment 16

[Neil Deakin]

Cancelling means that the user has closed the popup without performing an action and the transition should apply.  Otherwise the user has performed an action and the transition does not occur when closing the popup.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8911435 [details]
Bug 1396414 - Take the popup cancel path when dismissing the appMenu from CustomizableUI.

https://reviewboard.mozilla.org/r/182902/#review188412

(In reply to Neil Deakin from comment #16)
> Cancelling means that the user has closed the popup without performing an
> action and the transition should apply.  Otherwise the user has performed an
> action and the transition does not occur when closing the popup.

Hm, but with the patch a transition would apply even when closing the popup by performing an action. It also only fixes this for the hamburger panel, but based on the comments so far I would expect the same issue to occur with other popups.

As a result, I don't think this is right. Can we avoid the mispositioning some other way, for instance by having an attribute set once the popup shows that isn't removed until after popuphidden has fired (assuming that's late enough), and updating the CSS so the -70px offset doesn't apply until the attribute is gone ? Maybe such an attribute already exists?

Comment 18

[Sam Foster [:sfoster] (he/him)]

> (In reply to Neil Deakin from comment #16)
> > Cancelling means that the user has closed the popup without performing an
> > action and the transition should apply.  Otherwise the user has performed an
> > action and the transition does not occur when closing the popup.

Ah, that wasn't clear to me until now. Thanks. 

> As a result, I don't think this is right. Can we avoid the mispositioning
> some other way, for instance by having an attribute set once the popup shows
> that isn't removed until after popuphidden has fired (assuming that's late
> enough), and updating the CSS so the -70px offset doesn't apply until the
> attribute is gone ? Maybe such an attribute already exists?

We have the animate and the panelopen attributes to play with, I think that gives me the options I need.

Comment 19

[Sam Foster [:sfoster] (he/him)]

(In reply to Neil Deakin from comment #16)
> Cancelling means that the user has closed the popup without performing an
> action and the transition should apply.  Otherwise the user has performed an
> action and the transition does not occur when closing the popup.

So, this is why I was confused: On http://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#1609-1611 the comment says: 

        // If animate="false" then don't transition at all. If animate="cancel",
        // only show the transition if cancelling the popup or rolling up.
        // Otherwise, always show the transition.

And the code goes on to wait for a transition if 

        if (!animate.EqualsLiteral("false") &&
            (!animate.EqualsLiteral("cancel") || aIsCancel)) {


So, we expect a transition if the animate isn't "false" and isnt "cancel". It seems like animate=cancel is meaningless here, or at least misleading - we're not cancelling anything unless aIsCancel is also true. Why not animate="close"? It would be good to have unambiguous state, where animate="close" (action was taken, popup is being dismissed) or animate="cancel" (no action taken) to drive the hiding behavior and animation (or lack of it). But I would need to know if the panel is being cancelled or dismissed in the popuphiding handler, and I don't currently see a way to know that?

Comment 20

[Neil Deakin]

That seems reasonable. Providing an indication of whether the panel was being cancelled versus accepted in the popuphiding handler would involve creating a custom event with such a flag. Alternatively, nsXULPopupManager::FirePopupHidingEvent could set an attribute (such as setting the animate attribute itself) just before firing the event. The former would take more code, but the latter would be less flexible.

Comment 21

[Sam Foster [:sfoster] (he/him)]

(In reply to Neil Deakin from comment #20)
> That seems reasonable. Providing an indication of whether the panel was
> being cancelled versus accepted in the popuphiding handler would involve
> creating a custom event with such a flag. 

Are you suggesting something like a new popupcancelling event - instead of or as well as the popuphiding event? I suspect we wouldn't want to uplift a patch that changes the lifecycle events for popups, though that might be the optimal way to handle it in 58+. 

> Alternatively,
> nsXULPopupManager::FirePopupHidingEvent could set an attribute (such as
> setting the animate attribute itself) just before firing the event.

This seems lower risk, but I feel a bit ooky about having the nsXULPopupManager set attributes on panel nodes while firing events - seems like unexpected side-effects? Though maybe there is good precedent for this that I'm not aware of. 

I guess the other possibility - and maybe this is what you meant - is to have nsXULPopupManager::FirePopupHidingEvent fire a custom event of a new PopupHidingEvent type (which extends MouseEvent?) with a .cancelling property on it? That may not meet the trivial, upliftable bar either but I'm open to suggestions.

Comment 22

[Neil Deakin]

Your last paragraph describes what I had meant with an additional property in the popuphiding event. That would be the most ideal but I think setting an attribute would be fine if you needed some quick fix. I don't think there would be any issue with side effects if you set some unused attribute. Then switch to using an event later.

Comment 23

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1396414 - Only animate cancelled popups, using a temporary result attribute.

* Add temp. result attribute when hiding a popup, to communicate when a popup is being cancelled (no action taken) vs. not.
* Refactor arrowpanel transition properties to be clearer about what animates, when.

Review commit: https://reviewboard.mozilla.org/r/185272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185272/

Comment 24

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8913874 [details]
Bug 1396414 - Only animate cancelled popups, using a temporary result attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185272/diff/1-2/

Comment 25

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8913874 [details]
Bug 1396414 - Only animate cancelled popups, using a temporary result attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185272/diff/2-3/

Comment 26

[Sam Foster [:sfoster] (he/him)]

AFAICT, the changes in  attachment 8913874 [details] should fix the symptoms here, but on OSX I'm still seeing the panel jump up when it is dismissed (e.g. you click on "Customize" in the appMenu). The goal is to just hide the panel in these circumstances, but it seems like we are seeing the transform get restored to translateY(-70px) before it gets hidden. 

I've fiddled with it, but even if I set style.visibility to hidden in that popuphiding handler, we still see the transition. So I'm wondering if this could be related to the -moz-window-transform implementation somehow? Apologies in advance if I'm missing something silly, but I'm running out of ideas here.

Comment 27

[Markus Stange [:mstange]]

Yes, it could be caused by the implementation of -moz-window-transform. Specifically, we update the window transform synchronously when we process the restyle, whereas hiding a popups occurs asynchronously: nsView::SetVisibility does not propagate to the widget instantly, only once nsView::DoResetWidgetBounds is called.

This seems a bit unfortunate. Can you work around it by setting -moz-window-opacity to zero while the panel is hidden?

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Sam, are you still working on this issue? Anything ready to land for 58? We could still consider uplift to 57, though it is getting late in the beta cycle.

Comment 29

[Sam Foster [:sfoster] (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28)
> Hi Sam, are you still working on this issue? Anything ready to land for 58?
> We could still consider uplift to 57, though it is getting late in the beta
> cycle.

I'm not actively working on this right now. Markus and I talked over slack but I've not got a strong lead on the cause yet. I'll relinquish in case someone else can spot and land a fix in time for 57.