516924 - Cocoa unnecessarily repaints our views as soon as we paint a focus ring

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

When debugging the repaints in bug 424715, I noticed that drawRect is asked to repaint the whole view, even though we only call setNeedsDisplayInRect for much smaller rects. And not only that: the repainted rect was even larger than the view, expanded by 3 pixels to the top and to the bottom.
Then I remembered a paragraph from http://developer.apple.com/mac/library/releasenotes/Cocoa/AppKitOlderNotes.html#X10_5Notes :

> To help guarantee correct redraw of focus rings, AppKit may automatically draw
> additional parts of a window beyond those that application code marked as
> needing display. It may do this for the first responder view in the
> application's key window, if that first responder view's focusRingType property
> is set to a value other than NSFocusRingTypeNone. Any view that can become
> first responder, but doesn't draw a focus ring, should have its focusRingType
> set to NSFocusRingTypeNone to avoid unnecessary additional redraw.

So I added [self setFocusRingType:NSFocusRingTypeNone] to ChildView's init method, but it didn't make any difference.
Playing around some more, I discovered that the additional redraws only started after I had clicked the "Basic Render" button, i.e. only after we rendered a button with a focus ring.
The rest was easy.

When we draw a button with a focus ring, we first call setShowsFirstResponder:YES on the NSButtonCell, and then pass the NSView that contains our button frame to [button drawWithFrame:inView:]. This seems to mark our NSView permanently.

We can avoid that by simply using a pseudo view instead of the real view - whether the used view actually contains our button doesn't matter, as the investigation in bug 465069 has shown.
So that's what my patch does. I'm also adding setFocusRingType: NSFocusRingTypeNone (even though I couldn't see a difference) because Webkit does it, too.

Comment 1

[Josh Aas]

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

Looks good but please file a bug with Apple about NSFocusRingTypeNone not stopping the invalidations and record the Apple bug number here.

Comment 2

[Josh Aas]

this is potential perf worth blocking on, blocking1.9.2+

Comment 3

[Markus Stange [:mstange]]

This is a regression from bug 450800.

Comment 4

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/697b2063d06f

Comment 5

[Markus Stange [:mstange]]

Filed rdar://problem/7229728

Comment 6

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/25e1253030f4

Comment 7

[Markus Stange [:mstange]]

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

I think we can also take this on 1.9.1 - it's a regression from 1.9.0 and the fix is reasonably safe. I looked into writing a test for this, but I'm afraid doing so seems impossible without bug 510970.

Comment 8

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.1.5, a=dveditz for release-drivers

Comment 9

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c4bdcb43345