Closed
Bug 1102039
Opened 10 years ago
Closed 10 years ago
panel left behind and delayed when minimizing
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
People
(Reporter: kjozwiak, Assigned: enndeakin)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
image/gif
|
Details | |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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]:
status-firefox34:
--- → affected
tracking-firefox34:
--- → ?
Comment 2•10 years ago
|
||
This is wontfix for 34. I have marked 35+ as affected but don't think this bug needs to be tracked.
Updated•10 years ago
|
Flags: firefox-backlog?
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
(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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Iteration: --- → 37.3
Points: --- → 3
Comment 11•10 years ago
|
||
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).
Updated•10 years ago
|
Flags: qe-verify?
Updated•10 years ago
|
Component: Search → XUL
Flags: qe-verify? → qe-verify+
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8542586 -
Attachment is obsolete: true
Attachment #8543977 -
Flags: review?(tnikkel)
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•10 years ago
|
Comment 16•10 years ago
|
||
This is a minor issue that community can also verify on Mac.
QA Whiteboard: [good first verify][Mac issue]
Comment 17•10 years ago
|
||
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.
Description
•