Clear focus when keyboard is dismissed

VERIFIED FIXED in Firefox 34

Status

Firefox for Android Graveyard
Search Activity
VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: eedens, Assigned: eedens)

Tracking

unspecified
Firefox 34
All
Android

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8477457 [details] [diff] [review]
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)
Attachment #8477457 - Flags: review?(margaret.leibovic)
Attachment #8477457 - Flags: feedback?(alam)
(Assignee)

Comment 2

4 years ago
APK link: http://goo.gl/H5MVXY
Nice! It feels right to me right now :)

Updated

4 years ago
Attachment #8477457 - Flags: feedback?(alam) → feedback+

Comment 4

4 years ago
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-
(Assignee)

Comment 5

4 years ago
(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)

Comment 6

4 years ago
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)

Updated

4 years ago
Attachment #8477457 - Flags: review- → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8480098 [details] [diff] [review]
bug-1057390.v2.patch

Thanks Margaret! I created a fresh patch for landing.
Attachment #8477457 - Attachment is obsolete: true
Attachment #8480098 - Flags: review?(margaret.leibovic)

Updated

4 years ago
Attachment #8480098 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a4525382248e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Comment 11

4 years ago
Verified as fixed in build 35.0a1 (2014-10-02);
Device Nexus 7 (Android 4.4.4);
Status: RESOLVED → VERIFIED
status-firefox35: --- → verified

Updated

4 months ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.