[Mac] Preserve original selection when suggesting completions with diacritics

Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883
Committed: https://crrev.com/8a8bae9fbc2e72afc208a0376c0f4a49388a6da3
Cr-Commit-Position: refs/heads/master@{#425671}

[lgrey, 2016-10-10 20:56:51]

Description was changed from

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=
==========

to

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883
==========

[lgrey, 2016-10-11 21:15:43]

lgrey@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-10-12 21:03:47]

asvitkine@chromium.org changed reviewers:
+ erikchen@chromium.org

[Alexei Svitkine (slow), 2016-10-12 21:03:47]

Sorry. I'm not a good reviewer for this - I don't work on Mac code very much
these days.

I looked at chrome/browser/ui/cocoa/location_bar/OWNERS and unfortunately
neither are the three people listed there - as I think none of them actively
work on Mac code anymore either.

I'm redirecting the review to erikchen@ for now, but happy to provide a
rubber-stamp for OWNERS if Erik is happy with this.

(Also, it seems the Mac UI owners files need to be revamped with the current
team that works on Mac.)

[erikchen, 2016-10-12 22:37:48]

Thanks for looking into this. Really nice tests. :)

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:58: //
TODO(lgrey): Should the break iterator be cached?
sounds like premature optimization to me.

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:64: while
(range.location < text.length() &&
interesting. This works because NSString's internal representation uses UTF-16
code units.
https://developer.apple.com/reference/foundation/nsstring

I wasn't aware of that. Do you think it's worth adding a comment here?

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:527:
return actualSelectionRange_;
Is there any way we can avoid overriding this method? 

NSTextView has a lot of internal caching, and it's not clear to me that if we
override this method that all the caches also get updated appropriately.

[lgrey, 2016-10-13 15:42:52]

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:58: //
TODO(lgrey): Should the break iterator be cached?
On 2016/10/12 22:37:48, erikchen wrote:
> sounds like premature optimization to me.

Removed comment

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:64: while
(range.location < text.length() &&
On 2016/10/12 22:37:48, erikchen wrote:
> interesting. This works because NSString's internal representation uses UTF-16
> code units.
> https://developer.apple.com/reference/foundation/nsstring
> 
> I wasn't aware of that. Do you think it's worth adding a comment here?

Added a comment. I could also change it to |string.length|, but that still
requires the explanation, since the break iterator is operating on a string16.
What do you think?

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:527:
return actualSelectionRange_;
On 2016/10/12 22:37:48, erikchen wrote:
> Is there any way we can avoid overriding this method? 
> 
> NSTextView has a lot of internal caching, and it's not clear to me that if we
> override this method that all the caches also get updated appropriately. 

We could expose a new method like |actualRange| or similar, but then we'd need
to do a cast here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/omnibox/omnibox_...
and here
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/aut...

alternatively, the implementation could be something like:

NSRange visualRange = [super selectedRange];
if (!NSEqualRanges(visualRange, visualSelectionRange_)) {
  visualSelectionRange_ = actualSelectionRange_ = visualRange;
}
return actualSelectionRange_

Would that address the caching issue or is it at another level?

[erikchen, 2016-10-13 17:08:28]

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:64: while
(range.location < text.length() &&
On 2016/10/13 15:42:52, lgrey wrote:
> On 2016/10/12 22:37:48, erikchen wrote:
> > interesting. This works because NSString's internal representation uses
UTF-16
> > code units.
> > https://developer.apple.com/reference/foundation/nsstring
> > 
> > I wasn't aware of that. Do you think it's worth adding a comment here?
> 
> Added a comment. I could also change it to |string.length|, but that still
> requires the explanation, since the break iterator is operating on a string16.
> What do you think?

Comments seems good enough.

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:527:
return actualSelectionRange_;
On 2016/10/13 15:42:51, lgrey wrote:
> On 2016/10/12 22:37:48, erikchen wrote:
> > Is there any way we can avoid overriding this method? 
> > 
> > NSTextView has a lot of internal caching, and it's not clear to me that if
we
> > override this method that all the caches also get updated appropriately. 
> 
> We could expose a new method like |actualRange| or similar, but then we'd need
> to do a cast here:
If the only reason this code is necessary is to propagate information to other
Chrome classes, then we should definitely make a new method and do casts as
necessary.

>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/omnibox/omnibox_...
> and here
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/aut...
> 
> alternatively, the implementation could be something like:
> 
> NSRange visualRange = [super selectedRange];
> if (!NSEqualRanges(visualRange, visualSelectionRange_)) {
>   visualSelectionRange_ = actualSelectionRange_ = visualRange;
> }
> return actualSelectionRange_
> 
> Would that address the caching issue or is it at another level?
No. My concern is that the internal implementation of NSTextView [which is very
complex and uses caching] could easily have code that calls [self selectedRange]
and assumes that this is the same value that got cached in the earlier call to
setSelectedRange:... Overriding getters to return a different value than the one
passed to the setter is really scary. I would be willing to go great lengths to
avoid that in general.

[lgrey, 2016-10-13 21:03:56]

PTAL :)

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:527:
return actualSelectionRange_;
On 2016/10/13 17:08:28, erikchen wrote:
> On 2016/10/13 15:42:51, lgrey wrote:
> > On 2016/10/12 22:37:48, erikchen wrote:
> > > Is there any way we can avoid overriding this method? 
> > > 
> > > NSTextView has a lot of internal caching, and it's not clear to me that if
> we
> > > override this method that all the caches also get updated appropriately. 
> > 
> > We could expose a new method like |actualRange| or similar, but then we'd
need
> > to do a cast here:
> If the only reason this code is necessary is to propagate information to other
> Chrome classes, then we should definitely make a new method and do casts as
> necessary.
> 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/omnibox/omnibox_...
> > and here
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/aut...
> > 
> > alternatively, the implementation could be something like:
> > 
> > NSRange visualRange = [super selectedRange];
> > if (!NSEqualRanges(visualRange, visualSelectionRange_)) {
> >   visualSelectionRange_ = actualSelectionRange_ = visualRange;
> > }
> > return actualSelectionRange_
> > 
> > Would that address the caching issue or is it at another level?
> No. My concern is that the internal implementation of NSTextView [which is
very
> complex and uses caching] could easily have code that calls [self
selectedRange]
> and assumes that this is the same value that got cached in the earlier call to
> setSelectedRange:... Overriding getters to return a different value than the
one
> passed to the setter is really scary. I would be willing to go great lengths
to
> avoid that in general.

Exposed actualSelectedRange_ as a property and used that instead.

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_browsertest.mm (right):

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_browsertest.mm:100:
IN_PROC_BROWSER_TEST_F(OmniboxViewMacBrowserTest, CopyToPasteboardDiacritic) {
This test is basically a proxy for testing GetSelectedRange which is private

[erikchen, 2016-10-13 21:06:47]

Thanks! lgtm

[Alexei Svitkine (slow), 2016-10-13 21:19:43]

lgtm % nits

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:54: if
(range.location >= string.length) {
Nit: No {}'s

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:60: if
(!grapheme_iterator.Init()) {
Nit: No {}'s

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:68: if
(range.length > 0) {
Nit: No {}'s

[lgrey, 2016-10-14 14:20:55]

Thanks!

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:54: if
(range.location >= string.length) {
On 2016/10/13 21:19:43, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:60: if
(!grapheme_iterator.Init()) {
On 2016/10/13 21:19:43, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/2395233005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:68: if
(range.length > 0) {
On 2016/10/13 21:19:43, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

[lgrey, 2016-10-14 14:21:05]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-10-14 14:21:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2395233005/#ps120001
(title: "No braces for single-line ifs")

[commit-bot: I haz the power, 2016-10-14 14:21:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lgrey, 2016-10-14 14:48:32]

The CQ bit was unchecked by lgrey@chromium.org

[lgrey, 2016-10-14 15:01:09]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-10-14 15:01:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2395233005/#ps140001
(title: "Restore lost paren")

[commit-bot: I haz the power, 2016-10-14 15:01:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 16:20:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 16:20:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lgrey, 2016-10-14 18:13:41]

The CQ bit was checked by lgrey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 18:14:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lgrey, 2016-10-14 18:16:05]

PTAL erikchen@, updated to fix a case caught by the trybots (sorry!)

[erikchen, 2016-10-14 18:41:35]

https://codereview.chromium.org/2395233005/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:535:
stillSelecting:(BOOL)flag {
-setSelectedRange:... actually calls -setSelectedRanges:

disassembly pseudocode:
"""
void -[NSTextView setSelectedRange:affinity:stillSelecting:](void * self, void *
_cmd, struct _NSRange arg2, unsigned long long arg3, char arg4) {
    rdx = [NSSelectionArray arrayWithRange:arg2];
    [self setSelectedRanges:rdx affinity:arg4 stillSelecting:r9];
    return;
}
"""

Maybe we should just override the latter?

[lgrey, 2016-10-14 19:26:15]

The CQ bit was checked by lgrey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 19:26:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lgrey, 2016-10-14 19:46:05]

The CQ bit was checked by lgrey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 19:46:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lgrey, 2016-10-14 20:09:13]

The CQ bit was checked by lgrey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 20:09:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 20:35:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:35:24]

Dry run: This issue passed the CQ dry run.

[lgrey, 2016-10-14 20:42:36]

https://codereview.chromium.org/2395233005/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:535:
stillSelecting:(BOOL)flag {
On 2016/10/14 18:41:35, erikchen wrote:
> -setSelectedRange:... actually calls -setSelectedRanges:
> 
> disassembly pseudocode:
> """
> void -[NSTextView setSelectedRange:affinity:stillSelecting:](void * self, void
*
> _cmd, struct _NSRange arg2, unsigned long long arg3, char arg4) {
>     rdx = [NSSelectionArray arrayWithRange:arg2];
>     [self setSelectedRanges:rdx affinity:arg4 stillSelecting:r9];
>     return;
> }
> """
> 
> Maybe we should just override the latter?

How's this?

[erikchen, 2016-10-14 21:06:07]

lgtm with nit

https://codereview.chromium.org/2395233005/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:525:
NSMutableArray* mutableRanges = [ranges mutableCopy];
gotta throw this in a scoped_nsobject to avoid a leak.

[lgrey, 2016-10-17 13:30:28]

https://codereview.chromium.org/2395233005/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://codereview.chromium.org/2395233005/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:525:
NSMutableArray* mutableRanges = [ranges mutableCopy];
On 2016/10/14 21:06:07, erikchen wrote:
> gotta throw this in a scoped_nsobject to avoid a leak.

Thanks, I'm a little rusty on pre-ARC :) Done

[lgrey, 2016-10-17 13:30:42]

The CQ bit was checked by lgrey@chromium.org

[lgrey, 2016-10-17 13:30:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2395233005/#ps240001
(title: "Wrap retained object in scoped_nsobject")

[commit-bot: I haz the power, 2016-10-17 13:30:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 13:57:09]

Description was changed from

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883
==========

to

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883
==========

[commit-bot: I haz the power, 2016-10-17 13:57:10]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-17 13:58:53]

Description was changed from

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883
==========

to

==========
Preserve original selection when suggesting completions with diacritics

When NSTextView is asked to select a range which does not begin on a grapheme
boundary, it expands the selection to the previous boundary. This change
preserves the original selection, then contracts the range sent to
NSTextView to fall on the *next* boundary.

Text editing operations that operate on the selection use the original
selection instead of the visual selection.

Since the omnibox view uses the text view's selected range, the
view's |selectedRange| returns the original range and not the visual
range.
BUG=603883

Committed: https://crrev.com/8a8bae9fbc2e72afc208a0376c0f4a49388a6da3
Cr-Commit-Position: refs/heads/master@{#425671}
==========

[commit-bot: I haz the power, 2016-10-17 13:58:54]

Patchset 13 (id:??) landed as
https://crrev.com/8a8bae9fbc2e72afc208a0376c0f4a49388a6da3
Cr-Commit-Position: refs/heads/master@{#425671}

[lgrey, 2016-10-18 15:22:30]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2426983002/ by lgrey@chromium.org.

The reason for reverting is: This almost definitely caused crbug.com/656972, but
I can't repro the issue. Reverting while I investigate..