Do not show IME on every touchend event when input field has focus

Do not show IME on every touchend event when input field has focus

Stop showing the virtual keyboard every time a touchend is consumed by the page. Instead let focus() trigger it when appropriate. This still makes us work well with fastclick.

BUG=318460

[kbalazs, 2013-11-12 23:28:05]

I did not do a deep testing, just a quick check to see if IME works properly for
url bar and input fields. So far it seems to me that there is no regression
because of removing this code. Please point out if this is a wrong assumption.

[jochen (gone - plz use gerrit), 2013-11-13 11:53:10]

+suzhe, olilan - thoughts?

[olilan, 2013-11-13 18:39:13]

On 2013/11/13 11:53:10, jochen wrote:
> +suzhe, olilan - thoughts?

Sorry, I no longer work on Chrome. Perhaps aurimas can help?

[kbalazs, 2013-11-13 19:39:04]

One test has failed but from the error message it does not seem to related
although it has something to do with touch events. Do you think it is related?

C  154s Main  [FAIL]
org.chromium.chrome.browser.infobar.InfoBarTest#testInfoBarForPopUp:
C  154s Main  java.lang.SecurityException: Injecting to another application
requires INJECT_EVENTS permission
C  154s Main  	at android.os.Parcel.readException(Parcel.java:1327)
C  154s Main  	at android.os.Parcel.readException(Parcel.java:1281)
C  154s Main  	at
android.view.IWindowManager$Stub$Proxy.injectPointerEvent(IWindowManager.java:1203)
C  154s Main  	at
android.app.Instrumentation.sendPointerSync(Instrumentation.java:902)
C  154s Main  	at
org.chromium.content.browser.test.util.TestTouchUtils.sendAction(TestTouchUtils.java:41)
C  154s Main  	at
org.chromium.content.browser.test.util.TestTouchUtils.singleClick(TestTouchUtils.java:54)
C  154s Main  	at
org.chromium.content.browser.test.util.TestTouchUtils.singleClickView(TestTouchUtils.java:70)
C  154s Main  	at
org.chromium.content.browser.test.util.TestTouchUtils.singleClickView(TestTouchUtils.java:82)
C  154s Main  	at
org.chromium.chrome.test.util.InfoBarUtil.findButton(InfoBarUtil.java:28)
C  154s Main  	at
org.chromium.chrome.test.util.InfoBarUtil.clickPrimaryButton(InfoBarUtil.java:65)
C  154s Main  	at
org.chromium.chrome.browser.infobar.InfoBarTest.testInfoBarForPopUp(InfoBarTest.java:68)
C  154s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
C  154s Main  	at
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
C  154s Main  	at
android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
C  154s Main  	at
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
C  154s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
C  154s Main  	at
android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
C  154s Main  	at
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
C  154s Main  	at
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)

[James Su, 2013-11-14 01:08:45]

Rubber Stamp LGTM.

On 2013/11/13 19:39:04, kbalazs wrote:
> One test has failed but from the error message it does not seem to related
> although it has something to do with touch events. Do you think it is related?
> 
> C  154s Main  [FAIL]
> org.chromium.chrome.browser.infobar.InfoBarTest#testInfoBarForPopUp:
> C  154s Main  java.lang.SecurityException: Injecting to another application
> requires INJECT_EVENTS permission
> C  154s Main  	at android.os.Parcel.readException(Parcel.java:1327)
> C  154s Main  	at android.os.Parcel.readException(Parcel.java:1281)
> C  154s Main  	at
>
android.view.IWindowManager$Stub$Proxy.injectPointerEvent(IWindowManager.java:1203)
> C  154s Main  	at
> android.app.Instrumentation.sendPointerSync(Instrumentation.java:902)
> C  154s Main  	at
>
org.chromium.content.browser.test.util.TestTouchUtils.sendAction(TestTouchUtils.java:41)
> C  154s Main  	at
>
org.chromium.content.browser.test.util.TestTouchUtils.singleClick(TestTouchUtils.java:54)
> C  154s Main  	at
>
org.chromium.content.browser.test.util.TestTouchUtils.singleClickView(TestTouchUtils.java:70)
> C  154s Main  	at
>
org.chromium.content.browser.test.util.TestTouchUtils.singleClickView(TestTouchUtils.java:82)
> C  154s Main  	at
> org.chromium.chrome.test.util.InfoBarUtil.findButton(InfoBarUtil.java:28)
> C  154s Main  	at
>
org.chromium.chrome.test.util.InfoBarUtil.clickPrimaryButton(InfoBarUtil.java:65)
> C  154s Main  	at
>
org.chromium.chrome.browser.infobar.InfoBarTest.testInfoBarForPopUp(InfoBarTest.java:68)
> C  154s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
> C  154s Main  	at
>
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
> C  154s Main  	at
> android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
> C  154s Main  	at
>
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
> C  154s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
> C  154s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
> C  154s Main  	at
>
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
> C  154s Main  	at
>
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)

