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

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla1.9.3a1
All
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

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

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

8 years ago
Blocks: 516732

Updated

8 years ago
Blocks: 424715

Comment 1

8 years ago
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.
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+
(Assignee)

Comment 3

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

Comment 4

8 years ago
http://hg.mozilla.org/mozilla-central/rev/697b2063d06f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

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

Comment 5

8 years ago
Filed rdar://problem/7229728
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/25e1253030f4
status1.9.2: --- → beta1-fixed
(Assignee)

Comment 7

8 years ago
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.
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]
v1

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

Comment 9

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c4bdcb43345
status1.9.1: --- → .5-fixed
(Assignee)

Updated

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