[ios] Workaround error affecting 3rd party keyboards and omnibox.

[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host, protocol) when 
!self.editing, but also to hide real UITextField text under the _selection text when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the condition added
below. If we change mobile omnibox to match desktop and also color the omnibox while
self.editing, this workaround will no longer work.

BUG=737589
Review-Url: https://codereview.chromium.org/2956013002
Cr-Commit-Position: refs/heads/master@{#483196}
Committed: https://chromium.googlesource.com/chromium/src/+/dacc85d6208232ec0b2af40786ceec71559fa39c

[justincohen, 2017-06-27 03:57:28]

justincohen@chromium.org changed reviewers:
+ rohitrao@chromium.org

[justincohen, 2017-06-27 03:57:29]

for your consideration.

[justincohen, 2017-06-27 05:21:45]

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

[commit-bot: I haz the power, 2017-06-27 05:22:01]

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, 2017-06-27 05:37:02]

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

[commit-bot: I haz the power, 2017-06-27 05:37:04]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-06-27 13:48:48]

https://codereview.chromium.org/2956013002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right):

https://codereview.chromium.org/2956013002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:465:
GetApplicationContext()->GetApplicationLocale() == "ja") {
This should check the primaryLanguage of the current input, not the application
locale.

https://codereview.chromium.org/2956013002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:467: (!self.editing ||
![self.text isEqualToString:fieldText.string]);
I think I'm ok with this, for two reasons:
1) We set a number of attributes in the omnibox, but the only ones that vary are
the foreground and background colors.  Other attributes (like font, style, etc)
are set for the entire string, and a few (strikethrough) are only set when not
editing.
2) setAttributedText always forces the color of the string when editing, so any
color attributes here would be ignored anyways.

Can you capture the gist of what I wrote above in a comment somewhere?

[justincohen, 2017-06-28 15:19:31]

ptal

[justincohen, 2017-06-28 15:23:40]

Description was changed from

==========
[For discussion] Workaround error affecting 3rd party keyboards and omnibox.

BUG=
==========

to

==========
[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third
party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show
keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host,
protocol) when 
!self.exiting, but also to hide real UITextField text under the _selection text
when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is
different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the
condition added
below. If we change mobile omnibox to match desktop and also color the omnibox
while
self.editing, this workaround will no longer work.

BUG=737589
==========

[rohitrao (ping after 24h), 2017-06-28 16:48:23]

lgtm

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right):

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:463: // The following
bool was introduced to workaround a UIKit bug (://32817402).
BOOL

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:463: // The following
bool was introduced to workaround a UIKit bug (://32817402).
Is this a b/ url?  Should we also link to the radar?

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:469: // protocol) when
!self.exiting, but also to hide real UITextField text under
!self.editing.

Also in the CL description.

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:479: if
(experimental_flags::IsThirdPartyKeyboardWorkaroundEnabled()) {
Is this an expensive call?  How about calling it once in init and caching it?

[justincohen, 2017-06-28 21:36:51]

Description was changed from

==========
[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third
party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show
keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host,
protocol) when 
!self.exiting, but also to hide real UITextField text under the _selection text
when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is
different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the
condition added
below. If we change mobile omnibox to match desktop and also color the omnibox
while
self.editing, this workaround will no longer work.

BUG=737589
==========

to

==========
[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third
party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show
keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host,
protocol) when 
!self.editing, but also to hide real UITextField text under the _selection text
when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is
different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the
condition added
below. If we change mobile omnibox to match desktop and also color the omnibox
while
self.editing, this workaround will no longer work.

BUG=737589
==========

[justincohen, 2017-06-28 21:38:06]

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right):

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:463: // The following
bool was introduced to workaround a UIKit bug (://32817402).
On 2017/06/28 16:48:23, rohitrao (ping after 24h) wrote:
> Is this a b/ url?  Should we also link to the radar?

The crbug url links to the b/ urls.

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:463: // The following
bool was introduced to workaround a UIKit bug (://32817402).
On 2017/06/28 16:48:23, rohitrao (ping after 24h) wrote:
> BOOL

Done.

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:469: // protocol) when
!self.exiting, but also to hide real UITextField text under
On 2017/06/28 16:48:23, rohitrao (ping after 24h) wrote:
> !self.editing.
> 
> Also in the CL description.

Done.

https://codereview.chromium.org/2956013002/diff/40001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:479: if
(experimental_flags::IsThirdPartyKeyboardWorkaroundEnabled()) {
On 2017/06/28 16:48:23, rohitrao (ping after 24h) wrote:
> Is this an expensive call?  How about calling it once in init and caching it?

It's fast, we call base::FeatureList::IsEnabled a ton of times during auto
complete every time you type.

[justincohen, 2017-06-28 21:44:47]

The CQ bit was checked by justincohen@chromium.org

[justincohen, 2017-06-28 21:44:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2956013002/#ps60001
(title: "Update comment")

[commit-bot: I haz the power, 2017-06-28 21:45:18]

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, 2017-06-28 23:34:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498686287447100,
"parent_rev": "9aa44ab0d44d18475363d33873f8160d9022cab1", "commit_rev":
"dacc85d6208232ec0b2af40786ceec71559fa39c"}

[commit-bot: I haz the power, 2017-06-28 23:34:27]

Description was changed from

==========
[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third
party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show
keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host,
protocol) when 
!self.editing, but also to hide real UITextField text under the _selection text
when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is
different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the
condition added
below. If we change mobile omnibox to match desktop and also color the omnibox
while
self.editing, this workaround will no longer work.

BUG=737589
==========

to

==========
[ios] Workaround error affecting 3rd party keyboards and omnibox.

This is an attempted workaround to UIKit bug (://32817402) relating to third
party keyboards 
that check the value of textDocumentProxy.documentContextBeforeInput to show
keyboard
suggestions.  It appears that calling setAttributedText during an 
EditingChangedUIControlEvent somehow triggers this bug. The reason we update the
attributed text here is to change the colors of the omnibox (such as host,
protocol) when 
!self.editing, but also to hide real UITextField text under the _selection text
when self.editing.
Since we will correct the omnibox editing text color anytime |self.text| is
different than 
|fieldText|, it seems it's OK to skip calling self.attributedText during the
condition added
below. If we change mobile omnibox to match desktop and also color the omnibox
while
self.editing, this workaround will no longer work.

BUG=737589

Review-Url: https://codereview.chromium.org/2956013002
Cr-Commit-Position: refs/heads/master@{#483196}
Committed:
https://chromium.googlesource.com/chromium/src/+/dacc85d6208232ec0b2af40786ce...
==========

[commit-bot: I haz the power, 2017-06-28 23:34:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/dacc85d6208232ec0b2af40786ce...