Avoid clearing of Find In Page during BACK key press

Avoid clearing of Find In Page during BACK key press

This change takes care of avoiding the removal of Find In
Page toolbar when pressing BACK key in the presence of IME.
IME will be cleared first. Second BACK Key will clear the
Find In Page toolbar.

BUG=564368
Committed: https://crrev.com/b67b82af1eecc41bb80f51cdaddc0a129540c8b9
Cr-Commit-Position: refs/heads/master@{#401377}

[AKV, 2016-05-03 17:17:08]

ajith.v@chromium.org changed reviewers:
+ yfriedman@chromium.org

[AKV, 2016-05-03 17:17:09]

PTAL!

[Yaron, 2016-05-03 17:52:09]

yfriedman@chromium.org changed reviewers:
+ changwan@chromium.org

[Yaron, 2016-05-03 17:52:10]

I think changwan is a better reviewer for this but he OOO for the rest of the
week so this'll have to wait a bit

[Changwan Ryu, 2016-05-09 09:43:01]

could you also add a new test to FindTest.java?

https://codereview.chromium.org/1947603002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java
(right):

https://codereview.chromium.org/1947603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java:723:
if (mFindQuery.hasWindowFocus() && UiUtils.isKeyboardShowing(getContext(),
mFindQuery)) {
Please avoid using UiUtils#isKeyboardShowing() at all cost. It is a hacky method
at best, and may not work in some corner cases.

onKeyPreIme() was added 5 years ago, but I don't see the point of overriding it
for this class. (The same applies to UrlBar, AppMenu, BookmarkSearchView.)

Could you try removing the current onKeyPreIme() implementation and adding
onKeyUp() to handle KEYCODE_BACK event to dismiss deactivate FindToolbar? See
AppMenu#onKey() for reference.

That way, we can give on-screen keyboard a chance to handle KEYCODE_BACK event
before us, and dismiss FindToolbar only when OSK is already hidden. In case of
physical (bluetooth) keyboard, it should dismiss FindToolbar right away.

[Changwan Ryu, 2016-05-09 10:06:23]

changwan@chromium.org changed reviewers:
+ johnme@chromium.org

[Changwan Ryu, 2016-05-09 10:06:23]

johnme@, could you explain why we had to override onKeyPreIme() instead of
overriding onKeyUp() (and possibly onKeyDown())? Is the reason still valid?

[johnme, 2016-05-10 16:28:49]

On 2016/05/09 10:06:23, Changwan Ryu wrote:
> johnme@, could you explain why we had to override onKeyPreIme() instead of
> overriding onKeyUp() (and possibly onKeyDown())? Is the reason still valid?

I can't remember, possibly because KEYCODE_BACK never got delivered to those
events because it was handled by something else? But if it's possible to listen
for it in onKeyUp, and that only receives the event if the keyboard has already
been dismissed, that would be ideal.

Would need to make sure we properly handle the keyboard hiding; clearly
hideKeyboardAndStartFinding supports the case where we hide the keyboard
ourselves, but I don't know offhand how well the FiP UI handles the keyboard
hiding by itself (for example do the match rects in the scrollbar get adjusted
for the increased window size?)

[Changwan Ryu, 2016-05-11 02:24:59]

On 2016/05/10 16:28:49, johnme wrote:
> On 2016/05/09 10:06:23, Changwan Ryu wrote:
> > johnme@, could you explain why we had to override onKeyPreIme() instead of
> > overriding onKeyUp() (and possibly onKeyDown())? Is the reason still valid?
> 
> I can't remember, possibly because KEYCODE_BACK never got delivered to those
> events because it was handled by something else? But if it's possible to
listen
> for it in onKeyUp, and that only receives the event if the keyboard has
already
> been dismissed, that would be ideal.
> 
> Would need to make sure we properly handle the keyboard hiding; clearly
> hideKeyboardAndStartFinding supports the case where we hide the keyboard
> ourselves, but I don't know offhand how well the FiP UI handles the keyboard
> hiding by itself (for example do the match rects in the scrollbar get adjusted
> for the increased window size?)

Thanks for the answer, johnme@.
AKV, could you try my suggestions in #5 and see if there is any anomaly, e.g.
match rects get lost or not drawn properly?

[AKVT, 2016-05-18 01:49:40]

On 2016/05/11 02:24:59, Changwan Ryu wrote:
> On 2016/05/10 16:28:49, johnme wrote:
> > On 2016/05/09 10:06:23, Changwan Ryu wrote:
> > > johnme@, could you explain why we had to override onKeyPreIme() instead of
> > > overriding onKeyUp() (and possibly onKeyDown())? Is the reason still
valid?
> > 
> > I can't remember, possibly because KEYCODE_BACK never got delivered to those
> > events because it was handled by something else? But if it's possible to
> listen
> > for it in onKeyUp, and that only receives the event if the keyboard has
> already
> > been dismissed, that would be ideal.
> > 
> > Would need to make sure we properly handle the keyboard hiding; clearly
> > hideKeyboardAndStartFinding supports the case where we hide the keyboard
> > ourselves, but I don't know offhand how well the FiP UI handles the keyboard
> > hiding by itself (for example do the match rects in the scrollbar get
adjusted
> > for the increased window size?)
> 
> Thanks for the answer, johnme@.
> AKV, could you try my suggestions in #5 and see if there is any anomaly, e.g.
> match rects get lost or not drawn properly?

Thank you Changwan. Currently I am OOO for few days. I will check this as soon
as I reach office.

[AKV, 2016-06-09 14:39:35]

PTAL!

https://codereview.chromium.org/1947603002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java
(right):

https://codereview.chromium.org/1947603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java:723:
if (mFindQuery.hasWindowFocus() && UiUtils.isKeyboardShowing(getContext(),
mFindQuery)) {
On 2016/05/09 09:43:01, Changwan Ryu wrote:
> Please avoid using UiUtils#isKeyboardShowing() at all cost. It is a hacky
method
> at best, and may not work in some corner cases.
> 
> onKeyPreIme() was added 5 years ago, but I don't see the point of overriding
it
> for this class. (The same applies to UrlBar, AppMenu, BookmarkSearchView.)
> 
> Could you try removing the current onKeyPreIme() implementation and adding
> onKeyUp() to handle KEYCODE_BACK event to dismiss deactivate FindToolbar? See
> AppMenu#onKey() for reference.
> 
> That way, we can give on-screen keyboard a chance to handle KEYCODE_BACK event
> before us, and dismiss FindToolbar only when OSK is already hidden. In case of
> physical (bluetooth) keyboard, it should dismiss FindToolbar right away.

Done.

[Changwan Ryu, 2016-06-13 01:32:02]

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java
(right):

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java:107:
if (keyCode == KeyEvent.KEYCODE_BACK && event.getAction() == KeyEvent.ACTION_UP)
{
I think you still need key dispatcher state to track ACTION_DOWN correctly.
Otherwise, there is a chance that the container or framework may end up handling
this in a half-baked fashion.

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java
(right):

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:348:
public void testFindNotDismissedWithBackKey() throws InterruptedException {
could you rename this to testBackKeyDoesNotDismissFindWhenImeIsPresent
and add testBackKeyDismissesFind? We need to test both cases.

[AKV, 2016-06-13 16:11:08]

PTAL!

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java
(right):

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/findinpage/FindToolbar.java:107:
if (keyCode == KeyEvent.KEYCODE_BACK && event.getAction() == KeyEvent.ACTION_UP)
{
On 2016/06/13 01:32:02, Changwan Ryu wrote:
> I think you still need key dispatcher state to track ACTION_DOWN correctly.
> Otherwise, there is a chance that the container or framework may end up
handling
> this in a half-baked fashion.

Ok I think theoretically/practically that's correct. I will change it. Thank you
:)

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java
(right):

https://codereview.chromium.org/1947603002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:348:
public void testFindNotDismissedWithBackKey() throws InterruptedException {
On 2016/06/13 01:32:02, Changwan Ryu wrote:
> could you rename this to testBackKeyDoesNotDismissFindWhenImeIsPresent
> and add testBackKeyDismissesFind? We need to test both cases.

Ok. I will add other test as well.

[Changwan Ryu, 2016-06-13 23:39:32]

changwan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Changwan Ryu, 2016-06-13 23:39:33]

lgtm

Adding tedchoc@ for approval

[Ted C, 2016-06-13 23:55:19]

will defer to changwan@ about whether this is necessary...just looks a bit
"fishy" to me

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java
(right):

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:353:
// IME is present at this moment, so IME will consume BACK key.
should we wait for UiUtils.isKeyboardShowing here before proceeding?  in general
any time we are waiting for IME state changes, should we be doing that?

[Changwan Ryu, 2016-06-14 00:22:19]

There were two chromium accounts requesting fix of this, so I think this is a
necessary change.

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java
(right):

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:353:
// IME is present at this moment, so IME will consume BACK key.
On 2016/06/13 23:55:19, Ted C wrote:
> should we wait for UiUtils.isKeyboardShowing here before proceeding?  in
general
> any time we are waiting for IME state changes, should we be doing that?

+1

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:370:
KeyUtils.singleKeyEventView(getInstrumentation(), findQueryText,
KeyEvent.KEYCODE_A);
here, too

[AKV, 2016-06-14 14:49:59]

PTAL!

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java
(right):

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:353:
// IME is present at this moment, so IME will consume BACK key.
On 2016/06/14 00:22:19, Changwan Ryu wrote:
> On 2016/06/13 23:55:19, Ted C wrote:
> > should we wait for UiUtils.isKeyboardShowing here before proceeding?  in
> general
> > any time we are waiting for IME state changes, should we be doing that?
> 
> +1

Done.

https://codereview.chromium.org/1947603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/findinpage/FindTest.java:370:
KeyUtils.singleKeyEventView(getInstrumentation(), findQueryText,
KeyEvent.KEYCODE_A);
On 2016/06/14 00:22:19, Changwan Ryu wrote:
> here, too

Done.

[AKV, 2016-06-22 14:56:14]

friendly ping! PTAL!

[Ted C, 2016-06-22 17:59:57]

lgtm

[AKV, 2016-06-22 18:04:27]

The CQ bit was checked by ajith.v@chromium.org

[AKV, 2016-06-22 18:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/1947603002/#ps60001
(title: "Refined tests and fixed review comments.")

[commit-bot: I haz the power, 2016-06-22 18:05:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1947603002/60001

[commit-bot: I haz the power, 2016-06-22 19:05:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-22 19:07:22]

Description was changed from

==========
Avoid clearing of Find In Page during BACK key press

This change takes care of avoiding the removal of Find In
Page toolbar when pressing BACK key in the presence of IME.
IME will be cleared first. Second BACK Key will clear the
Find In Page toolbar.

BUG=564368
==========

to

==========
Avoid clearing of Find In Page during BACK key press

This change takes care of avoiding the removal of Find In
Page toolbar when pressing BACK key in the presence of IME.
IME will be cleared first. Second BACK Key will clear the
Find In Page toolbar.

BUG=564368

Committed: https://crrev.com/b67b82af1eecc41bb80f51cdaddc0a129540c8b9
Cr-Commit-Position: refs/heads/master@{#401377}
==========

[commit-bot: I haz the power, 2016-06-22 19:07:23]

Patchset 4 (id:??) landed as
https://crrev.com/b67b82af1eecc41bb80f51cdaddc0a129540c8b9
Cr-Commit-Position: refs/heads/master@{#401377}