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>
Created attachment 230059 [details] Fixes the bug
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.
(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 _.
Created attachment 230098 [details] Patch for landing
Comment on attachment 230098 [details] Patch for landing Clearing flags on attachment: 230098 Committed r167770: <http://trac.webkit.org/changeset/167770>
All reviewed patches have been landed. Closing bug.