Text caret disappears in Mail after returning from another application

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Reproduction steps:
1. Run Mail.
2. Create a new blank message
3. Type something into the message
4. Click in Finder.
5. Select Finder>Hide Others (Mail disappears)
6. Click on Mail in the Dock (Mail reappears)

Expected result:
The text caret blinks

Actual result:
The text caret is never rendered :(

<rdar://problem/12328567>

[Ryosuke Niwa]

Created attachment 230059 [details]
Fixes the bug

[Darin Adler]

Comment on attachment 230059 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=230059&action=review

I’m saying review+ but I am not sure about a few aspects of the patch.

> Source/WebKit/mac/WebView/WebView.mm:5227
> +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_windowKeyStateChanged:)
> +            name:NSWindowDidBecomeKeyNotification object:nil];
> +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_windowKeyStateChanged:)
> +            name:NSWindowDidResignKeyNotification object:nil];

This says we want a notification any time *any* window in this app becomes or resigns key, since we pass an object of nil. Is that really what we want? I think we want to pass object:window here, but then we’ll have to retest to be sure this actually fixes the bug.

> Source/WebKit/mac/WebView/WebView.mm:-5335
> -- (void)_windowChangedKeyState

I’m not sure exactly why I overrode this internal AppKit method in 2007 rather than using the notifications. I am concerned that this might have significantly different behavior from the old code in ways that go beyond fixing this bug.

I think that in 2007 this exactly matched how AppKit itself kept controls up to date, so maybe that’s why I did it that way.

> Source/WebKit/mac/WebView/WebView.mm:5342
> +- (void)_windowKeyStateChanged:(NSNotification *)notification

I know WebKit does this all over the place, but it’s dangerous to use an underscore prefix because this could easily conflict with a same-named method inside a future version of AppKit. Given that names without prefixes and names with an underscore prefix are both fair game for both AppKit and for apps who might subclass WebView, we should probably use some kind of crazy wk_ prefix on our internal methods. Sorry, not really specific to this patch, and really ugly so maybe others on the project won’t agree.

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 230059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230059&action=review
> 
> I’m saying review+ but I am not sure about a few aspects of the patch.
> 
> > Source/WebKit/mac/WebView/WebView.mm:5227
> > +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_windowKeyStateChanged:)
> > +            name:NSWindowDidBecomeKeyNotification object:nil];
> > +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_windowKeyStateChanged:)
> > +            name:NSWindowDidResignKeyNotification object:nil];
> 
> This says we want a notification any time *any* window in this app becomes or resigns key, since we pass an object of nil. Is that really what we want? I think we want to pass object:window here, but then we’ll have to retest to be sure this actually fixes the bug.

Oops, fixed that.

> > Source/WebKit/mac/WebView/WebView.mm:-5335
> > -- (void)_windowChangedKeyState
> 
> I’m not sure exactly why I overrode this internal AppKit method in 2007 rather than using the notifications. I am concerned that this might have significantly different behavior from the old code in ways that go beyond fixing this bug.
> 
> I think that in 2007 this exactly matched how AppKit itself kept controls up to date, so maybe that’s why I did it that way.

We can make a more conservative change to observe these two notification as well as overriding _windowChangedKeyState.  But given WebKit2 only uses the notifications, I'd rather stick to the standardized API surface.  We can always add the code back.

> > Source/WebKit/mac/WebView/WebView.mm:5342
> > +- (void)_windowKeyStateChanged:(NSNotification *)notification
> 
> I know WebKit does this all over the place, but it’s dangerous to use an underscore prefix because this could easily conflict with a same-named method inside a future version of AppKit. Given that names without prefixes and names with an underscore prefix are both fair game for both AppKit and for apps who might subclass WebView, we should probably use some kind of crazy wk_ prefix on our internal methods. Sorry, not really specific to this patch, and really ugly so maybe others on the project won’t agree.

Removed the leading _.

[Ryosuke Niwa]

Created attachment 230098 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 230098 [details]
Patch for landing

Clearing flags on attachment: 230098

Committed r167770: <http://trac.webkit.org/changeset/167770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.