Closed Bug 432131 Opened 16 years ago Closed 16 years ago

[10.4] Slight delay / flash / lag when window loses focus

Categories

(Core :: Widget: Cocoa, defect, P2)

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files, 1 obsolete file)

This bug has originally been reported by Henrik Skupin in bug 406730 comment 128.

Up to now it has only been confirmed by users of Mac OS X Tiger; Leopard doesn't seem to be affected by the glitch.

Steps to reproduce:
1. Open a Firefox browser window and the Download Manager.
2. Focus the browser window.
3. Focus the Download Manager.

Expected results:
The browser window's titlebar and chrome should change color at the same time.

Actual results:
The change of the chrome color is delayed slightly.

I have attached a screenshot; what's interesting is that (with the patch for bug 432115) when the sidebar is focused, it changes its color without delay and only the rest of the chrome lags behind.
Is this really a theme issue or a widget one?
Summary: Slight delay / flash / lag when window loses focus → [10.4] Slight delay / flash / lag when window loses focus
Probably not a theme issue, but I have no idea where to look for the real cause. Might be widget, might be something else.
You're right, widget is a better starting point.
Assignee: nobody → joshmoz
Component: Theme → Widget: Cocoa
Product: Firefox → Core
QA Contact: theme → cocoa
Flags: blocking1.9?
I don't think this blocks as-is; it's not totally broken, it's just not fully polished.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
Would it be possible to remove this for 10.4? Native apps don't have this effect anyway in Tiger.
(In reply to comment #5)
> Would it be possible to remove this for 10.4? Native apps don't have this
> effect anyway in Tiger.

Sure? I cannot read anything in the Apple HIG:
http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_18_section_5.html#//apple_ref/doc/uid/20000961-TPXREF54

Perhaps it's too weak to see in some situations?
I don't have Tiger anymore, but I am pretty sure that there is a difference how inactive windows look in Tiger compared to Leopard. 

I googled a bit and found this:
http://arstechnica.com/reviews/os/mac-os-x-10-5.ars/3

"The inactive state of the new windows is now more clearly distinguished from the active state. Inactive windows fade to a much lighter shade of gray."

Easiest would be to compare this effect screen to screen though.
José, the discussion should be moved to bug 432221 which already handles the transition hardness. You can point it out there again.
This looks REALLY bad on Tiger, I think it need to be fixed before firefox 3.
Priority: -- → P2
today's nightly feels a lot better on this issue, not sure if anything changed or Im just crazy.... :)
It's probably because of a change from bug 54488 (the window is invalidated whenever focus changes), but it looks like that only helps as long as the chrome is focused. The problem returns when you click into the content area.
Thats what I thought as well.

However, I do have ore details that Im not sure if is unique case

When I open minefield fresh, (i have tabbar enable all the time), only one tab open, the delay is still there. but the delay does not apply to ANY new tabs I open, while it still applies to THE FIRST tab. If I then close that first tab, there is no more delay anywhere.

Feels like the delay is tied to one certain tab....

I will try to disable tabbar when only one tab open, see if anything changes.
ok, I tried, this time, when minefield start, only one tab, no tabbar, delay exist.

When I did cmd+T open a new tab, no delay on either tab.

So my conclusion is, delay now only significant on initial UI of the app, once the UI changed, delay is gong. Super strange.
Depends on: 432817
I have a fix for this but I need to do some performance testing.
Assignee: joshmoz → mstange
Status: NEW → ASSIGNED
Attachment #322295 - Flags: review?(joshmoz)
Did we add any other invalidations for the active/inactive chrome appearance in particular that could go away with these new invalidations?
I didn't add any in bug 406730; I suppose changing the attribute causes some invalidation but I don't know yet where that takes place. I'm trying to find out.
Without the patch, the invalidation occurs in nsViewManager::UpdateWidgetArea which is called by a reflow event.
With the patch, the reflow event doesn't cause any further invalidations. So it looks like there's nothing to remove.
Comment on attachment 322295 [details] [diff] [review]
Fix v1.0: Invalidate the window when key state changes

watch those perf #s :)
Attachment #322295 - Flags: review?(joshmoz) → review+
Attachment #322295 - Flags: superreview?(roc)
(In reply to comment #21)
> (From update of attachment 322295 [details] [diff] [review])
> watch those perf #s :)

Yeah. Let's see what happens...

(In reply to comment #20)
> With the patch, the reflow event doesn't cause any further invalidations.

I need to correct that. There is a further invalidation, but that's not directly caused by a reflow event. I think this is what happens when the window loses focus:

1.  The window loses key state.
2.  nsWindowMap.mm:335 asks OSX to redraw the window in the near future.
    (That's the line that's introduced in the patch.)
3.  The window loses main state.
4.  The NS_DEACTIVATE event is sent, the "active" attribute is removed.
5.  OSX calls -[ChildView drawRect:] as answer to (2).
6.  A paint event is dispatched to Gecko.
7.  During WillPaint(), Gecko detects that there are pending restyles, caused
    by the absence of the "active" attribute.
8.  Gecko processes these restyles and marks the affected part of the window
    as dirty. The window has not been redrawn yet.
9.  During EndUpdateViewBatch, Gecko detects this dirty state and send an
    invalidation request for the dirty part. This invalidation request is
    queued in -[ChildView setNeedsPendingDisplayInRect:]. This function starts
    a 0-interval timer.
10. The window is finally drawn.
11. The timer from (9) fires, -[ChildView processPendingRedraws:] asks OSX to
    redraw the "dirty" part (which has already been drawn in (10)).
12. OSX answers by calling -[ChildView drawRect:] (see (5)) and the paint event
    is dispatched. Only this time, there are no pending restyles and no
    further pending invalidations, so the window doesn't get invalidated again.
13. Gecko redraws the "dirty" part a second time.
(There are even more calls to drawRect but I don't know what causes them.)

Is there anything I can / should do about this, roc?
Attachment #322295 - Flags: superreview?(roc) → superreview+
This is tough. Once the OS starts paint event processing, I guess there's no way to enlarge the area we want to draw, so if there are pending restyles we have no choice but to paint again if the restyles invalidate an area that the current paint doesn't cover.

Compositor would fix this (ha!) because instead of asking the OS to schedule its paint event, we'd schedule our own XPCOM paint event, and when that fires we can flush restyles etc and then do a synchronous OS paint with the correct dirty area.
(In reply to comment #23)
> Compositor would fix this (ha!)

I'm beginning to wonder if there's anything that Compositor won't fix... ;)

This patch should be checked in to hg first, if I understood the process correctly. Let's see what happens to the perf numbers.

(The most interesting numbers in bug 432950 came from cb-xserve01, but that's based on CVS trunk, isn't it?)
Keywords: checkin-needed
Actually you can submit your patch to the try-servers and it will be run through Talos tests to give you numbers. You should definitely do that. Jump on #developers if you don't have access.
(In reply to comment #24)
> (The most interesting numbers in bug 432950 came from cb-xserve01, but that's
> based on CVS trunk, isn't it?)

Yes, though at the moment it's completely offline awaiting a brain transplant (bug 435927) :(
I started a try-server build some minutes ago. I'll give an URL when its ready.

Robert, where the results of the Talos tests can be seen?
(In reply to comment #27)
> Robert, where the results of the Talos tests can be seen?

Raw numbers will show up on the build on the MozillaTry tinderbox; the hard part is figuring out which machine is most alike to compare them to (it looks like probably qm-pmac-fast01, where the try-Ts seems to be right about at the lower edge of that machine's Ts for today—but note Ts jumped on that machine overnight right after the 02:47 orange build), given any single try-build could have wildly adverse effects on perf.

I'm unable to get a single graphs.m.o instance to graph the try-Mac's results with any of the 1.9 qm-* Mac results; I can graph them separately, but that makes comparison difficult.

try-Mac Ts: http://graphs-stage.mozilla.org/graph.html#spst=range&spstart=0&spend=1212874448&bpst=cursor&bpstart=0&bpend=1212874448&m1tid=184032&m1bl=0&m1avg=0
qm-pmac-fast01 Ts: http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1212881437&bpst=cursor&bpstart=0&bpend=1212881437&m1tid=148165&m1bl=0&m1avg=0

Tp on those two machines doesn't seem near-identical at all, nor ts_RSS, and the other tests aren't run on try-server, so it's hard to tell any other effects.  Ts was the big problem with bug 54488 (for whatever reason), though, and as far as I can tell with my limited ability to make the graph server do anything useful, it seems OK with this patch.
(In reply to comment #27)
> I started a try-server build some minutes ago.

Thanks!

(In reply to comment #29)
> Ts was the big problem with bug 54488 (for whatever reason), though,
> and as far as I can tell with my limited ability to make the graph server do
> anything useful, it seems OK with this patch.

Thank you for the analysis, Smokey.
So let's check it in to hg now. The CVS checkin shouldn't be done before cb-xserve01 is alive again.
http://hg.mozilla.org/index.cgi/mozilla-central/rev/fd0a09d29072
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Depends on: 444976
When the CVS check-in will be happen? Is cb-xserve01 still down?
It will never happen unless somebody requests approval.
Comment on attachment 322295 [details] [diff] [review]
Fix v1.0: Invalidate the window when key state changes

We have the flag wanted1.9.0.x set. So asking for approval for 1.9.0.2.
Attachment #322295 - Flags: approval1.9.0.2?
I don't have 10.4 anymore. Marcia, would it be possible for you to verify the fix on a labs machine?
Verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1pre) Gecko/2008071602 Minefield/3.1a1pre. I verified using the steps in Comment 0.
Status: RESOLVED → VERIFIED
Comment on attachment 322295 [details] [diff] [review]
Fix v1.0: Invalidate the window when key state changes

Approved for 1.9.0.2. Please land in CVS. a=ss.
Attachment #322295 - Flags: approval1.9.0.2? → approval1.9.0.2+
Keywords: checkin-needed
Landed attachment #322295 [details] [diff] [review] on cvs trunk for 1.9.0.2:

Checking in widget/src/cocoa/nsWindowMap.mm;
/cvsroot/mozilla/widget/src/cocoa/nsWindowMap.mm,v  <--  nsWindowMap.mm
new revision: 1.11; previous revision: 1.10
done
I've backed this out on branch and trunk due to the ~10% twinopen regression that it was causing.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment on attachment 322295 [details] [diff] [review]
Fix v1.0: Invalidate the window when key state changes

Minusing until the perf regression gets fixed.
Attachment #322295 - Flags: approval1.9.0.2+ → approval1.9.0.2-
In my opinion we should take this patch even though twinopen shows a performance regression due to the following reasons:

1. Only Mac OS 10.4 is affected. On Leopard, we've always had the behavior
   that the patch adds, and we've always lived with the performance impact.
   It's basically built-in into Leopard and I don't think we can do anything
   about that.

2. The patch improves perceived responsiveness. A uniform transition simply
   looks snappier.

3. The patch does not hurt responsiveness in real world usage.

4. The patch lays groundwork for other background window fixes, most notably
   bug 54488. Without this patch, the fixes for bug 54488 and bug 439354 make
   no sense.
   Some of these further fixes might mitigate the performance impact of this
   patch.

So basically the performance regression is understood, necessary and mostly harmless.


I'll try to explain what this patch does and how I've come to these conclusions.

Before the patch, this is what happened when activating / deactivating a window:
 1. repaint the titlebar
 2. *display refresh*
 3. repaint the window contents
 4. *display refresh*

The patch basically adds another repaint before the titlebar repaint:
 1. repaint the window contents
 2. repaint the titlebar
 3. *display refresh*
 4. repaint the window contents again
 5. *display refresh*

So when the first display refresh happens, both the titlebar and the rest of the window have been repainted which results in a uniform color change.

Now there are two problems.
The first problem is that the second repaint is unnecessary; it doesn't make sense to repaint things that haven't changed.
The first repaint is caused by the whole-window invalidation that I added, the second repaint is caused by the CSS styles for active/inactive windows.

There is no quick fix to this problem (see comment 22 and comment 23). However, I'm gradually moving away from using CSS styles for background window appearance by converting more and more things to -moz-appearance (see e.g. bug 439354). Using less CSS styles for the background window appearance means that the second repaint will become cheaper.
Without this patch, using -moz-appearance for these things won't work because the areas wouldn't get repainted properly.

The second problem is that we're now repainting the whole window instead of only repainting the areas that have changed. That's a pity but not really fixable; especially for bug 54488 it's unavoidable.
There's no reliable way of tracking the areas that are painted using -moz-appearance.


Finally I should say something about reason 3, "does not hurt responsiveness in real world usage".

The only way this patch impacts performance is by slightly increasing the delay after which a window is responsive after it was focused / unfocused. But that's not that important:
 - When switching away from a window, you don't want to interact with it.
 - When switching to a window, the short delay doesn't really matter either,
   because you can't react fast enough to notice.


I'd like to reconsider landing this in the light of this background info.
I think we should reland this and eat the regression. Josh?
I kind of think we should eat the regression on trunk in the hopes that we'll pick up other optimizations to bring it back down, but I'm not sure we should eat it on the 1.9 branch.
Relanding this (for 1.9.1) and thus make it possible to resolve bug 439354 will pave the way for lots of look & feel (theme) improvements for all apps.
I should clarify point 1 of comment 41.

*On Leopard, this patch doesn't change anything.* No performance regression.

Leopard has always done the invalidation that this patch adds. The patch only makes Tiger match Leopard's default behavior.
The test machines that showed the performance regression run Tiger.
I'm OK with taking this and eating the perf regression on 10.4. Seems like we understand the problem pretty well and the perceived responsiveness win is more important.
We need to reland this on a quiet tree so that the regression can be noted and won't hide other possible regressions. That means we won't be able to land it for alpha 2, so not marking check-needed just yet.
Attached patch updated to tipSplinter Review
Attachment #322295 - Attachment is obsolete: true
Re-landed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/4d8970733c8a
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1a1 → mozilla1.9.1
JFTR, some twinopen values (first cycle, Darwin 8.8.1 talos machines):

qm-pmac-trunk01: 463.32 --> 497.37 (+7.4%)
qm-pmac-trunk02: 468.16 --> 496.26 (+6%)
qm-pmac-trunk03 467.58 --> 497.53 (+ 6.4%)
qm-pmac-fast03: 469.00 --> 495.68 (+5.7%)
qm-pmac-trunk10: 468.58 --> 493.74 (+5.4%)
Target Milestone: mozilla1.9.1 → mozilla1.9.1b1
verifying again on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre
Status: RESOLVED → VERIFIED
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: