[rAC, OSX] Fix Omnibox font issue

[rAC, OSX] Fix Omnibox font issue

Omnibox text can lose text attributes when the origin chip becomes
invisible due to focus loss. This is due to side effects in
EmphasizeURLComponents, which essentially remove all text attributes
from the Omnibox when this routine is called during focus loss.

BUG=347316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255605

[groby-ooo-7-16, 2014-03-05 19:01:32]

PTAL.

[Scott Hess - ex-Googler, 2014-03-06 02:42:24]

Sorry about the delay, I took the day to get my taxes done.

Because on Friday I'm getting my wisdom teeth out.  So I'll try to pay attention
tomorrow.

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
I cannot understand how the comment applies at all.  |text| itself will be
stripped of all attributes, so getting it up here shouldn't do anything at all
WRT the attributes.  What it sounds like is that you want the else{} rather than
the if(){} code path in this case.

AFAICT, the problem is that -currentEditor returns non-nil, but editor has
released first responder, so the -beginEditing/-endEditing stuff isn't
appropriate.  If so, would a better check be if (editor == [[editor window]
firstResponder])?

[groby-ooo-7-16, 2014-03-06 04:07:51]

Longish explanation to follow... (It took me almost two days to figure this out,
so it's somewhat convoluted)

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
On 2014/03/06 02:42:24, shess wrote:
> I cannot understand how the comment applies at all.  |text| itself will be
> stripped of all attributes, so getting it up here shouldn't do anything at all
> WRT the attributes.  What it sounds like is that you want the else{} rather
than
> the if(){} code path in this case.
> 
> AFAICT, the problem is that -currentEditor returns non-nil, but editor has
> released first responder, so the -beginEditing/-endEditing stuff isn't
> appropriate.  If so, would a better check be if (editor == [[editor window]
> firstResponder])?

Unfortunately, it still is the firstResponder, except having released it.  This
is called from within resignFirstResponder, which is called from [window
makeFirstResponder]. And window can't update its firstResponder before it has
gotten the return value from resignFirstResponder. As far as I can tell, there's
no straightforward way to distinguish this state.

The issue with GetText is that it calls [field_ stringValue] - which gets the
editors current attributedString, and updated [field_ attributedStringValue] in
the process. And the previous code has _just_ stripped all attributes off.

So now field_ has a "bare" string. Storage gets updated to be the full string,
but that doesn't matter, becomes we're called from the tail end of
[AutocompleteTextFieldEditor resignFirstResponder], with [NSTextView
resignFirstResponder] already finished. And so there will be no more syncing
from the editors proper value to the fields attributedStringValue.

You could also fix this by just calling [field_ attributedStringValue] after
-endEditing, forcing a sync - but that would break the IME use case which
necessitated this code in the first place.

(I believe the root cause for the IME issue is that OnDidChange is triggered
from interpetKeys, which isn't the proper place to handle this - but I don't
have a proper fix for that yet, and so I need to work around it here)

[Scott Hess - ex-Googler, 2014-03-06 06:30:32]

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
On 2014/03/06 04:07:51, groby wrote:
> The issue with GetText is that it calls [field_ stringValue] - which gets the
> editors current attributedString, and updated [field_ attributedStringValue]
in
> the process. And the previous code has _just_ stripped all attributes off.

Still nack.  base::string16 cannot have Cocoa attributes on it.  I don't mean
it's unlikely, there is no there, there, GetText() doesn't have the scope to
return attributed text.  Either SetText() or ApplyTextAttributes() is what adds
the attributes to an unattributed string, so fetching it earlier shouldn't make
a difference.  I suspect that what's happening is that something in the if
(editor) case is causing a problem because the field editor is being torn town. 
Perhaps it is re-syncing the bare string, but then modifying the |storage|
string which is never re-synced.

I'm going to hate myself if there's an obvious thing sitting right in front of
me, here.  I just can't see how the comment relates.  The attributes of the text
before this call shouldn't matter, it's the attributes _after_ this call which
matter.

I would look to see if one of these cases could catch the case:
 - ![storage length]
 - [editor delegate] != field_
 - ![editor window]
and then modify it to do the else{} in that case (so that it modifies the field
value not the editor value).

[groby-ooo-7-16, 2014-03-06 19:07:57]

Now with pseudo-stacktraces, in hopes of being clearer.

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
On 2014/03/06 06:30:32, shess wrote:
> On 2014/03/06 04:07:51, groby wrote:
> > The issue with GetText is that it calls [field_ stringValue] - which gets
the
> > editors current attributedString, and updated [field_ attributedStringValue]
> in
> > the process. And the previous code has _just_ stripped all attributes off.
> 
> Still nack.  base::string16 cannot have Cocoa attributes on it.  I don't mean
> it's unlikely, there is no there, there, GetText() doesn't have the scope to
> return attributed text. 

It doesn't. It's a side effect. Sequence of events, with indentation for
callstack levels

-[AutocompleteTextFieldEditor -resignFirstResponder] is called
   It calls [super resignFirstResponder]
       That calls a private method to sync field values between editor and
NSTextField
   Now ACTFE calls OnFocusLost
       That calls EmphasizeURLComponents
           Reset the storages text attributes
           Call GetText in preparation for ApplyTextAttributes
               GetText calls [field_ stringValue]
                   stringValue syncs the editor and the text field, both string
and attributedString 
                   (since resignFirstResponder hasn't completed, the field has
still an editor)
                   As result, field's attributed string has gotten the editor's
string, which just got stripped
           EmphasizeURLComponents applies attributes, updating storage only
    ACTFE's resignFirstResponder is now done
AppKit removes the editor and sets a new first responder, _without_ updating
field's values from the editor.

At that point, NSTextField has an attributedStringValue without any attributes,
and the editor is gone.

> Either SetText() or ApplyTextAttributes() is what adds
> the attributes to an unattributed string, so fetching it earlier shouldn't
make
> a difference.  I suspect that what's happening is that something in the if
> (editor) case is causing a problem because the field editor is being torn
town. 
> Perhaps it is re-syncing the bare string, but then modifying the |storage|
> string which is never re-synced.

Yes, that's exactly what's happening. And the re-syncing is caused by GetText().
I've got the stack traces to prove it ;)
> 
> I'm going to hate myself if there's an obvious thing sitting right in front of
> me, here.  I just can't see how the comment relates.  The attributes of the
text
> before this call shouldn't matter, it's the attributes _after_ this call which
> matter.

Hm. How would you word the comment then, in light of what I explained above. I
_thought_ the comment made that somewhat clear, but obviously it's misleading.

> I would look to see if one of these cases could catch the case:
>  - ![storage length]
>  - [editor delegate] != field_
>  - ![editor window]

None of these do. The editor is still valid - AppKit tears it down _after_
resignFirstResponder.

> and then modify it to do the else{} in that case (so that it modifies the
field
> value not the editor value).

I've looked for all sorts of conditions, but short of setting a flag in ACTFE's
resignFirstResponder and querying it here, there's no way to do that. (That I
can find)

