views: update textfield cursor state after programmatic text changes

views: update textfield cursor state after programmatic text changes

Subclasses of Textfield (specifically OmniboxView) use these methods to implement
their own paste semantics and so on. They should not be responsible for remembering
to call UpdateAfterChange() after doing so, so call it in mutator methods here.

BUG=651016

[Elly Fong-Jones, 2016-09-28 14:48:06]

Description was changed from

==========
views: update textfield cursor state after programmatic text changes

Subclasses of Textfield (specifically OmniboxView) use these methods to
implement
their own paste semantics and so on. They should not be responsible for
remembering
to call UpdateAfterChange() after doing so, so call it in mutator methods here.

BUG=651016
==========

to

==========
views: update textfield cursor state after programmatic text changes

Subclasses of Textfield (specifically OmniboxView) use these methods to
implement
their own paste semantics and so on. They should not be responsible for
remembering
to call UpdateAfterChange() after doing so, so call it in mutator methods here.

BUG=651016
==========

[Elly Fong-Jones, 2016-09-28 14:48:06]

ellyjones@chromium.org changed reviewers:
+ pkasting@chromium.org

[Elly Fong-Jones, 2016-09-28 14:48:31]

pkasting: ptal? :)

[Peter Kasting, 2016-09-28 18:56:47]

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:310: UpdateAfterChange(true, true);
UpdateAfterChange() calls OnCaretBoundsChanged(), SchedulePaint(), and
NotifyAccessibilityEvent(), so those calls are now unnecessary (and, in the
accessibility case, probably outright wrong).  (multiple places)

This will now notify the controller about the text contents changing, where it
didn't before.  Do we need to worry about this, e.g. some kind of infinite loop
or double-handling of changes because whoever is calling us expected not to be
notified?

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:320: UpdateAfterChange(true, true);
The next two functions now trigger accessibility notifications that didn't
before.  What effect will this have on e.g. screen readers?  This may be a
bugfix, or we may now get annoying notifications we didn't care about.

[Elly Fong-Jones, 2016-10-03 17:37:30]

pkasting: ptal? :)

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:310: UpdateAfterChange(true, true);
On 2016/09/28 18:56:47, Peter Kasting wrote:
> UpdateAfterChange() calls OnCaretBoundsChanged(), SchedulePaint(), and
> NotifyAccessibilityEvent(), so those calls are now unnecessary (and, in the
> accessibility case, probably outright wrong).  (multiple places)

Done.

> This will now notify the controller about the text contents changing, where it
> didn't before.  Do we need to worry about this, e.g. some kind of infinite
loop
> or double-handling of changes because whoever is calling us expected not to be
> notified?

I don't know. The behavior isn't documented one way or the other.

Looking at the callsites for these functions:

1) ::SetText() is used in a zillion places. It's probably not feasible to check
each callsite.
2) ::AppendText() is used only in Views unittests, so if it's broken, we'll find
out from the tests.
3) ::InsertOrReplaceText() is used in only one place:
OmniboxViewViews::OnPaste(). I looked at that code and tested it, and it seems
to work okay.

So, I'm not sure what to do about SetText(). Maybe we don't need to
UpdateAfterChange() there, but I don't know how to find out.

https://codereview.chromium.org/2373403002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:320: UpdateAfterChange(true, true);
On 2016/09/28 18:56:47, Peter Kasting wrote:
> The next two functions now trigger accessibility notifications that didn't
> before.  What effect will this have on e.g. screen readers?  This may be a
> bugfix, or we may now get annoying notifications we didn't care about.

I don't know. There are several screenreaders, all with different behavior and
handling of different notifications, so it's just about impossible to say for
certain whether this change is good or bad. :\

[Peter Kasting, 2016-10-04 07:56:48]

On 2016/10/03 17:37:30, Elly Fong-Jones wrote:
> pkasting: ptal? :)

Will try to get to this Tue 10/4

[Peter Kasting, 2016-10-05 00:34:33]

pkasting@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Peter Kasting, 2016-10-05 00:34:33]

LGTM, but I'm definitely concerned about the accessibility effect.

I suggest asking dmazzoni (added as a reviewer) whether there's a procedure or
some well-defined QA help where we can be sure we don't cause obvious a11y
regressions (in this case, extra "something changed" notifications) when
interacting with Chrome UI textfields.  Hopefully QA already has some tests for
this sort of thing they can run.

[dmazzoni, 2016-10-05 17:52:21]

lgtm

This seems like a good change, it's definitely
better to consolidate the logic in one place
rather than spreading it out, which is more
error-prone.

I'll ask kathrelkeld@ to do a bit of extra QA
on Chrome OS, and I'll take a quick look at
Windows.

Luckily I think this is the sort of thing we'll
hear about quickly if it breaks. It's the
most obscure UI controls that are the ones I
worry about reaching stable.

[Elly Fong-Jones, 2016-10-05 17:53:08]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-10-05 17:53:37]

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-05 18:26:29]

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

[commit-bot: I haz the power, 2016-10-05 18:26:30]

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

[Elly Fong-Jones, 2016-10-06 13:27:46]

On 2016/10/05 18:26:30, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

There are two failures happening here:

1) A couple of the tests assert that the model does *not* get callbacks when the
programmatic setters are used. That's disturbing.
2) A lot of the bookmark editor tests explode because their controller assumes
it's only called via user input, meaning the UI is already visible.

So, I think this CL is fundamentally a mistake and will probably subtly break
other stuff / introduce a lot of crashes. Instead, what I'm going to do is have
the omnibox's ::OnPaste (which is overriding the textfield's OnPaste) call
UpdateAfterChange() itself, which means making that function protected instead
of private.

[Peter Kasting, 2016-10-10 21:23:02]

On 2016/10/06 13:27:46, Elly Fong-Jones wrote:
> On 2016/10/05 18:26:30, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> There are two failures happening here:
> 
> 1) A couple of the tests assert that the model does *not* get callbacks when
the
> programmatic setters are used. That's disturbing.
> 2) A lot of the bookmark editor tests explode because their controller assumes
> it's only called via user input, meaning the UI is already visible.
> 
> So, I think this CL is fundamentally a mistake and will probably subtly break
> other stuff / introduce a lot of crashes.

The test results suggest your second clause is probably true.

I don't know whether that means the first clause is true.  That is, I would
prefer that we aim for whatever we think the right long-term implementation is,
not aim for whatever we think will be least likely to disrupt existing
expectations of the code.

I do not have a strong opinion of what that right long-term implementation
actually is.  I am reluctant to believe that the omnibox is a special snowflake
and other clients should not get this behavior, but I am not sufficiently
certain of that to say that you are wrong to go the alternate route.

However, please be certain that it's the right thing to do, and keep in mind the
general biases I mention above.  Let's not get to a point someday where a
non-omnibox consumer of textfields has wonky behavior that they wouldn't have
had if we had just Done It Right in textfields to begin with.