[aurimas (slooooooooow), 2013-11-14 01:20:17]

NOT LGTM. Please do not land it yet and follow up on the discussion on the bug.

[kbalazs, 2013-12-02 22:39:06]

Besides removing the problematic part I changed the call to UpdateTextInputState
in RenderViewImpl::didChangeSelection so that the arg show_ime_if_needed is set
to true. This makes focus() on an input element trigger the IME and works well
will fastclick.

[aurimas (slooooooooow), 2013-12-02 23:15:04]

https://codereview.chromium.org/59553003/diff/110001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/59553003/diff/110001/content/renderer/render_...
content/renderer/render_view_impl.cc:2622: UpdateTextInputState(true, true);
Selection change is not the same as focus(), this call gets called multiple
times while the same not is focused. Maybe you are thinking about
RenderViewImpl::focusedNodeChanged()?

[kbalazs, 2013-12-03 20:57:52]

On 2013/12/02 23:15:04, aurimas wrote:
>
https://codereview.chromium.org/59553003/diff/110001/content/renderer/render_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/59553003/diff/110001/content/renderer/render_...
> content/renderer/render_view_impl.cc:2622: UpdateTextInputState(true, true);
> Selection change is not the same as focus(), this call gets called multiple
> times while the same not is focused. Maybe you are thinking about
> RenderViewImpl::focusedNodeChanged()?

Right, I did not really think it over, I just saw that there is a call to
UpdateTextInputState from here with show_ime_if_needed=false. I'm gonna check
the call chain of focus() and find a more appropriate place to add this, likely
focusedNodeChanged will be that.

[kbalazs, 2013-12-04 18:50:54]

If I call UpdateTextInputState with show_ime_if_needed=false than it will not
show the ime because the following condition is not satisfied:

if (show_ime_if_needed || (text_input_type_ != new_type
    || text_input_info_ != new_info
    || can_compose_inline_ != new_can_compose_inline))

If I set show_ime_if_needed to true than I think it would be shown when not
appropriate, i.e. calling focus() on any element.

When UpdateTextInputState is called from RenderViewImpl::didChangeSelection,
however, the condition is true because of the second part, but the ime is still
not showing. This makes me think that the real problem exist at browser side.

[kbalazs, 2013-12-04 20:12:41]

The code that prevents the ime to show on the browser side when
didChangeSelection calls UpdateTextInputState is this early return in
ImeAdapter::attachAndShowIfNeeded:

if (mTextInputType == sTextInputTypeNone && !showIfNeeded) {
    return;
}

So I can let didChangeSelection untouched and remove this early return as an
alternative to the current patch but it does not look any better.

I think there are several issues here and it's not obvious what would be the
perfect fix. I would like to concentrate on fixing the actual bug without
attempting some bigger redesign.

So the issues I see are:
 - it's not very clear what show_ime_if_needed really means in
UpdateTextInputState

 - the IME code tries to avoid sending superfluous messages as much as possible
but it looks fragile especially because the renderer has no knowledge whether
the virtual keyboard is actually active or not at the moment

 - UpdateTextInputState and UpdateTextInputType is conflicting
   both refreshes the value of text_input_type_, text_input_mode_ and
can_compose_inline_. Since it most cases UpdateTextInputState is called after
UpdateTextInputType, the former will obviously not see any change in the values

 - GetTextInputType() return TEXT_INPUT_TYP_NONE in
