1057390 - Clear focus when keyboard is dismissed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Eric Edens [:eedens]]

Currently it requires two back button taps to clear the focus on the search bar. We should match the behaviour of Fennec and only require one tap.

Here's the relevant code for Fennec's search bar:

http://lxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarEditText.java#536

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1057390.patch

Hey Anthony, here's an APK showing the interaction. If it's alright, can you give an f+?  (It doesn't include the code to hide the settings button)

Comment 2

[Eric Edens [:eedens]]

APK link: http://goo.gl/H5MVXY

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Nice! It feels right to me right now :)

Comment 4

[:Margaret Leibovic]

Comment on attachment 8477457 [details] [diff] [review]
bug-1057390.patch

Review of attachment 8477457 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with the logic, but I think we can use CustomEditText to do this without adding a new class to the tree.

::: mobile/android/search/java/org/mozilla/search/ui/BackCaptureEditText.java
@@ +30,5 @@
> +    public boolean onKeyPreIme(int keyCode, KeyEvent event) {
> +        if (event.getAction() == KeyEvent.ACTION_UP && keyCode == KeyEvent.KEYCODE_BACK) {
> +            clearFocus();
> +        }
> +        return super.onKeyPreIme(keyCode, event);

Instead of creating a new class, I think we should just use gecko's CustomEditText:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/CustomEditText.java

This is actually the same reason we originally created that class :)
http://hg.mozilla.org/mozilla-central/rev/99ce4d0052cc

Also, that should probably be moved into /widget, since it doesn't really belong in /base. But that could be a separate bug.

Comment 5

[Eric Edens [:eedens]]

(In reply to :Margaret Leibovic from comment #4)

> I agree with the logic, but I think we can use CustomEditText to do this
> without adding a new class to the tree.

I thought about this, but I didn't think the tradeoff is worth it.

First, CustomEditText doesn't offer any code savings. It allows you to easily add a PreIme listener -- we'd still need to implement the actual listener logic. The real code saving would be from ToolbarEditText.KeyPreImeListener, but that's private and the containing class has a ton of extra logic and dependencies.

Second, we'd either be adding CustomEditText along with its chain of dependencies -- which includes the generated class ThemedEditText.java.in. So our choice is to create a skeleton version of CustomEditText, and commit to keeping it in sync, or bring over all of CustomEditText's dependencies. (For example, if we later change the package of CustomEditText, then this will need to get updated in our tree)

In summary, I think this is a finkle ninja way of solving the problem :)

Comment 6

[:Margaret Leibovic]

Valid point about the ThemedEditText dependency. We would need to make a copied version of CustomEditText in the github repo no matter what, but I agree making a different stubbed version would add an extra layer of complexity.

Thinking about your patch more, and thinking about the additional stuff CustomEditText does that we won't need, I'll change my review to an r+.

The main point of my previous review is that I do not want to let maintaining the standalone activity get in the way of what we would do if we were working purely from mozilla-central. And if we were working purely from mozilla-central, I don't know that we would add a new custom widget here. Yes, we would need to add the listener, but that's a small addition to ClearableEditText, as opposed to creating a new UI widget.

Comment 7

[Eric Edens [:eedens]]

Attachment: bug-1057390.v2.patch

Thanks Margaret! I created a fresh patch for landing.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/a4525382248e

Comment 9

[Eric Edens [:eedens]]

https://github.com/mozilla/fennec-search/commit/b7c5c9431fdac62ee48633bb0cb2cf9b45beda80

Comment 10

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/a4525382248e

Comment 11

[Flaviu Cos]

Verified as fixed in build 35.0a1 (2014-10-02);
Device Nexus 7 (Android 4.4.4);