Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla1.9.3a1



Widget: Cocoa
8 years ago
6 years ago


(Reporter: mstange, Assigned: mstange)


(Depends on: 1 bug, {regression})

Mac OS X
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .6-fixed)




(1 attachment)



8 years ago
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.
Flags: blocking1.9.2?
Attachment #400976 - Flags: review?(joshmoz)


8 years ago
Blocks: 516732


8 years ago
Blocks: 424715

Comment 1

8 years ago
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.
Attachment #400976 - Flags: review?(joshmoz) → review+

Comment 2

8 years ago
this is potential perf worth blocking on, blocking1.9.2+
Flags: blocking1.9.2? → blocking1.9.2+

Comment 3

8 years ago
This is a regression from bug 450800.
Blocks: 450800

Comment 4

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED


8 years ago
Keywords: regression
Target Milestone: --- → mozilla1.9.3a1

Comment 5

8 years ago
Filed rdar://problem/7229728

Comment 6

8 years ago
status1.9.2: --- → beta1-fixed

Comment 7

8 years ago
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.
Attachment #400976 - Flags: approval1.9.1.4?
Attachment #400976 - Flags: approval1.9.1.4? → approval1.9.1.5?
Comment on attachment 400976 [details] [diff] [review]

Approved for, a=dveditz for release-drivers
Attachment #400976 - Flags: approval1.9.1.5? → approval1.9.1.5+

Comment 9

8 years ago
status1.9.1: --- → .5-fixed


8 years ago
Blocks: 497514
Depends on: 663340
You need to log in before you can comment on or make changes to this bug.