RenderViewImpl::focusedNodeChanged, although the focused node is an input field
   This looks a complex Blink problem


IMHO the current patch is good enough to fix the actual bug so I would stick to
it. Does that sound ok?

[aurimas (slooooooooow), 2013-12-05 00:19:01]

> The code that prevents the ime to show on the browser side when
> didChangeSelection calls UpdateTextInputState is this early return in
> ImeAdapter::attachAndShowIfNeeded:
> 
> if (mTextInputType == sTextInputTypeNone && !showIfNeeded) {
>     return;
> }
> 
> So I can let didChangeSelection untouched and remove this early return as an
> alternative to the current patch but it does not look any better.
> 
> I think there are several issues here and it's not obvious what would be the
> perfect fix. I would like to concentrate on fixing the actual bug without
> attempting some bigger redesign.
> 
> So the issues I see are:
>  - it's not very clear what show_ime_if_needed really means in
> UpdateTextInputState
show_ime_if_needed is the way to make sure that only certain
UpdateTextInputState messages bring up keyboard. For example, a website cannot
simply just call document.getElementById("myinputbox").focus(); to bring up the
keyboard. The goal was so that there would be no websites that would keep
toggling the keyboard up and down without any user interaction. As you see now,
only gestures can bring the keyboard up as they are the only calls that set
show_ime_if_needed to true (I agree that this argument could be renamed). The
behavior is consistent with  the stock android browser. If you believe that it
is incorrect behavior please start a thread on chromium-dev@ and cc me.

> 
>  - the IME code tries to avoid sending superfluous messages as much as
possible
> but it looks fragile especially because the renderer has no knowledge whether
> the virtual keyboard is actually active or not at the moment
Why would renderer care if the software keyboard is visible or not?

> 
>  - UpdateTextInputState and UpdateTextInputType is conflicting
>    both refreshes the value of text_input_type_, text_input_mode_ and
> can_compose_inline_. Since it most cases UpdateTextInputState is called after
> UpdateTextInputType, the former will obviously not see any change in the
values
I was the one who split the UpdateTextInputState and UpdateTextInputType calls.
There used to be only one, however none of the desktop platforms where using the
value of the text, require_ack. Android currently does not use
UpdateTextInputType messages on the browser side. text_input_info is a guard
that is only used in UpdateTextInputState call so UpdateTextInputType should not
affect it.

> 
>  - GetTextInputType() return TEXT_INPUT_TYP_NONE in
> RenderViewImpl::focusedNodeChanged, although the focused node is an input
field
>    This looks a complex Blink problem
Yes, I remember looking into that. It does seem that focusedNodeChanged gets
called before the type gets updated internally, however I can definitely see
focusedNodeChanged pass the text type value as an argument to fix that.
> 
> 
> IMHO the current patch is good enough to fix the actual bug so I would stick
to
> it. Does that sound ok?

Your patch would break the behavior of only allowing gestures to bring up the
software keyboard so I do not think this is good enough of a fix.

[kbalazs, 2013-12-05 01:37:09]

> Your patch would break the behavior of only allowing gestures to bring up the
> software keyboard so I do not think this is good enough of a fix.

Actually the current state of the world is that only gestures can bring it up,
plus this |if (touchend && processed)| extra case which is there to be
compatible with fastclick (at least this is the most noticable effect of it).
There is no way to be compatible with fastclick if only gestures can bring it
up. So my intention is to turn this extra consition into something more
reasonable to fix the bug that a processed touchend can bring up the keyboard
with no good reason.

Do we really need to defend against sites that would toggle the keyboard up and
down? Why would one do that? :)

I think the only undesired side effect of the current patch is that we are going
to send more messages, i.e. one on every selection change. I don't say that it's
a perfect fix though. I will see if I can do what you propose for
focusedNodeChanged.

[kbalazs, 2013-12-05 22:23:01]

> >  - GetTextInputType() return TEXT_INPUT_TYP_NONE in
> > RenderViewImpl::focusedNodeChanged, although the focused node is an input
> field
> >    This looks a complex Blink problem
> Yes, I remember looking into that. It does seem that focusedNodeChanged gets
> called before the type gets updated internally, however I can definitely see
> focusedNodeChanged pass the text type value as an argument to fix that.

