Closed
Bug 1057390
Opened 11 years ago
Closed 11 years ago
Clear focus when keyboard is dismissed
Categories
(Firefox for Android Graveyard :: Search Activity, defect)
Tracking
(firefox35 verified)
VERIFIED
FIXED
Firefox 34
Tracking | Status | |
---|---|---|
firefox35 | --- | verified |
People
(Reporter: eedens, Assigned: eedens)
References
Details
Attachments
(1 file, 1 obsolete file)
3.41 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 years ago
|
||
APK link: http://goo.gl/H5MVXY
Comment 3•11 years ago
|
||
Nice! It feels right to me right now :)
Updated•11 years ago
|
Attachment #8477457 -
Flags: feedback?(alam) → feedback+
Comment 4•11 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•11 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•11 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•11 years ago
|
Attachment #8477457 -
Flags: review- → review+
Assignee | ||
Comment 7•11 years ago
|
||
Thanks Margaret! I created a fresh patch for landing.
Attachment #8477457 -
Attachment is obsolete: true
Attachment #8480098 -
Flags: review?(margaret.leibovic)
Updated•11 years ago
|
Attachment #8480098 -
Flags: review?(margaret.leibovic) → review+
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 11•11 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•7 years 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.
Description
•