Closed Bug 958674 Opened 6 years ago Closed 6 years ago

Panels flicker when appearing

Categories

(Core :: Widget: Cocoa, defect)

29 Branch
All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 + verified
firefox30 --- verified

People

(Reporter: phlsa, Assigned: mstange)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

When panel menus (hamburger menu, bookmarks list, doorhangers...) appear, they flicker. This is not an issue on Windows 8.1
The problem first occurred with build 2014-01-08
I suspect this is a widget or Core issue.

(In reply to Philipp Sackl [:phlsa] from comment #1)
> The problem first occurred with build 2014-01-08

2014-01-08 works fine for me.

Last good revision: cf2d1bd796ea (2014-01-08)
First bad revision: 711f08b0aa1d (2014-01-09)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf2d1bd796ea&tochange=711f08b0aa1d

Last good revision: cf2d1bd796ea
First bad revision: 9022f0cacaed
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cf2d1bd796ea&tochange=9022f0cacaed I think that pushlog isn't very meaningful so I'm including the m-c pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf2d1bd796ea&tochange=9022f0cacaed

Possibly due to bug 874792?
Blocks: 874792
Component: Theme → XUL
Product: Firefox → Core
Hardware: x86_64 → All
The first bad revision is:
changeset:   162579:9bbdc400dc1f
user:        Markus Stange <mstange@themasta.com>
date:        Wed Jan 08 10:37:59 2014 +0100
summary:     Bug 957192 - Ignore unnecessary invalidations from NSView / NSWindow. Improves scrolling performance with plugins. r=smichaud

Regression found using mozcommitbuilder 0.4.10 on darwin at 2014-01-21 12:10:15
Assignee: nobody → mstange
Blocks: 957192
No longer blocks: 874792
Component: XUL → Widget: Cocoa
Attached patch v1Splinter Review
Attachment #8364354 - Flags: review?(smichaud)
Comment on attachment 8364354 [details] [diff] [review]
v1

Looks fine to me.
Attachment #8364354 - Flags: review?(smichaud) → review+
Can you confirm this is fixed?
Flags: needinfo?(philipp)
I need to land the patch first.
The behavior didn't change so far, so I'll assume it hasn't landed yet.
Flags: needinfo?(philipp)
(In reply to Benjamin Kerensa [:bkerensa] from comment #6)
> Can you confirm this is fixed?

When the patch have landed in mozilla-central, this bug will be resolved as fixed. Then it can be verified. Note that people usually land patches in another repo first (like mozilla-inbound), then the changes are merged into mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/602063f5fc68
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
While the issue appears to come up less often now that the patch has landed, it still happens.
Here's a video of what it looks like: http://cl.ly/292v1x0F1e2q (you see the problem when the panel opens for the third time).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Does it happen more often than before bug 957192 landed? If not, let's open a new bug for it.
I agree that there's still a bug to fix, but I'd like to get that out of the blame chain of bug 957192 ;-)
When bug 957192 landed, this issue popped up the first time for me, but I'm not enough of an engineer to understand whether this is a causality or just a correlation.
It appears that the glitch happens a little less frequently as of today (when the patch for this bug landed AFAIK).
This bug is effecting triggering of the menu panel in the Australis on-boarding UITour (now due to launch in Aurora). Can this bug get fixed in the next week?
Gavin, I just wanted to make sure this is on your radar as we move closer to Aurora merge.
Flags: needinfo?(gavin.sharp)
I'm working on this.
Status: REOPENED → ASSIGNED
This makes the bug happen more deterministically. Without this patch, popups are painted at the next drawRect call after the popup has been shown, and not during the [mWindow orderFront:nil]; call, because the ChildView is hidden at that point. With this patch, we unhide the view first and then call orderFront, so it gets painted during the viewWillDraw and drawRect calls that happen inside orderFront. The latter setup is also used for normal toplevel windows.
I think you've reviewed a very similar patch to this one before, but it wasn't landed because the bug I originally attached it on was fixed in a different way.

The reentrancy guard is necessary because viewWillDraw causes a flush that causes the view manager to call Show(true) on the widget again.
Attachment #8368913 - Flags: review?(smichaud)
The popup contents get painted during the content view's -[ChildView viewWillDraw] call. But painting doesn't happen if widget->NeedsPaint() at http://dxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#396 returns false. And nsIWidget::NeedsPaint starts with if (!IsVisible()) { return false; }, so we need to return true from IsVisible at that point.

(Due to part 2, viewWillDraw is called synchronously during -[NSWindow orderFront:] in nsCocoaWindow::Show.)

This fixes the flicker bug.
Attachment #8368916 - Flags: review?(smichaud)
One addition to comment 19: When widget->NeedsPaint() is false, the cached layer contents in the panel don't get updated during viewWillDraw, but during the actual drawRect (which still happens in the same orderFront call), compositing happens anyway. So what gets composited are the old layer contents from before the panel closed the last time. That's why you see the flickering: When the panel appears, it paints the completely opaque state that it was in before it got closed. After that, widget->NeedsPoint() starts returning true, so the fade-in animation paints normally. The flickering is the quick change from opaque -> transparent -> opaque.
(In reply to Markus Stange [:mstange] from comment #18)
> I think you've reviewed a very similar patch to this one before,

This was in bug 562138 comment 4, and it was actually Josh who reviewed it originally.
Comment on attachment 8368913 [details] [diff] [review]
part 2: paint popups synchronously

Sounds reasonable to me.  Sometimes Gecko does things in an order that's inappropriate for Cocoa widgets (for example "showing" an nsCocoaWindow object while its nsChildView child is still invisible).  In principle I suppose we should try to change Gecko (cross-platform code).  But it's probably a lot easier to work around it like this.
Attachment #8368913 - Flags: review?(smichaud) → review+
Comment on attachment 8368916 [details] [diff] [review]
part 3: return true from nsCocoaWindow::IsVisible during the initial paint

This also sounds reasonable.

Conceivably this and the previous patch will also fix other bugs.  But it might also cause/trigger undesirable side effects, which we'll need to watch out for.
Attachment #8368916 - Flags: review?(smichaud) → review+
(In reply to Steven Michaud from comment #22)
> Sometimes Gecko does things in an order that's
> inappropriate for Cocoa widgets (for example "showing" an nsCocoaWindow
> object while its nsChildView child is still invisible).  In principle I
> suppose we should try to change Gecko (cross-platform code).  But it's
> probably a lot easier to work around it like this.

In general I agree, but for popups, it's not Gecko's fault, because Gecko doesn't have any reference to the popup's nsChildView. From Gecko's perspective, the nsCocoaWindow and the nsChildView of a popup are one nsIWidget object (the nsCocoaWindow), and it's the nsCocoaWindow that manages all access to the nsChildView (through mPopupContentView). So this patch is not a workaround, in my opinion, but the right way to fix it.
> From Gecko's perspective, the nsCocoaWindow and the nsChildView of a
> popup are one nsIWidget object (the nsCocoaWindow), and it's the
> nsCocoaWindow that manages all access to the nsChildView (through
> mPopupContentView). So this patch is not a workaround, in my
> opinion, but the right way to fix it.

True enough.  I stand corrected.
We should get this in 29 if it doesn't make the merge today, but it's not a blocker for un-throttling Aurora updates.
Flags: needinfo?(gavin.sharp)
https://hg.mozilla.org/mozilla-central/rev/0d119852513a
https://hg.mozilla.org/mozilla-central/rev/89554fdaf65d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla29 → mozilla30
This didn't make in time for Aurora after all. Markus, could you nominate this for approval for uplift?
Flags: needinfo?(mstange)
Comment on attachment 8368913 [details] [diff] [review]
part 2: paint popups synchronously

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 957192
User impact if declined: panels flicker when opened
Testing completed (on m-c, etc.): a bit of manual testing, just landed on mozilla-central yesterday
Risk to taking this patch (and alternatives if risky): low to medium, alternative: backing out bug 957192
String or IDL/UUID changes made by this patch: none
Attachment #8368913 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mstange)
Attachment #8368916 - Flags: approval-mozilla-aurora?
Attachment #8368916 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8368913 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reproduced in nightly 2014-01-10.
The flicker is gone now in 30.0a1(2014-03-10), 29.0a2(2014-03-10) OS X 10.9.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.