[Android] Preserve selection on hide/detach

[Android] Preserve selection on hide/detach

Previously, the selection was cleared and the handles dismissed if
Chrome was backgrounded (via home or task switcher buttons) or the
screen was locked. This breaks any kind of multitasking with an
active selection. Instead, in such cases preserve the selection,
hiding the action bar and restoring as necessary when the view is
reattached or is reshown.

Also expose a programmatic way for preserving the selection when the
view loses focus, which is typically undesirable (tapping omnibox or
switching tabs), but not always.

BUG=406925
Committed: https://crrev.com/6a4d1490ab6b5146a3dedd469d49a52045ce9194
Cr-Commit-Position: refs/heads/master@{#294082}

[jdduke (slow), 2014-08-27 22:44:45]

jdduke@chromium.org changed reviewers:
+ ajith.v@samsung.com, pedrosimonetti@chromium.org

[jdduke (slow), 2014-08-27 22:44:45]

Pedro could you see if the latest version works for you?

ajith.v@samsung.com: Will this work for your purposes? Could you give this a
test? We need it for focus changes and window attachment, but doing the same
thing for show/hide wasn't too much additional work. We'll probably need some
more tweaks for WebView (where the selection handles are rendered via
PopupWindows), but it shouldn't be too bad.

[AKVT, 2014-08-28 14:16:57]

On 2014/08/27 22:44:45, jdduke wrote:
> Pedro could you see if the latest version works for you?
> 
> mailto:ajith.v@samsung.com: Will this work for your purposes? Could you give
this a
> test? We need it for focus changes and window attachment, but doing the same
> thing for show/hide wasn't too much additional work. We'll probably need some
> more tweaks for WebView (where the selection handles are rendered via
> PopupWindows), but it shouldn't be too bad.

@jdduke - As per code flow I think it works for my use case as well. Once it
lands I will cross check again. Thank you :)

[jdduke (slow), 2014-08-28 20:15:40]

Patchset #3 (id:40001) has been deleted

[jdduke (slow), 2014-08-28 20:15:45]

Patchset #3 (id:60001) has been deleted

[jdduke (slow), 2014-08-28 20:16:43]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, tedchoc@chromium.org
- ajith.v@samsung.com

[jdduke (slow), 2014-08-28 20:16:43]

tedcho@, aelias@: Do either of you have any thoughts on this behavior change? Do
we need to run it through more UX review?

[Ted C, 2014-08-28 20:23:35]

On 2014/08/28 20:16:43, jdduke wrote:
> tedcho@, aelias@: Do either of you have any thoughts on this behavior change?
Do
> we need to run it through more UX review?

This seems reasonable to me.  Having the selection remain when going home and
coming
back seems like a good change.

As long as it is cleared when switching tabs and other in chrome navigations,
this seems
like a good thing.

I wouldn't worry about going through UX review for this.

[pedro (no code reviews), 2014-08-28 22:52:55]

On 2014/08/28 20:23:35, Ted C wrote:
> On 2014/08/28 20:16:43, jdduke wrote:
> > tedcho@, aelias@: Do either of you have any thoughts on this behavior
change?
> Do
> > we need to run it through more UX review?
> 
> This seems reasonable to me.  Having the selection remain when going home and
> coming
> back seems like a good change.
> 
> As long as it is cleared when switching tabs and other in chrome navigations,
> this seems
> like a good thing.
> 
> I wouldn't worry about going through UX review for this.

Jared, as we discussed offline, this does seem to work for our case. Thanks a
lot for implementing this!

[jdduke (slow), 2014-08-28 23:11:32]

aurimas@ brought up the fact that we should probably clear the selection if
another selection is made *within* the app, e.g., select some text, then tap the
omnibox. In this case, I'm not sure the signal seen by ContentViewCore is all
that different than the contextual search signal (loss of focus), so we may need
a few more bits here.

[Ted C, 2014-08-28 23:22:47]

On 2014/08/28 23:11:32, jdduke wrote:
> aurimas@ brought up the fact that we should probably clear the selection if
> another selection is made *within* the app, e.g., select some text, then tap
the
> omnibox. In this case, I'm not sure the signal seen by ContentViewCore is all
> that different than the contextual search signal (loss of focus), so we may
need
> a few more bits here.

I think a combination of:
onWindowFocusChanged
onFocusChanged
should do what you want.

!windowFocus && focus == keep selection
windowFocus && !focus == clear selection

[jdduke (slow), 2014-08-29 16:20:05]

On 2014/08/28 23:22:47, Ted C wrote:
> !windowFocus && focus == keep selection
> windowFocus && !focus == clear selection

