1102039 - panel left behind and delayed when minimizing

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: delayIssue.gif

When minimizing fx while the search toolbar panel is opened, the panel will be left behind until fx is completely minimized.

Attached a .gif that demonstrates the issue.

Comment 1

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

[Tracking Requested - why for this release]:

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

This is wontfix for 34. I have marked 35+ as affected but don't think this bug needs to be tracked.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Neil, is this a generic panel issue (not dismissing until the end of the windowState transition)?

Comment 4

[Neil Deakin]

The panel dismisses and hides right away, but repainting doesn't happen until the minimize effect has finished. (nsView::NotifyEffectiveVisibilityChanged calls ResetWidgetBounds requesting asynchronous painting) Maybe there's a way we could force a repaint in this specific case when a popup view has been hidden?

This issue affects all popups and menus including select element popups.

Comment 5

[Timothy Nikkel (:tnikkel)]

I believe that the NotifyEffectiveVisibilityChanged call for popups comes during reflow and if we were to allow that to synchronously call ResetWidgetBounds the nsIWidget::Show or resize calls it does can cause the OS to ask us to repaint synchronously from those calls. Painting during reflow is a no-no. How does the minimize animation get kicked off? Can we just ensure that we've processed pending updates on the view manager before we kick off that animation?

Comment 6

[Neil Deakin]

The panel is rolled up in the normal way using nsXULPopupManager::Rollup, the same as other ways that the panel might disappear due to some other user interaction. On Mac, this process starts at nsCocoaWindow.mm windowWillMiniaturize.

SetViewVisibility is only called for nsMenuPopupFrames during layout to show the popup. Hiding is done outside of layout. In fact, I just tested by adding a direct call to ResetWidgetBounds with synchronous painting directly after SetViewVisibility(false) is called and this causes the panel to disappear right away, and "fixes" this bug.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Neil Deakin from comment #6)
> The panel is rolled up in the normal way using nsXULPopupManager::Rollup,
> the same as other ways that the panel might disappear due to some other user
> interaction. On Mac, this process starts at nsCocoaWindow.mm
> windowWillMiniaturize.
> 
> SetViewVisibility is only called for nsMenuPopupFrames during layout to show
> the popup. Hiding is done outside of layout. In fact, I just tested by
> adding a direct call to ResetWidgetBounds with synchronous painting directly
> after SetViewVisibility(false) is called and this causes the panel to
> disappear right away, and "fixes" this bug.

Ah, good point. I'd still prefer if we didn't do the actual nsIWidget::Show call from nsMenuPopupFrame::HidePopup where we are holding frame pointers and we are in the middle of updating state on the frame/content tree for the hiding of the popup. The code is probably safe as is but I think we should just avoid adding any more tricky to handle calls in this code.

How about if we add a call to the end of nsXULPopupManager::Rollup that flushes any pending widget visibility changes on the view manager?

Comment 8

[Neil Deakin]

Oh I wasn't suggesting that calling Show more directly was the right fix. But I can add a visibility flush after rollup is called.

Comment 10

[Neil Deakin]

Attachment: Flush after rollup

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8542586 [details] [diff] [review]
Flush after rollup

Great. Would be even better if we did the ProcessPendingUpdates variant that didn't do painting (just reset widget bounds and painting if the os asks for it in response to that).

Comment 12

[Neil Deakin]

Attachment: Ensure that the popup's widget visibility is updated after rollup

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8543977 [details] [diff] [review]
Ensure that the popup's widget visibility is updated after rollup

Thanks

Comment 14

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5356f6a251

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This is a minor issue that community can also verify on Mac.

Comment 17

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 37 beta 6 under Mac OS X 10.9.5.

I also checked on Ubuntu 12.04 32-bit: when minimizing the window, the search panel is dismissed, but the window remains on view. After investigating, it seems that this patch didn't cause this behavior, so I'll file a new bug for it.