958674 - Panels flicker when appearing

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

When panel menus (hamburger menu, bookmarks list, doorhangers...) appear, they flicker. This is not an issue on Windows 8.1

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

The problem first occurred with build 2014-01-08

Comment 2

[Matthew N. [:MattN]]

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?

Comment 3

[Matthew N. [:MattN]]

The first bad revision is:
changeset:   162579:9bbdc400dc1f
user:        Markus Stange <[e-mail]>
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 4

[Markus Stange [:mstange]]

Attachment: v1

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8364354 [details] [diff] [review]
v1

Looks fine to me.

Comment 6

[Benjamin Kerensa [:bkerensa]]

Can you confirm this is fixed?

Comment 7

[Markus Stange [:mstange]]

I need to land the patch first.

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 9

[Stefan [:stefanh]]

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

Comment 10

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/602063f5fc68

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/602063f5fc68

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 13

[Markus Stange [:mstange]]

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

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 15

[Alex Gibson [:agibson]]

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?

Comment 16

[Benjamin Kerensa [:bkerensa]]

Gavin, I just wanted to make sure this is on your radar as we move closer to Aurora merge.

Comment 17

[Markus Stange [:mstange]]

I'm working on this.

Comment 18

[Markus Stange [:mstange]]

Attachment: part 2: paint popups synchronously

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.

Comment 19

[Markus Stange [:mstange]]

Attachment: part 3: return true from nsCocoaWindow::IsVisible during the initial paint

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.

Comment 20

[Markus Stange [:mstange]]

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.

Comment 21

[Markus Stange [:mstange]]

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

[Steven Michaud [:smichaud] (Retired)]

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 23

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 24

[Markus Stange [:mstange]]

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

Comment 25

[Steven Michaud [:smichaud] (Retired)]

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

Comment 26

[Markus Stange [:mstange]]

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

Comment 27

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

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 30

[Markus Stange [: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

Comment 31

[Markus Stange [:mstange]]

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

Comment 33

[Paul Silaghi, QA [:pauly]]

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.