Pedro, when contextual search is activated, are you seeing |windowFocus &&
!focus|?

[jdduke (slow), 2014-09-03 16:44:47]

Pedro: Could you verify that this will work for your purposes? Unfortunately,
there's currently no way to determine whether loss of focus is from switching
tabs or from activating the contextual search preview. We need the extra hook.

[pedro (no code reviews), 2014-09-03 18:43:03]

On 2014/09/03 16:44:47, jdduke wrote:
> Pedro: Could you verify that this will work for your purposes? Unfortunately,
> there's currently no way to determine whether loss of focus is from switching
> tabs or from activating the contextual search preview. We need the extra hook.

As we discussed offline, it would be nice resetting the boolean back to its
original value once the ContentViewCore gets focused again.

[jdduke (slow), 2014-09-03 18:46:00]

On 2014/09/03 18:43:03, pedrosimonetti wrote:
> On 2014/09/03 16:44:47, jdduke wrote:
> > Pedro: Could you verify that this will work for your purposes?
Unfortunately,
> > there's currently no way to determine whether loss of focus is from
switching
> > tabs or from activating the contextual search preview. We need the extra
hook.
> 
> As we discussed offline, it would be nice resetting the boolean back to its
> original value once the ContentViewCore gets focused again.

I uploaded a new version that uses an extra bit for clarity (and safety).

[jdduke (slow), 2014-09-03 18:51:59]

tedchoc@: Could you take a look? This isn't exactly pretty, but it works well
for the cases we're interested in (lock screen, switching apps,
backgrounding/foregrounding Chrome), while also supporting exceptional cases (as
in contextual search). The hide/focus/detach sequence is identical between
switching tabs and activating the contextual search preview, hence the
unfortunate need for a separate hook =/

[jdduke (slow), 2014-09-08 21:00:29]

aelias@: Owner review for render_widget_host_view_android.cc?
tedchoc@: Thoughts?

[aelias_OOO_until_Jul13, 2014-09-08 22:56:17]

lgtm for RWHVA

[Ted C, 2014-09-09 20:00:59]

lgtm (assuming the popups don't cause issue) and a few nits

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1468:
if (mUnselectAllOnActionModeDismiss) hideTextHandles();
If we don't hide the text handles, do we get log warnings that we are leaking
windows?  Aren't the text selection handles backed by popupwindows?  I thought
at some point it would complain, but maybe that doesn't happen anymore.

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2131:
public boolean hasSelection() {
add a javadoc for this

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
(right):

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:130:
assertTrue(mContentViewCore.hasSelection());
Can you add a couple follow up sets that make sure that the flag is cleared
(i.e. just the majority of the content of the above method).

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:184:

remove extra blank line

[jdduke (slow), 2014-09-10 00:20:16]

Patchset #9 (id:220001) has been deleted

[jdduke (slow), 2014-09-10 00:21:31]

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1468:
if (mUnselectAllOnActionModeDismiss) hideTextHandles();
On 2014/09/09 20:00:59, Ted C wrote:
> If we don't hide the text handles, do we get log warnings that we are leaking
> windows?  Aren't the text selection handles backed by popupwindows?  I thought
> at some point it would complain, but maybe that doesn't happen anymore.

Hmm, for WebView (we no longer use PopupWindow in in Chrome)? The PopupWindow
container we use (PopupTouchHandleDrawable) now only holds a weak reference to
the external world (the view), so we should be OK there.

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2131:
public boolean hasSelection() {
On 2014/09/09 20:00:59, Ted C wrote:
> add a javadoc for this

Done.

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
(right):

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:130:
assertTrue(mContentViewCore.hasSelection());
On 2014/09/09 20:00:59, Ted C wrote:
> Can you add a couple follow up sets that make sure that the flag is cleared
> (i.e. just the majority of the content of the above method).

Ah, good call.

https://codereview.chromium.org/516523003/diff/200001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:184:

On 2014/09/09 20:00:59, Ted C wrote:
> remove extra blank line

Done.

[jdduke (slow), 2014-09-10 00:21:39]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:30:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/516523003/240001

[commit-bot: I haz the power, 2014-09-10 02:44:24]

Committed patchset #9 (id:240001) as c889cefdf7886664b122f87bcd7b5f4e864795a3

[commit-bot: I haz the power, 2014-09-10 03:58:29]

Patchset 9 (id:??) landed as
https://crrev.com/6a4d1490ab6b5146a3dedd469d49a52045ce9194
Cr-Commit-Position: refs/heads/master@{#294082}