Closed Bug 1057390 Opened 5 years ago Closed 5 years ago

Clear focus when keyboard is dismissed

Categories

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

All
Android
defect
Not set

Tracking

(firefox35 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox35 --- verified

People

(Reporter: eedens, Assigned: eedens)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch bug-1057390.patch (obsolete) — Splinter Review
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)
Attachment #8477457 - Flags: review?(margaret.leibovic)
Attachment #8477457 - Flags: feedback?(alam)
APK link: http://goo.gl/H5MVXY
Nice! It feels right to me right now :)
Attachment #8477457 - Flags: feedback?(alam) → feedback+
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.
Attachment #8477457 - Flags: review?(margaret.leibovic) → review-
(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 :)
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic)
Attachment #8477457 - Flags: review- → review+
Thanks Margaret! I created a fresh patch for landing.
Attachment #8477457 - Attachment is obsolete: true
Attachment #8480098 - Flags: review?(margaret.leibovic)
Attachment #8480098 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a4525382248e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Verified as fixed in build 35.0a1 (2014-10-02);
Device Nexus 7 (Android 4.4.4);
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.