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?

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

Comment on attachment 8364354 [details] [diff] [review] v1 Looks fine to me.

Can you confirm this is fixed?

I need to land the patch first.

The behavior didn't change so far, so I'll assume it hasn't landed yet.

(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.

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).

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.

I'm working on this.

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.

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.

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.

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.

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d119852513a https://hg.mozilla.org/integration/mozilla-inbound/rev/89554fdaf65d

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.

https://hg.mozilla.org/mozilla-central/rev/0d119852513a https://hg.mozilla.org/mozilla-central/rev/89554fdaf65d

This didn't make in time for Aurora after all. Markus, could you nominate this for approval for uplift?

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bb3f06babe5 https://hg.mozilla.org/releases/mozilla-aurora/rev/5fc85c474c8b

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.