Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138103003:
Remember previous keyboard type and use that when switching to "none" type. (Closed)
DescriptionRemember previous keyboard type and use that when switching to "none" type.
BUG=484139
Committed: https://crrev.com/e8e5d60be5f3b36634a1b012ec21874a4abe0ef0
Cr-Commit-Position: refs/heads/master@{#330551}
Patch Set 1 #
Total comments: 3
Patch Set 2 : clear sPreviousOutAttrs when closing keyboard #Messages
Total messages: 24 (3 generated)
bcwhite@chromium.org changed reviewers: + aurimas@chromium.org
https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:144: if (imeAdapter.getTextInputType() == TextInputType.NONE && sPreviousOutAttrs != null) { I dont think this is correct. Don't we also get NONE input type when we defocus an input field?
On 2015/05/11 20:22:39, aurimas wrote: > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > (right): > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:144: > if (imeAdapter.getTextInputType() == TextInputType.NONE && sPreviousOutAttrs != > null) { > I dont think this is correct. Don't we also get NONE input type when we defocus > an input field? Yes but that's okay. This doesn't affect visibility of the keyboard; that's handled differently. It just means that the keyboard type won't reset to the "default" when it's being put away. When a new field is selected, it goes to the desired type.
https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:103: private static EditorInfo sPreviousOutAttrs; Should this be cleared when we call hideSoftInputFromWindow?
https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:103: private static EditorInfo sPreviousOutAttrs; On 2015/05/11 20:35:33, aurimas wrote: > Should this be cleared when we call hideSoftInputFromWindow? I don't think it's necessary but can be done.
On 2015/05/11 at 20:58:18, bcwhite wrote: > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:103: private static EditorInfo sPreviousOutAttrs; > On 2015/05/11 20:35:33, aurimas wrote: > > Should this be cleared when we call hideSoftInputFromWindow? > > I don't think it's necessary but can be done. I am just worried that this will somehow cause more bugs for us. It does not seem correct that we report the wrong input type. Could we ask Google Keyboard team to make sure this will not break things for them? If they are OK with it then LGTM.
On 2015/05/11 21:11:27, aurimas wrote: > On 2015/05/11 at 20:58:18, bcwhite wrote: > > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > > File > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > (right): > > > > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:103: > private static EditorInfo sPreviousOutAttrs; > > On 2015/05/11 20:35:33, aurimas wrote: > > > Should this be cleared when we call hideSoftInputFromWindow? > > > > I don't think it's necessary but can be done. > > I am just worried that this will somehow cause more bugs for us. It does not > seem correct that we report the wrong input type. Could we ask Google Keyboard > team to make sure this will not break things for them? If they are OK with it > then LGTM. It doesn't change anything for the keyboard. It would have been configured either way; this just keeps the configuration the same as previously rather that a more basic state without the extra configuration for specialized input fields.
On 2015/05/11 at 21:18:00, bcwhite wrote: > On 2015/05/11 21:11:27, aurimas wrote: > > On 2015/05/11 at 20:58:18, bcwhite wrote: > > > > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > > > File > > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > > (right): > > > > > > > > https://codereview.chromium.org/1138103003/diff/1/content/public/android/java... > > > > > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:103: > > private static EditorInfo sPreviousOutAttrs; > > > On 2015/05/11 20:35:33, aurimas wrote: > > > > Should this be cleared when we call hideSoftInputFromWindow? > > > > > > I don't think it's necessary but can be done. > > > > I am just worried that this will somehow cause more bugs for us. It does not > > seem correct that we report the wrong input type. Could we ask Google Keyboard > > team to make sure this will not break things for them? If they are OK with it > > then LGTM. > > It doesn't change anything for the keyboard. It would have been configured either way; this just keeps the configuration the same as previously rather that a more basic state without the extra configuration for specialized input fields. Can we add tests for this? Also did you test with other keyboards like Swiftkey or Swype?
aelias@chromium.org changed reviewers: + aelias@chromium.org
Hmm, logic like this doesn't belong in a factory method, I think this patch is
masking a problem at another layer. TextInputType "none" looks like it refers
to unfocus, so I'm guessing a higher-level system like Blink is unfocusing then
refocusing the keyboard. Could we avoid sending an IME update for the
intermediate stage in that upper layer?
There's an existing related hack in ImeAdapter.attach():
if (mTextInputType == TextInputType.NONE) {
if (delayDismissInput) {
// Set a delayed task to do unfocus. This avoids hiding the
keyboard when tabbing
// through text inputs or when JS rapidly changes focus to
another text element.
mHandler.postDelayed(mDismissInputRunnable,
INPUT_DISMISS_DELAY);
mIsShowWithoutHideOutstanding = false;
} else {
// Some things (including tests) expect the keyboard to be
dismissed immediately.
dismissInput(true);
}
}
This is another ugly workaround, to avoid the problem of the keyboard
disappearing on the unfocus event. But it still runs into the problem you're
trying to fix here. Instead of piling more complexity here, could we delete
this original workaround too and fix the problem closer to the root?
On 2015/05/12 00:06:43, aelias wrote:
> Hmm, logic like this doesn't belong in a factory method, I think this patch is
> masking a problem at another layer. TextInputType "none" looks like it refers
> to unfocus, so I'm guessing a higher-level system like Blink is unfocusing
then
> refocusing the keyboard. Could we avoid sending an IME update for the
> intermediate stage in that upper layer?
>
> There's an existing related hack in ImeAdapter.attach():
>
> if (mTextInputType == TextInputType.NONE) {
> if (delayDismissInput) {
> // Set a delayed task to do unfocus. This avoids hiding the
> keyboard when tabbing
> // through text inputs or when JS rapidly changes focus to
> another text element.
> mHandler.postDelayed(mDismissInputRunnable,
> INPUT_DISMISS_DELAY);
> mIsShowWithoutHideOutstanding = false;
> } else {
> // Some things (including tests) expect the keyboard to be
> dismissed immediately.
> dismissInput(true);
> }
> }
>
> This is another ugly workaround, to avoid the problem of the keyboard
> disappearing on the unfocus event. But it still runs into the problem you're
> trying to fix here. Instead of piling more complexity here, could we delete
> this original workaround too and fix the problem closer to the root?
My thoughts exactly. I don't know how to improve Blink in this regard, though,
or even if it's possible. The defocus/focus logic is pretty far down it seems.
It's beyond my knowledge to know how to adjust at all, let alone safely. Is
there a Blink expert who could help?
I could put the code in the AdapterInputConnection. I originally started with
it there but moved it to the ImeAdapter because it seemed more closely related
(and I was hoping to tie in to the object, not the factory with via a static
variable).
As for tests... I looked into that but couldn't think of anything to test.
It's a brief flicker in the displayed keyboard on older versions of Android. I
don't think there is any way to capture that in a test and it certainly wouldn't
be stable (i.e. non-flaky) across systems.
On 2015/05/12 at 14:49:00, bcwhite wrote: > My thoughts exactly. I don't know how to improve Blink in this regard, though, or even if it's possible. The defocus/focus logic is pretty far down it seems. It's beyond my knowledge to know how to adjust at all, let alone safely. Is there a Blink expert who could help? Well, Tokyo text editing experts like yosin@ (cc'ed, although he's OOO this week) are probably the best people for that. I'm not sure if anyone will be able to solve the problem without investigation though. In my opinion, Blink isn't that much more rocket sciency than the Android-level code and you could dig in there and experiment with it until you start to form an opinion of how to do it cleanly, and see what the Blink folks think of it.
yutak@ is the Blink selection OWNER so cc'ing him as well.
I'd like to submit this and get the fix in and then remove it (and the delayed keyboard dismissal) if a Blink fix can be done.
I disagree, let's not add more hacks. Even with urgent issues, I generally say we should take a few hours to fully understand an issue and land the best patch that we can, and this is not urgent.
I've checked this against Google Keyboard, Swype, SwiftKey, and Samsung Keyboard with no side effects. A better fix involving Blink would be a better long-term solution but this will do for now and it's easy to remove (along with delayed keyboard-close). I'll open a new Issue to track that.
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138103003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e8e5d60be5f3b36634a1b012ec21874a4abe0ef0 Cr-Commit-Position: refs/heads/master@{#330551}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1148503003/ by aelias@chromium.org. The reason for reverting is: Patch landed despite rejection from code reviewers..
Message was sent while issue was closed.
Brian, this patch was opposed by both me and aurimas@. My opposition was clear, and aurimas's "green" comment #7 was not intended as an approval but just a case of accidental string matching. He said "I am just worried that this will somehow cause more bugs for us. It does not seem correct that we report the wrong input type. Could we ask Google Keyboard team to make sure this will not break things for them? If they are OK with it then LGTM." That is not a basis for landing. To be clear, landing against clear reviewer opposition is unprofessional. This is the first time I've had to revert a patch for this reason in my 7 years at Google.
Message was sent while issue was closed.
On 2015/05/19 22:06:56, aelias_OOO_until_May29 wrote: > Brian, this patch was opposed by both me and aurimas@. My opposition was clear, > and aurimas's "green" comment #7 was not intended as an approval but just a case > of accidental string matching. He said "I am just worried that this will > somehow cause more bugs for us. It does not seem correct that we report the > wrong input type. Could we ask Google Keyboard team to make sure this will not > break things for them? If they are OK with it then LGTM." That is not a basis > for landing. > > To be clear, landing against clear reviewer opposition is unprofessional. This > is the first time I've had to revert a patch for this reason in my 7 years at > Google. Back off. I had approval and I addressed Aurimas's comments. The change does not affect keyboard operation. The same calls are made, just without changing the configuration. It may not have been how you would have done it but it was a legitimate way to do it and consistent with existing code for handling the transient condition. I also addressed, by opening a new issue, your concerns about finding a better way to handle this and the delayed dismiss of the keyboard but your messages says OOO until the end of May so there was little point in expecting feedback from you. You're also being hypocritical as you just went and reverted the patch without any discussion. So in essence, you did the very same thing you're criticizing me about. Sadly, this isn't the first time I've encountered this in my 8 years at Google. |