Do you think about something like passing IsEditableNode(node) for
show_im_if_needed? If so, that sounds good to me.

[kbalazs, 2013-12-05 23:52:35]

On 2013/12/05 22:23:01, kbalazs wrote:
> > >  - GetTextInputType() return TEXT_INPUT_TYP_NONE in
> > > RenderViewImpl::focusedNodeChanged, although the focused node is an input
> > field
> > >    This looks a complex Blink problem
> > Yes, I remember looking into that. It does seem that focusedNodeChanged gets
> > called before the type gets updated internally, however I can definitely see
> > focusedNodeChanged pass the text type value as an argument to fix that.
> 
> Do you think about something like passing IsEditableNode(node) for
> show_im_if_needed? If so, that sounds good to me.

Nevermind, that will no be enough since GetTextInputType will still return
outdated info. We need to do what WebViewImpl::textInpuInfo() does but for a
given node. Which needs a lot of refactor for a still not very clean fix.

Probably the root problem is that Document::setFocusedElement calls
page()->chrome().focusedNodeChanged() before
frame->selection().didChangeFocus()?

[aurimas (slooooooooow), 2013-12-06 00:00:19]

On 2013/12/05 23:52:35, kbalazs wrote:
> On 2013/12/05 22:23:01, kbalazs wrote:
> > > >  - GetTextInputType() return TEXT_INPUT_TYP_NONE in
> > > > RenderViewImpl::focusedNodeChanged, although the focused node is an
input
> > > field
> > > >    This looks a complex Blink problem
> > > Yes, I remember looking into that. It does seem that focusedNodeChanged
gets
> > > called before the type gets updated internally, however I can definitely
see
> > > focusedNodeChanged pass the text type value as an argument to fix that.
> > 
> > Do you think about something like passing IsEditableNode(node) for
> > show_im_if_needed? If so, that sounds good to me.
> 
> Nevermind, that will no be enough since GetTextInputType will still return
> outdated info. We need to do what WebViewImpl::textInpuInfo() does but for a
> given node. Which needs a lot of refactor for a still not very clean fix.
> 
> Probably the root problem is that Document::setFocusedElement calls
> page()->chrome().focusedNodeChanged() before
> frame->selection().didChangeFocus()?

Yeah, i think you might be right about that.

[kbalazs, 2013-12-06 23:30:42]

On 2013/12/06 00:00:19, aurimas wrote:
> On 2013/12/05 23:52:35, kbalazs wrote:
> > On 2013/12/05 22:23:01, kbalazs wrote:
> > > > >  - GetTextInputType() return TEXT_INPUT_TYP_NONE in
> > > > > RenderViewImpl::focusedNodeChanged, although the focused node is an
> input
> > > > field
> > > > >    This looks a complex Blink problem
> > > > Yes, I remember looking into that. It does seem that focusedNodeChanged
> gets
> > > > called before the type gets updated internally, however I can definitely
> see
> > > > focusedNodeChanged pass the text type value as an argument to fix that.
> > > 
> > > Do you think about something like passing IsEditableNode(node) for
> > > show_im_if_needed? If so, that sounds good to me.
> > 
> > Nevermind, that will no be enough since GetTextInputType will still return
> > outdated info. We need to do what WebViewImpl::textInpuInfo() does but for a
> > given node. Which needs a lot of refactor for a still not very clean fix.
> > 
> > Probably the root problem is that Document::setFocusedElement calls
> > page()->chrome().focusedNodeChanged() before
> > frame->selection().didChangeFocus()?
> 
> Yeah, i think you might be right about that.

That does not work either. The problem is that webwidget_->textInputInfo() does
not give the right information information about the focused node until the
selection is updated. By taking a look at this function it seems like it's not
clear that it should work with the currently focused node or the current
selection. It early returns if there is no selection, than queries some
information based on the focused node, than use the selection to query offsets.

It seems like this inconsistency is coming from this changeset:
http://trac.webkit.org/changeset/121451.

