Clear the selection when hiding the handles

Clear the selection when hiding the handles

Clear the selected text when hiding the handles and the desired
behavior is to "unselect all on dismiss".

BUG=430859
internal b/18200283

Committed: https://crrev.com/be8997fdc783efbbbb040ffe4ba77f8f30e416b6
Cr-Commit-Position: refs/heads/master@{#303946}

[sgurun-gerrit only, 2014-11-08 00:32:04]

sgurun@chromium.org changed reviewers:
+ benm@chromium.org, jdduke@chromium.org

[jdduke (slow), 2014-11-08 00:43:10]

https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1821:
if (isSelectionEditable()) {
So, the question I have is, why aren't we hitting this code path?

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

https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:546:
if (mFocusedNodeEditable) hideTextHandles();
I'm not sure this will interact nicely with IME.

[sgurun-gerrit only, 2014-11-08 00:54:13]

On 2014/11/08 00:43:10, jdduke wrote:
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (left):
> 
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1821:
> if (isSelectionEditable()) {
> So, the question I have is, why aren't we hitting this code path?
> 
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:546:
> if (mFocusedNodeEditable) hideTextHandles();
> I'm not sure this will interact nicely with IME.

If there is any better way or if there is any way to verify IME interaction, I
can try.

[sgurun-gerrit only, 2014-11-08 00:54:19]

https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1821:
if (isSelectionEditable()) {
On 2014/11/08 00:43:10, jdduke wrote:
> So, the question I have is, why aren't we hitting this code path?

Actually, we do, but it comes earlier. What I observe is we get:
onHide()
  |
  ->hidePopupsAndPreserveSelection() 
onDestroyActionMode()
    clear selection (but mUnselectAllBla is true at this moment)
onFocusChanged()
  |
  ->hidePopupsAndPreserveSelection()

[sgurun-gerrit only, 2014-11-08 00:55:08]

On 2014/11/08 00:54:19, sgurun wrote:
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (left):
> 
>
https://codereview.chromium.org/713493003/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1821:
> if (isSelectionEditable()) {
> On 2014/11/08 00:43:10, jdduke wrote:
> > So, the question I have is, why aren't we hitting this code path?
> 
> Actually, we do, but it comes earlier. What I observe is we get:
> onHide()
>   |
>   ->hidePopupsAndPreserveSelection() 
> onDestroyActionMode()
>     clear selection (but mUnselectAllBla is true at this moment)
> onFocusChanged()
>   |
>   ->hidePopupsAndPreserveSelection()
                   ^^^^^^^^
                    ClearSelection()

[sgurun-gerrit only, 2014-11-08 02:47:14]

After discussing with jdduke@, I tried his suggestion and got rid of extra
boolean that we use to carry state for unselecting selected test. as far as I
can see, both clank and webview are ok, but I really don't know if there are any
cornercases to be tested. Will look at it on monday.

[jdduke (slow), 2014-11-10 15:58:05]

On 2014/11/08 02:47:14, sgurun wrote:
> After discussing with jdduke@, I tried his suggestion and got rid of extra
> boolean that we use to carry state for unselecting selected test. as far as I
> can see, both clank and webview are ok, but I really don't know if there are
any
> cornercases to be tested. Will look at it on monday.

Yay, this looks pretty good, let me try it out locally.

[jdduke (slow), 2014-11-10 18:48:53]

On 2014/11/10 15:58:05, jdduke wrote:
> On 2014/11/08 02:47:14, sgurun wrote:
> > After discussing with jdduke@, I tried his suggestion and got rid of extra
> > boolean that we use to carry state for unselecting selected test. as far as
I
> > can see, both clank and webview are ok, but I really don't know if there are
> any
> > cornercases to be tested. Will look at it on monday.
> 
> Yay, this looks pretty good, let me try it out locally.

It looks like the main issue is that the selection isn't cleared after the
action bar is destroyed as consequence of performing an explicit (action bar)
action like copy/share/etc...

[sgurun-gerrit only, 2014-11-11 02:35:55]

On 2014/11/10 18:48:53, jdduke wrote:
> On 2014/11/10 15:58:05, jdduke wrote:
> > On 2014/11/08 02:47:14, sgurun wrote:
> > > After discussing with jdduke@, I tried his suggestion and got rid of extra
> > > boolean that we use to carry state for unselecting selected test. as far
as
> I
> > > can see, both clank and webview are ok, but I really don't know if there
are
> > any
> > > cornercases to be tested. Will look at it on monday.
> > 
> > Yay, this looks pretty good, let me try it out locally.
> 
> It looks like the main issue is that the selection isn't cleared after the
> action bar is destroyed as consequence of performing an explicit (action bar)
> action like copy/share/etc...

jdduke@, can you please take a look at this again? thanks!

[jdduke (slow), 2014-11-11 03:11:53]

Yup, this should work nicely, thanks, just a couple thoughts.

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1216:
if (mInputConnection != null && mHasSelection) {
Hmm, can mInputConnection be null if the user just selects some read-only text?

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1220:
} else {
Should we pull this out into a separate method?  Then we can also call it from
onDestroyActionMode (before the hideTextHandles call there).

private void clearUserSelection() {
  if (!mHasSelection) return;
  if (isSelectionEditable() && mInputConnection != null) {
    int selectionEnd = Selection.getSelectionEnd(mEditable);
    mInputConnection.setSelection(selectionEnd, selectionEnd);
  } else {
    mImeAdapter.unselect();
  }
}

[sgurun-gerrit only, 2014-11-11 20:21:04]

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1216:
if (mInputConnection != null && mHasSelection) {
On 2014/11/11 03:11:53, jdduke wrote:
> Hmm, can mInputConnection be null if the user just selects some read-only
text?

I see. In my experiments with read only text I don't see it, and its
onCreateInputConnection is called first but I cannot be sure so changing it.

https://codereview.chromium.org/713493003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1220:
} else {
On 2014/11/11 03:11:53, jdduke wrote:
> Should we pull this out into a separate method?  Then we can also call it from
> onDestroyActionMode (before the hideTextHandles call there).
> 
> private void clearUserSelection() {
>   if (!mHasSelection) return;
>   if (isSelectionEditable() && mInputConnection != null) {
>     int selectionEnd = Selection.getSelectionEnd(mEditable);
>     mInputConnection.setSelection(selectionEnd, selectionEnd);
>   } else {
>     mImeAdapter.unselect();
>   }
> }
> 

Ah this caused problems with action bar selection. will look into more after
lunch.

[sgurun-gerrit only, 2014-11-12 20:58:06]

ptal, thanks!

[jdduke (slow), 2014-11-12 21:15:57]

Thanks, this lgtm if it works for WebView (I played with it locally, works just
fine with contextual search).

If you have time, a test in ContentViewCoreSelectionTest would be nice. I think
you can simply copy the testSelectionClearedAfterLossOfFocus test, but insert an
|hideOnUiThread()| call before the |requestFocusOnUiThread(false)| call, which
was the problem case here that we weren't handling properly.

[sgurun-gerrit only, 2014-11-12 22:11:17]

On 2014/11/12 21:15:57, jdduke wrote:
> Thanks, this lgtm if it works for WebView (I played with it locally, works
just
> fine with contextual search).
> 
> If you have time, a test in ContentViewCoreSelectionTest would be nice. I
think
> you can simply copy the testSelectionClearedAfterLossOfFocus test, but insert
an
> |hideOnUiThread()| call before the |requestFocusOnUiThread(false)| call, which
> was the problem case here that we weren't handling properly.

running really low in time with another fire. opened a bug for writing the test.
 crbug.com/432669

thanks!

[sgurun-gerrit only, 2014-11-12 22:11:27]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2014-11-12 22:12:17]

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

[commit-bot: I haz the power, 2014-11-12 23:56:10]

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

[commit-bot: I haz the power, 2014-11-12 23:56:11]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jdduke (slow), 2014-11-12 23:59:59]

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

https://codereview.chromium.org/713493003/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1336:
mImeAdapter.unselect();
Loosk like we need a null check for mImeAdapter as well.

[hush (inactive), 2014-11-13 00:04:13]

On 2014/11/12 23:59:59, jdduke wrote:
>
https://codereview.chromium.org/713493003/diff/120001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/713493003/diff/120001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1336:
> mImeAdapter.unselect();
> Loosk like we need a null check for mImeAdapter as well.

setImeAdapterForTest was only  called in ContentViewCoreInputConnectionTest.
In ContentViewCoreViewAndroidDelegateTest, we're just not calling it. So
mImeAdapter has been null the whole time during the
ContentViewCoreViewAndroidDelegateTest I suppose?

[sgurun-gerrit only, 2014-11-13 00:05:07]

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

https://codereview.chromium.org/713493003/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1336:
mImeAdapter.unselect();
On 2014/11/12 23:59:59, jdduke wrote:
> Loosk like we need a null check for mImeAdapter as well.

added. thanks

[sgurun-gerrit only, 2014-11-13 00:05:12]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2014-11-13 00:05:47]

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

[commit-bot: I haz the power, 2014-11-13 00:57:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-13 00:58:08]

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