[Scott Hess - ex-Googler, 2014-03-06 19:28:12]

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
On 2014/03/06 19:07:58, groby wrote:
>                GetText calls [field_ stringValue]
>                    stringValue syncs the editor and the text field, both
string
> and attributedString 
>                    (since resignFirstResponder hasn't completed, the field has
> still an editor)
>                    As result, field's attributed string has gotten the
editor's
> string, which just got stripped

This implies that calling [field_ stringValue] after -endEditing would re-sync
the right thing.

[Brevity of comment because I wanted to suggest that before joining the group
leaving for lunch.  I'll comment on the comment commenting when I'm back.]

[Scott Hess - ex-Googler, 2014-03-06 20:54:27]

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
On 2014/03/06 19:07:58, groby wrote:
> Hm. How would you word the comment then, in light of what I explained above. I
> _thought_ the comment made that somewhat clear, but obviously it's misleading.

OK, I'm reading old CLs trying to wrap my head around everything.  AFAICT, this
exists to do our annotations in-place over possibly-marked text (which only
exists in the editor, not the control).  So we cannot do anything which would
push in that direction.

The core problem is that [field_ stringValue] pulls the current data from editor
to control, and at the point where ApplyTextAttributes() is called, the
attributes have just been cleared.  The editor is then attributed, but nobody
ever pulls the data over to the field again.

