Copying text closes the keyboard and the text input gets unfocused, forcing virtual keyboard is get…

Copying text closes the keyboard and the text input gets unfocused, forcing virtual keyboard is getting closed.

During copy operation inside an editable field, we are clearing the selection, which is clearing the cursor
as well. In turn there is no rootEditable Element for the selection and Keyboard type is sending as None and hence IME is dismissing by ImeAdapater. This is preventing user to edit the input field continuously even after a copy operation.
To prevent this keyboard hiding issue, we are collapsing the selection to end instead of clearing the selection, so that the cursor and caret will be visible and keyboard will be ON and user can continue editing.

BUG=225090
Changes curresponding to this has been landed at Chromium side with https://codereview.chromium.org/330623002/
Hence closing this issue.

[AKVT, 2014-06-12 10:45:10]

PTAL

[yosin_UTC9, 2014-06-13 04:20:15]

Please implement functionality in FrameSelection rather than WebFrameImpl. We
would like Web* are thin API wrapper.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1847: if (hasSelection()) {
nit: We prefer early return, e.g.

if (!hasSelection())
  return false;

....

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1850: if (selection.isInTextField() &&
selection.isRange()) {
We may want not lose selection for contenteditable and TEXTAREA? It seems
isInTextFiled() doesn't return true for TEXTAREA.

How about |if (selection.rootEditablElement() && selection.isRange())|?

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1851: // Check whether it's a range selection.
It appears to be reduandant.
It seems Selection.collapseToEnd() == selection().moveTo(selection.end()) is
enough.

[AKVT, 2014-06-13 10:41:00]

On 2014/06/13 04:20:15, yosi wrote:
> Please implement functionality in FrameSelection rather than WebFrameImpl. We
> would like Web* are thin API wrapper.
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> File Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> Source/web/WebLocalFrameImpl.cpp:1847: if (hasSelection()) {
> nit: We prefer early return, e.g.
> 
> if (!hasSelection())
>   return false;
> 
> ....
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> Source/web/WebLocalFrameImpl.cpp:1850: if (selection.isInTextField() &&
> selection.isRange()) {
> We may want not lose selection for contenteditable and TEXTAREA? It seems
> isInTextFiled() doesn't return true for TEXTAREA.
> 
> How about |if (selection.rootEditablElement() && selection.isRange())|?
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> Source/web/WebLocalFrameImpl.cpp:1851: // Check whether it's a range
selection.
> It appears to be reduandant.
> It seems Selection.collapseToEnd() == selection().moveTo(selection.end()) is
> enough.

@Yosi Thanks for reviewing. Could you please clarify more on this
Selection.collapseToEnd() == selection().moveTo(selection.end()) point. When I
checked the code this API is not calling from anywhere from .cpp files. Also I
get compilation issues when I used to call this method as VisibleSelection
doesn't have this members.

[yosin_UTC9, 2014-06-16 01:21:34]

On 2014/06/13 10:41:00, AJITH KUMAR V wrote:
> @Yosi Thanks for reviewing. Could you please clarify more on this
> Selection.collapseToEnd() == selection().moveTo(selection.end()) point. When I
> checked the code this API is not calling from anywhere from .cpp files. Also I
> get compilation issues when I used to call this method as VisibleSelection
> doesn't have this members.

Sorry for confusion, I said we can use |selection().moveTo(selection.end())|. It
makes selection to collapse to end.

[AKVT, 2014-06-16 06:45:48]

On 2014/06/16 01:21:34, yosi wrote:
> On 2014/06/13 10:41:00, AJITH KUMAR V wrote:
> > @Yosi Thanks for reviewing. Could you please clarify more on this
> > Selection.collapseToEnd() == selection().moveTo(selection.end()) point. When
I
> > checked the code this API is not calling from anywhere from .cpp files. Also
I
> > get compilation issues when I used to call this method as VisibleSelection
> > doesn't have this members.
> 
> Sorry for confusion, I said we can use |selection().moveTo(selection.end())|.
It
> makes selection to collapse to end.

@Yosi Thanks for the clarification. I am uploading new patch set based on your
review comments.

[AKVT, 2014-06-16 06:50:12]

@Yosi PTAL this change. I have optimized the code to collapse the selection to
end.

[Yuta Kitamura, 2014-06-16 07:58:43]

As Yoshi said in #2, please do not add logic in
WebLocalFrame. Actual operations should be done within
core/ classes.

I couldn't understand what the C++ code tries to do, though.
It's unclear to me how adding a new method to WebFrame
fixes (or helps fixing) the issue stated in the change
description.

Please explain what the added function is expected to do.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1852: // During copy, Range selection should be
available. For safety purpse added this condition.
typo: purpse

[AKVT, 2014-06-16 09:25:25]

On 2014/06/16 07:58:43, Yuta Kitamura wrote:
> As Yoshi said in #2, please do not add logic in
> WebLocalFrame. Actual operations should be done within
> core/ classes.
> 
> I couldn't understand what the C++ code tries to do, though.
> It's unclear to me how adding a new method to WebFrame
> fixes (or helps fixing) the issue stated in the change
> description.
> 
> Please explain what the added function is expected to do.
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> File Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
> Source/web/WebLocalFrameImpl.cpp:1852: // During copy, Range selection should
be
> available. For safety purpse added this condition.
> typo: purpse

@Yuta and Yosi, there was some problem while uploading new patch. I will upload
it soon. I addressed all the concerns in new patch.
@Yuta - The logic is to avoid the clearing of selection and change the selection
to caret selection. So that user can continue editing without disturbance.

[AKVT, 2014-06-16 10:34:35]

Fixed all review comments.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1850: if (selection.isInTextField() &&
selection.isRange()) {
On 2014/06/13 04:20:15, yosi wrote:
> We may want not lose selection for contenteditable and TEXTAREA? It seems
> isInTextFiled() doesn't return true for TEXTAREA.
> 
> How about |if (selection.rootEditablElement() && selection.isRange())|?

Done.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1850: if (selection.isInTextField() &&
selection.isRange()) {
On 2014/06/13 04:20:15, yosi wrote:
> We may want not lose selection for contenteditable and TEXTAREA? It seems
> isInTextFiled() doesn't return true for TEXTAREA.
> 
> How about |if (selection.rootEditablElement() && selection.isRange())|?

Done.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1851: // Check whether it's a range selection.
It appears to be reduandant.
On 2014/06/13 04:20:15, yosi wrote:
> It seems Selection.collapseToEnd() == selection().moveTo(selection.end()) is
> enough.

Done.

https://codereview.chromium.org/330613002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1852: // During copy, Range selection should be
available. For safety purpse added this condition.
On 2014/06/16 07:58:43, Yuta Kitamura wrote:
> typo: purpse

Done.

[AKVT, 2014-06-16 10:38:44]

@Yosi and Yuta. PTAL this change.

[yosin_UTC9, 2014-06-17 01:16:59]

Could you add test case? We can add test case into "web/tests/WebFrameTest.cpp".

https://codereview.chromium.org/330613002/diff/60001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/330613002/diff/60001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1839: if (!hasSelection())
Q: Why do we need to call |hasSelection|?
It seems just call |FrameSelection::collapseToEnd()| is enough.
|WebLocalFrameImpl::setelectionRange()| is one line function, it just call
|FrameSelection::toNormalizedRange()|.

[Yuta Kitamura, 2014-06-17 03:56:45]

I still don't understand why adding an interface to
WebFrame is necessary. Which class clears the selection
on copy? Why do you need to add an interface to WebFrame
in order to control the behavior on copy? Are existing
APIs insufficient? What are you going to do after this API
is added?

Adding an interface to WebFrame has some cost. You need
to provide a good justification to show its necessity.

I couldn't decide whether your approach is plausible or not.
Please provide enough context *in the change description*
so that people who find this patch later can understand
the patch well.

[AKVT, 2014-06-17 04:27:02]

On 2014/06/17 03:56:45, Yuta Kitamura wrote:
> I still don't understand why adding an interface to
> WebFrame is necessary. Which class clears the selection
> on copy? Why do you need to add an interface to WebFrame
> in order to control the behavior on copy? Are existing
> APIs insufficient? What are you going to do after this API
> is added?
> 
> Adding an interface to WebFrame has some cost. You need
> to provide a good justification to show its necessity.
> 
> I couldn't decide whether your approach is plausible or not.
> Please provide enough context *in the change description*
> so that people who find this patch later can understand
> the patch well.

@Yuta - RenderFrameImpl::OnUnselect() is getting called to unselect the
selection from ImeAdapter. This will call
frame()->editor().command(command).execute(); -> executeUnselect() ->
frame.selection().clear(); and clears the selection.
Clearing of selection on copy is uniform across all Android apps.
But if the selection is on an input field, other apps shows the cursor to the
end point of selection after do a copy. In browser, it clears the cursor and
keyboard gets disappeared.
To prevent this, I am changing the existing selection to collapse at the end. So
that it will continue to show the cursor and keyboard won't be hidden. Normal
frame selection case it continues to clear the selection.
I couldn't find alternative in WebLocalFrameImpl to serve this purpose. I will
update more details in change description.

[AKVT, 2014-06-17 06:35:46]

@Yosi - I made WebLocalFrameImpl light weight as per the comments. I will upload
the test case soon.

https://codereview.chromium.org/330613002/diff/60001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/330613002/diff/60001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1839: if (!hasSelection())
On 2014/06/17 01:16:59, yosi wrote:
> Q: Why do we need to call |hasSelection|?
> It seems just call |FrameSelection::collapseToEnd()| is enough.
> |WebLocalFrameImpl::setelectionRange()| is one line function, it just call
> |FrameSelection::toNormalizedRange()|.

Done.

[AKVT, 2014-06-17 07:09:05]

@Yosi and Yuta PTAL. I have added test case for this change.

[AKVT, 2014-06-17 09:11:15]

PTAL

[Yuta Kitamura, 2014-06-18 02:41:49]

On 2014/06/17 04:27:02, AJITH KUMAR V wrote:
> @Yuta - RenderFrameImpl::OnUnselect() is getting called to unselect the
> selection from ImeAdapter. This will call
> frame()->editor().command(command).execute(); -> executeUnselect() ->
> frame.selection().clear(); and clears the selection.
> Clearing of selection on copy is uniform across all Android apps.
> But if the selection is on an input field, other apps shows the cursor to the
> end point of selection after do a copy. In browser, it clears the cursor and
> keyboard gets disappeared.
> To prevent this, I am changing the existing selection to collapse at the end.
So
> that it will continue to show the cursor and keyboard won't be hidden. Normal
> frame selection case it continues to clear the selection.
> I couldn't find alternative in WebLocalFrameImpl to serve this purpose. I will
> update more details in change description.

Thanks, I think I understand the issue well now.

Considering the issue you explained, I feel like the function you
added may be too specialized for your purpose. It is pretty much
unlikely that anybody else will use your function.

I think you can fix the issue in the embedder side by combinating
the existing WebFrame functions: selectionRange() and
selectRange(const WebRange&), no?

[An overload of selectRange taking const WebRange& is NOT
deprecated as far as I understand; see commit
a834d23b2de10644ccf58852bbbe1155fb54180e for your reference.]

[AKVT, 2014-06-18 12:03:10]

On 2014/06/18 02:41:49, Yuta Kitamura wrote:
> On 2014/06/17 04:27:02, AJITH KUMAR V wrote:
> > @Yuta - RenderFrameImpl::OnUnselect() is getting called to unselect the
> > selection from ImeAdapter. This will call
> > frame()->editor().command(command).execute(); -> executeUnselect() ->
> > frame.selection().clear(); and clears the selection.
> > Clearing of selection on copy is uniform across all Android apps.
> > But if the selection is on an input field, other apps shows the cursor to
the
> > end point of selection after do a copy. In browser, it clears the cursor and
> > keyboard gets disappeared.
> > To prevent this, I am changing the existing selection to collapse at the
end.
> So
> > that it will continue to show the cursor and keyboard won't be hidden.
Normal
> > frame selection case it continues to clear the selection.
> > I couldn't find alternative in WebLocalFrameImpl to serve this purpose. I
will
> > update more details in change description.
> 
> Thanks, I think I understand the issue well now.
> 
> Considering the issue you explained, I feel like the function you
> added may be too specialized for your purpose. It is pretty much
> unlikely that anybody else will use your function.
> 
> I think you can fix the issue in the embedder side by combinating
> the existing WebFrame functions: selectionRange() and
> selectRange(const WebRange&), no?
> 
> [An overload of selectRange taking const WebRange& is NOT
> deprecated as far as I understand; see commit
> a834d23b2de10644ccf58852bbbe1155fb54180e for your reference.]

@Yuta Thanks for the detailed review. Now I shall remove FrameSelection changes
and make use of selectionRange() and selectReange() APIs from WebLocalFrameImpl.
I have made the changes and found that new API is little heavy by considering
the policy of keeping WebLocalFrameImpl as light weight. I will upload new patch
set with this change. Please suggest your opinion.

[AKVT, 2014-06-18 12:04:37]

@Yuta and Yosi PTAL

[yosin_UTC9, 2014-06-19 01:02:06]

On 2014/06/18 12:04:37, AJITH KUMAR V wrote:
> @Yuta and Yosi PTAL

I think yuta@ said to fix this issue is changing Chromium side rather than Blink
side.
We don't need to add new API into Blink.

[AKVT, 2014-06-19 01:10:56]

On 2014/06/19 01:02:06, yosi wrote:
> On 2014/06/18 12:04:37, AJITH KUMAR V wrote:
> > @Yuta and Yosi PTAL
> 
> I think yuta@ said to fix this issue is changing Chromium side rather than
Blink
> side.
> We don't need to add new API into Blink.

Thanks Yosi. I got it, The code I added with WebRange, I can place in
OnUnSelect() itself. I will do that. In that case do I need to write a test case
in WebFrameTest.cpp ?
Because change will be limited to render_frame_impl.cc.

[yosin_UTC9, 2014-06-19 01:59:26]

On 2014/06/19 01:10:56, AJITH KUMAR V wrote:
> On 2014/06/19 01:02:06, yosi wrote:
> > On 2014/06/18 12:04:37, AJITH KUMAR V wrote:
> > > @Yuta and Yosi PTAL
> > 
> > I think yuta@ said to fix this issue is changing Chromium side rather than
> Blink
> > side.
> > We don't need to add new API into Blink.
> 
> Thanks Yosi. I got it, The code I added with WebRange, I can place in
> OnUnSelect() itself. I will do that. In that case do I need to write a test
case
> in WebFrameTest.cpp ?
> Because change will be limited to render_frame_impl.cc.

Sorry for confusion, I mean it should be implemented in
AdapterInputConnection.java, ImeAdapter.java or another file in Android side;
I'm not so familiar with Android implementation, rather than platform common
layer.

You may want to cancel this patch and create a new patch for Android, since
reviewers different. I recommend to ask bcwhite@chromium.org.

[Yuta Kitamura, 2014-06-19 02:50:33]

On 2014/06/19 01:59:26, yosi wrote:
> Sorry for confusion, I mean it should be implemented in
> AdapterInputConnection.java, ImeAdapter.java or another file in Android side;
> I'm not so familiar with Android implementation, rather than platform common
> layer.
> 
> You may want to cancel this patch and create a new patch for Android, since
> reviewers different. I recommend to ask mailto:bcwhite@chromium.org.

Yup, my point was that existing functions could be
sufficient to fix the issue. You don't have to add a new
one if that's the case.

[AKVT, 2014-06-19 07:12:31]

On 2014/06/19 02:50:33, Yuta Kitamura wrote:
> On 2014/06/19 01:59:26, yosi wrote:
> > Sorry for confusion, I mean it should be implemented in
> > AdapterInputConnection.java, ImeAdapter.java or another file in Android
side;
> > I'm not so familiar with Android implementation, rather than platform common
> > layer.
> > 
> > You may want to cancel this patch and create a new patch for Android, since
> > reviewers different. I recommend to ask mailto:bcwhite@chromium.org.
> 
> Yup, my point was that existing functions could be
> sufficient to fix the issue. You don't have to add a new
> one if that's the case.

Thanks Yosi & Yuta for the clarification. I shall make the changes in
ContentViewCore, so that no specific API is not required in Blink side. I will
refine patch submitted @ https://codereview.chromium.org/330623002/

[Yuta Kitamura, 2014-06-19 07:26:36]

Sure, please don't forget to close this code review
from "Edit Issue" link. (Otherwise, this code review
will keep being shown in the reviewers' pages.)