Closed Bug 1102039 Opened 5 years ago Closed 5 years ago

panel left behind and delayed when minimizing

Categories

(Core :: XUL, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox34 - wontfix
firefox35 --- affected
firefox36 --- affected
firefox37 --- verified

People

(Reporter: kjozwiak, Assigned: enndeakin)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image 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.
[Tracking Requested - why for this release]:
This is wontfix for 34. I have marked 35+ as affected but don't think this bug needs to be tracked.
Flags: firefox-backlog?
Neil, is this a generic panel issue (not dismissing until the end of the windowState transition)?
Flags: needinfo?(enndeakin)
Flags: firefox-backlog?
Flags: firefox-backlog+
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.
Flags: needinfo?(enndeakin) → needinfo?(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?
Flags: needinfo?(tnikkel) → needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
(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?
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
Duplicate of this bug: 1051902
Attached patch Flush after rollup (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Iteration: --- → 37.3
Points: --- → 3
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).
Flags: qe-verify?
Component: Search → XUL
Flags: qe-verify? → qe-verify+
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Attachment #8542586 - Attachment is obsolete: true
Attachment #8543977 - Flags: review?(tnikkel)
Comment on attachment 8543977 [details] [diff] [review]
Ensure that the popup's widget visibility is updated after rollup

Thanks
Attachment #8543977 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/bb5356f6a251
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
This is a minor issue that community can also verify on Mac.
QA Whiteboard: [good first verify][Mac issue]
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.