Last Comment Bug 516924 - Cocoa unnecessarily repaints our views as soon as we paint a focus ring
: Cocoa unnecessarily repaints our views as soon as we paint a focus ring
: regression
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
-- normal (vote)
: mozilla1.9.3a1
Assigned To: Markus Stange [:mstange]
: Markus Stange [:mstange]
Depends on: 663340
Blocks: 424715 450800 497514 516732 516740
  Show dependency treegraph
Reported: 2009-09-16 02:53 PDT by Markus Stange [:mstange]
Modified: 2011-06-10 02:21 PDT (History)
6 users (show)
jaas: blocking1.9.2+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (10.67 KB, patch)
2009-09-16 02:53 PDT, Markus Stange [:mstange]
jaas: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Description User image Markus Stange [:mstange] 2009-09-16 02:53:07 PDT
Created attachment 400976 [details] [diff] [review]

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 :

> 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 User image Josh Aas 2009-09-16 06:17:28 PDT
Comment on attachment 400976 [details] [diff] [review]

Looks good but please file a bug with Apple about NSFocusRingTypeNone not stopping the invalidations and record the Apple bug number here.
Comment 2 User image Josh Aas 2009-09-16 10:21:43 PDT
this is potential perf worth blocking on, blocking1.9.2+
Comment 3 User image Markus Stange [:mstange] 2009-09-16 12:33:34 PDT
This is a regression from bug 450800.
Comment 4 User image Markus Stange [:mstange] 2009-09-16 15:08:32 PDT
Comment 5 User image Markus Stange [:mstange] 2009-09-16 15:30:38 PDT
Filed rdar://problem/7229728
Comment 6 User image Markus Stange [:mstange] 2009-09-20 16:26:08 PDT
Comment 7 User image Markus Stange [:mstange] 2009-09-20 16:27:52 PDT
Comment on attachment 400976 [details] [diff] [review]

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 User image Daniel Veditz [:dveditz] 2009-10-21 15:55:17 PDT
Comment on attachment 400976 [details] [diff] [review]

Approved for, a=dveditz for release-drivers
Comment 9 User image Markus Stange [:mstange] 2009-10-28 12:31:59 PDT

Note You need to log in before you can comment on or make changes to this bug.