So I think that calling GetText() up top merely retains the pre-existing
attributes, but it still throws away the attributes in ApplyTextAttributes().  I
can't really think of a case where a user event could make a change that won't
have already been synced, but it still seems subtly incorrect because our code
could make such a change.

I think if calling -stringValue after -endEditing works, that would probably
make the most sense.  Then that call needs a local comment, but it's pretty
straightforward about making sure the editor data is sync'ed to the control
(perhaps with a kicker that -resignFirstResponder is a case which doesn't
otherwise guarantee this, so that nobody decides it's unnecessary).

[Every time I try to phrase a comment for the existing code, I end up thinking
that just making the case not happen would be better.]

[groby-ooo-7-16, 2014-03-07 01:47:08]

OK, changed according to your suggestion.

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/184783007/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:409: base::string16 text =
GetText();
> OK, I'm reading old CLs trying to wrap my head around everything.  AFAICT,
this
> exists to do our annotations in-place over possibly-marked text (which only
> exists in the editor, not the control).  So we cannot do anything which would
> push in that direction.

Based on my reading of old CL's, we cannot call
setAttributedStringValue/setStringValue because it messed up IME events in
progress. (I'm not sure why -stringValue still works, but it seems to)

> The core problem is that [field_ stringValue] pulls the current data from
editor
> to control, and at the point where ApplyTextAttributes() is called, the
> attributes have just been cleared.  The editor is then attributed, but nobody
> ever pulls the data over to the field again.

Yep.

> So I think that calling GetText() up top merely retains the pre-existing
> attributes, but it still throws away the attributes in ApplyTextAttributes(). 
I
> can't really think of a case where a user event could make a change that won't
> have already been synced,

This corner case _only_ happens during resignFirstResponder. Any key events have
already invoked EmphasizeURL and set the proper attributes for user input.

> but it still seems subtly incorrect because our code
> could make such a change.

This code is still equivalent to the code we had before - we just get the
stringValue a bit earlier. If there was any editing case that lost attributes,
we'd already have seen it.

> I think if calling -stringValue after -endEditing works, that would probably
> make the most sense.  Then that call needs a local comment, but it's pretty
> straightforward about making sure the editor data is sync'ed to the control

I can try that. I'm worried about IME interactions, but that will always be a
worry.

> (perhaps with a kicker that -resignFirstResponder is a case which doesn't
> otherwise guarantee this, so that nobody decides it's unnecessary).
> 
> [Every time I try to phrase a comment for the existing code, I end up thinking
> that just making the case not happen would be better.]

I think I have an inkling of how to do it, but I don't have the time to
investigate. If I understand Cocoa's text handling correctly, we don't want to
intercept interpretKeys for the OnChanged event - because those happen for every
key stroke, and thus in the middle of IME compositing. Instead, we only care if
a glyph is added, which would be insertText. That would, theoretically, obviate
the need to even account for IME issues, so all this would be is
SetText(GetText()).

[groby-ooo-7-16, 2014-03-07 01:48:50]

Oops. Removed two extraneous lines

[Scott Hess - ex-Googler, 2014-03-07 02:04:16]

lgtm

[Scott Hess - ex-Googler, 2014-03-07 02:06:27]

On 2014/03/07 01:47:08, groby wrote:
> I think I have an inkling of how to do it, but I don't have the time to
> investigate. If I understand Cocoa's text handling correctly, we don't want to
> intercept interpretKeys for the OnChanged event - because those happen for
every
> key stroke, and thus in the middle of IME compositing. Instead, we only care
if
> a glyph is added, which would be insertText. That would, theoretically,
obviate
> the need to even account for IME issues, so all this would be is
> SetText(GetText()).

There were cases where you starting from a composing-character case, entering a
new character could commit the current character and start composing a new one. 
But there are definitely many ways to skin a cat.  I mean I imagine there are.

[groby-ooo-7-16, 2014-03-07 02:43:25]

Hence my "didn't have time to investigate" comment. It looks like a deceptively
simple fix, which means it will be a 2+ week rabbit hole ;)

[groby-ooo-7-16, 2014-03-07 02:43:29]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-03-07 02:47:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/184783007/120001

[commit-bot: I haz the power, 2014-03-07 13:30:59]

Change committed as 255605