388995 - popup showing and hiding issues

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

This bug is about ensuring that the showing and hiding of popups works without getting into a funky state. I've found a couple of issues while creating popup tests.

When <menupopup onpopuphiding="event.preventDefault();" is used, the popup is supposed to be prevented from being hidden. That works, but the popup then gets into a state where it can't be hidden if preventDefault isn't called on a later attempt to hide it, or the popup can't be shown again. Also, the event shouldn't be cancellable for content popups.

This is because, for some reason I don't understand, I wrote nsXULPopupManager::HidePopup to remove the popup from the open popups list before the popuphiding event when it should really be removed afterwards.

The 'is a popup open' checks needs to be a bit better. I think four states are needed: closed, showing, open and hiding.

Comment 1

[Neil Deakin]

Attachment: adjust the open item list properly

This patch does several things, all related:
 - makes it so that the chain of open popups is adjusted properly when hiding popups, that is deletes the items after the popuphiding event rather than before.
 - ensures that popups from content can't cancel the popuphiding event otherwise they could block popups from closing.
 - makes it clearer that the apis for showing and hiding require strong references
 - maintains the open/close state of the popup better. This allows better checks for when a popup can be shown and hidden. I have a test for this which will ensure that the synchronous/asynchronous opening and closing works properly and that opening and closing in sequence also works. This test will require an api which gets this popup state, which I'll add as part of bug 386917, so I'm not including this test here.

The patch looks big, but almost all of it is tests from 279703 plus a few more tests. They now all work properly. I'm including them with this bug instead as some of the accessibility events don't get fired twice any more with this patch, which would change the tests a bit. There is still some duplicated events for menus on menubars which I'll investigate in a followup bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So in nsXULPopupManager::HidePopup the logic for mCurrentMenu is now pretty different, no?  Why?  That is, why do we no longer need to unhook the top item from the list?

Also, we're now calling SetCaptureState() even if the popup came from mPanels.  We didn't use to.  Is that correct?

Where is the removal from list that used to happen in nsXULPopupManager::HidePopupCallback happening now?

Does nsXULPopupManager::HidePopupsInDocument need to delete the things it's detaching?

Comment 3

[Neil Deakin]

(In reply to comment #2)
> So in nsXULPopupManager::HidePopup the logic for mCurrentMenu is now pretty
> different, no?  Why?  That is, why do we no longer need to unhook the top item
> from the list?
> 

Yes, that's the point of this bug, to unhook it after the popuphiding event instead. Otherwise, the popuphiding event could cancel the hiding of the popup meaning it stays open, yet the popup is no longer in the list.

> Also, we're now calling SetCaptureState() even if the popup came from mPanels. 
> We didn't use to.  Is that correct?
> 

Good catch. It should only be called if it was in the mCurrentMenu list. I'll change that.

> Where is the removal from list that used to happen in
> nsXULPopupManager::HidePopupCallback happening now?

At the beginning of the nsXULPopupManager::HidePopupCallback when it calls it again after the event is fired.

> 
> Does nsXULPopupManager::HidePopupsInDocument need to delete the things it's
> detaching?
> 

Yes it should be. I'll add some delete lines

Comment 4

[Boris Zbarsky [:bzbarsky]]

> to unhook it after the popuphiding event instead

My question was why we now unhook a different item, as far as I can tell.  We used to unhook the top item; now we unhook the thing we found...

Comment 5

[Neil Deakin]

Removing the top item is still what is desired.

Now, at the end of HidePopup, the content node for the top item corresponds to 'popupToHide' which is then passed as the first argument to 'new nsXULPopupHidingEvent' or 'FirePopupHidingEvent'. When HidePopupCallback is called, this popup is passed as the first argument aPopup. We search the list again to find the item that corresponds to aPopup. This is done because it's possible someone added another item (attempted to open another popup) during the event processing so the item isn't at the front anymore. This isn't a likely scenario, but need to handle it nevertheless. Also, the item could have been removed if a frame was destroyed for instance.

I'll add this description as a comment in HidePopupCallback

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273595 [details] [diff] [review]
adjust the open item list properly

Sounds reasonable.  One more thing.  This code:

+  aPopupFrame->HidePopup(aDeselectMenu, ePopupClosed);
   ENSURE_TRUE(weakFrame.IsAlive());
 
+  delete item;

needs to delete before the NS_ENSURE_TRUE, no?

With that and the other comments, r=bzbarsky.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273595 [details] [diff] [review]
adjust the open item list properly

a19=dbaron

(This got blocking1.9+ in our latest round of triage; roc said he was going to mark those today...)

Comment 8

[Neil Deakin]

Checked in. I had to make some adjustments to the tests for bug 387981, and changed from using load events to focus events due to occasional hangs on Windows.