I think it looks good to just rearrange this function so that if there is no
selection it will return info for the focused element. Than the offsets can be
extracted from the selection so it will not contradict the other change. Anyway
we have tests so it should be covered. I'm gonna try this.

[kbalazs, 2013-12-07 00:38:04]

New patch is depending on https://codereview.chromium.org/103863007/

[kbalazs, 2013-12-11 17:04:52]

On 2013/12/07 00:38:04, kbalazs wrote:
> New patch is depending on https://codereview.chromium.org/103863007/

Blink patch landed, we can go forward with this one.

[aurimas (slooooooooow), 2013-12-11 17:07:30]

LGTM

[commit-bot: I haz the power, 2013-12-11 18:07:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/59553003/120001

[commit-bot: I haz the power, 2013-12-11 18:27:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jochen (gone - plz use gerrit), 2013-12-11 21:15:22]

lgtm

[commit-bot: I haz the power, 2013-12-11 22:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/59553003/120001

[kbalazs, 2013-12-13 15:37:13]

FYI the blink patch was rolled out because it caused a regression in an android
content-shell instrumentation test. I need to fix that before going forward with
this one.

[kbalazs, 2013-12-13 16:09:37]

So it turns out that both Blink and ime tests agree that WebView::textInputInfo
should be based on selection and not focus. I can think of two solution:
 1 introduce new api WebView::textInputInfoForFocusedElement, probably rename
textInputInfo to textInputInfoForSelection. Go with patchset 3 and use the
former.
 2 do not touch Blink, resort to patchset 2. I believe it will not cause any
regression. The concern with it was that we only need to toggle ime when
selection changes initially and when it becomes empty again, so if we send ime
message on every change that adds noise. That is true but I am not sure that is
a big problem though, we already have redundant messages once the ime gets
activated (I added a LOG in UpdateTextInputState in the if where it sends the
message, once the ime is up it is triggered periodically. I did not look further
for the reason).

Please tell me what do you think and which solution do you prefer.

[kbalazs, 2013-12-17 21:16:24]

@aurimas: could you share your opinion about my last comments please?

[aurimas (slooooooooow), 2013-12-18 17:06:11]

On 2013/12/13 16:09:37, kbalazs wrote:
> So it turns out that both Blink and ime tests agree that
WebView::textInputInfo
> should be based on selection and not focus. I can think of two solution:
>  1 introduce new api WebView::textInputInfoForFocusedElement, probably rename
> textInputInfo to textInputInfoForSelection. Go with patchset 3 and use the
> former.
When would you use either of those calls?

>  2 do not touch Blink, resort to patchset 2. I believe it will not cause any
> regression. The concern with it was that we only need to toggle ime when
> selection changes initially and when it becomes empty again, so if we send ime
> message on every change that adds noise. That is true but I am not sure that
is
> a big problem though, we already have redundant messages once the ime gets
> activated (I added a LOG in UpdateTextInputState in the if where it sends the
> message, once the ime is up it is triggered periodically. I did not look
further
> for the reason).
Yes, there are UpdateTextInputState calls every time text type, selection, or
composition range change, however there is only one call that sets
show_ime_if_needed to true. Your patch #2 would make that show_ime_if_needed
would be set to true on every selection change and thus sending through the IPC
through every time even if the text type, selection or composition haven't
changed. 

> 
> Please tell me what do you think and which solution do you prefer.

[kbalazs, 2013-12-18 22:23:29]

On 2013/12/18 17:06:11, aurimas wrote:
> On 2013/12/13 16:09:37, kbalazs wrote:
> > So it turns out that both Blink and ime tests agree that
> WebView::textInputInfo
> > should be based on selection and not focus. I can think of two solution:
> >  1 introduce new api WebView::textInputInfoForFocusedElement, probably
rename
> > textInputInfo to textInputInfoForSelection. Go with patchset 3 and use the
> > former.
> When would you use either of those calls?

I meant I would use that in RenderViewImpl::focusedNodeChanged. How does that
sound to you?

> 
> >  2 do not touch Blink, resort to patchset 2. I believe it will not cause any
> > regression. The concern with it was that we only need to toggle ime when
> > selection changes initially and when it becomes empty again, so if we send
ime
> > message on every change that adds noise. That is true but I am not sure that
> is
> > a big problem though, we already have redundant messages once the ime gets
> > activated (I added a LOG in UpdateTextInputState in the if where it sends
the
> > message, once the ime is up it is triggered periodically. I did not look
> further
> > for the reason).
> Yes, there are UpdateTextInputState calls every time text type, selection, or
> composition range change, however there is only one call that sets
> show_ime_if_needed to true. Your patch #2 would make that show_ime_if_needed
> would be set to true on every selection change and thus sending through the
IPC
> through every time even if the text type, selection or composition haven't
> changed. 

Right, I was wrong, I probably tested the patched version.

[kbalazs, 2013-12-20 01:50:41]

On 2013/12/18 22:23:29, kbalazs wrote:
> On 2013/12/18 17:06:11, aurimas wrote:
> > On 2013/12/13 16:09:37, kbalazs wrote:
> > > So it turns out that both Blink and ime tests agree that
> > WebView::textInputInfo
> > > should be based on selection and not focus. I can think of two solution:
> > >  1 introduce new api WebView::textInputInfoForFocusedElement, probably
> rename
> > > textInputInfo to textInputInfoForSelection. Go with patchset 3 and use the
> > > former.
> > When would you use either of those calls?
> 
> I meant I would use that in RenderViewImpl::focusedNodeChanged. How does that
> sound to you?

Ok, I realized that it would be a bigger refactor. Plus probably the test
failure is just a matter of changing the expectation so I will try this first.

[klobag.chromium, 2014-01-23 23:01:49]

https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
content/renderer/render_view_impl.cc:2831: void
RenderViewImpl::focusedNodeChanged(const WebNode& node) {
If user dismisses the OSK, and re-tap the text field, is this get called?

[aurimas (slooooooooow), 2014-01-23 23:05:22]

https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
content/renderer/render_view_impl.cc:2831: void
RenderViewImpl::focusedNodeChanged(const WebNode& node) {
On 2014/01/23 23:01:49, klobag.chromium wrote:
> If user dismisses the OSK, and re-tap the text field, is this get called?

I think klobag@ means when you use a back key on Android to hide the keyboard.
That hides the keyboard but the focus stays on the node.

[kbalazs, 2014-01-24 20:56:06]

On 2014/01/23 23:05:22, aurimas wrote:
>
https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
> content/renderer/render_view_impl.cc:2831: void
> RenderViewImpl::focusedNodeChanged(const WebNode& node) {
> On 2014/01/23 23:01:49, klobag.chromium wrote:
> > If user dismisses the OSK, and re-tap the text field, is this get called?
> 
> I think klobag@ means when you use a back key on Android to hide the keyboard.
> That hides the keyboard but the focus stays on the node.

On retap it does not gets called but tap changes the selection and it will
trigger the keyboard. My code in focusedNodeChanged is only needed for allowing
js to trigger it.

[aurimas (slooooooooow), 2014-01-24 21:01:09]

On 2014/01/24 20:56:06, kbalazs wrote:
> On 2014/01/23 23:05:22, aurimas wrote:
> >
>
https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/59553003/diff/230001/content/renderer/render_...
> > content/renderer/render_view_impl.cc:2831: void
> > RenderViewImpl::focusedNodeChanged(const WebNode& node) {
> > On 2014/01/23 23:01:49, klobag.chromium wrote:
> > > If user dismisses the OSK, and re-tap the text field, is this get called?
> > 
> > I think klobag@ means when you use a back key on Android to hide the
keyboard.
> > That hides the keyboard but the focus stays on the node.
> 
> On retap it does not gets called but tap changes the selection and it will
> trigger the keyboard. My code in focusedNodeChanged is only needed for
allowing
> js to trigger it.

Why would change in selection trigger a keyboard to show up?

[kbalazs, 2014-01-24 21:14:25]

As it turned out we don't want the behavior change this patch implies (showing
the virtual keyboard when js calls focus()). So the tests are right and we
should not land this. Although the quirk we have now and which makes us
compatible with fastclick is not very correct, I'm not sure we can do much
better. Closing this for now.