Closed Bug 1046972 Opened 5 years ago Closed 5 years ago

Hide settings button when keyboard is active

Categories

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

x86
Android
defect

Tracking

(firefox35 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox35 --- verified

People

(Reporter: antlam, Assigned: eedens)

References

Details

Attachments

(2 files, 2 obsolete files)

I know some mocks I made earlier had this, but after discussion with Ian, we want to remove it so I'm filing this bug to track it
Blocks: search
Priority: -- → P1
Assignee: nobody → eric.edens
Summary: Remove 3-dot overflow when keyboard is active → Hide settings button when keyboard is active
Hey Anthony,

What do you think of the show / hide animation for the settings icon?

APK for bug 1046972, bug 1053208: http://goo.gl/XeFQfY
Flags: needinfo?(alam)
Looking good Eric!

What's the duration on this fade right now? It's a little bit slow...
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #2)
> Looking good Eric!
> 
> What's the duration on this fade right now? It's a little bit slow...

It was 1 second for showing and 0.2 seconds for hiding. Here's a version where each value is halved: http://goo.gl/e7BVfw

I noticed that the back button needs to be pressed twice to clear the focus from the search bar. Is that the intended behavior? Or should we file another bug?
Flags: needinfo?(alam)
(In reply to Eric Edens [:eedens] from comment #3)
> It was 1 second for showing and 0.2 seconds for hiding. Here's a version
> where each value is halved: http://goo.gl/e7BVfw

I think 0.25 for both showing and hiding is better. Appearing still feels slow right now on this build..

> I noticed that the back button needs to be pressed twice to clear the focus
> from the search bar. Is that the intended behavior? Or should we file
> another bug?

I noticed this too. I don't think all Android devices have this duality of function for the 'back' button do they? The awkwardness for me is when the Search field is still selected, but the keyboard is down (not active). I think the settings cog should still appear (and be tap-able). 

Can we try that? :)
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #4)
> (In reply to Eric Edens [:eedens] from comment #3)
> > It was 1 second for showing and 0.2 seconds for hiding. Here's a version
> > where each value is halved: http://goo.gl/e7BVfw
> 
> I think 0.25 for both showing and hiding is better. Appearing still feels
> slow right now on this build..

Alright, 0.25 seconds for each: http://goo.gl/OgQbsr

> 
> > I noticed that the back button needs to be pressed twice to clear the focus
> > from the search bar. Is that the intended behavior? Or should we file
> > another bug?
> 
> I noticed this too. I don't think all Android devices have this duality of
> function for the 'back' button do they? The awkwardness for me is when the
> Search field is still selected, but the keyboard is down (not active). I
> think the settings cog should still appear (and be tap-able). 
> 
> Can we try that? :)

Is there a reason to keep the search bar focused while the keyboard is hidden?

I propose, for another bug: When the keyboard is active, and the back button is pressed, we should: 
 - Hide the keyboard
 - Remove focus from the search input
 - Display any ui elements that were obscured by the keyboard (eg: settings button).
Flags: needinfo?(alam)
(In reply to Eric Edens [:eedens] from comment #5)
> I propose, for another bug: When the keyboard is active, and the back button
> is pressed, we should: 
>  - Hide the keyboard
>  - Remove focus from the search input
>  - Display any ui elements that were obscured by the keyboard (eg: settings
> button).

I would agree that changing it back to the way Android used to do it (with the 'back' button not turning into the 'down' arrow when keyboard is up but staying as a 'back' arrow) seems like it makes sense.

Though I have concerns that it might get confusing since we're now talking about deviating from the (now) Android standard interactions. 

Sure, let's track that in another bug cause now that I run it through my head there seems to be more to it.
Flags: needinfo?(alam)
Attached patch bug-1046972.v1.patch (obsolete) — Splinter Review
Alright, this is the patch for the final APK (the one that Anthony liked). The transition is 250 ms for appearing and hiding.
Attachment #8477003 - Flags: review?(margaret.leibovic)
Comment on attachment 8477003 [details] [diff] [review]
bug-1046972.v1.patch

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

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +210,5 @@
>      }
>  
>      @Override
>      public void onSuggest(String query) {
> +        editText.setText(query);

Converted 3 spaces to 4.

@@ +239,5 @@
>      /**
>       * Animates search suggestion to search bar. This animation has 2 main parts:
>       *
> +     * 1) Vertically translate query text from suggestion card to search bar.
> +     * 2) Expand suggestion card to fill the results view area.

Ugh, Android studio kept removing this indent when re-tabbing the code, and I couldn't find a way around it. I can hand remove this if you don't like the new formatting.
Created bug 1057390 for ensuring the search bar loses focus when the keyboard is dismissed.
(In reply to Eric Edens [:eedens] from comment #8)
> Comment on attachment 8477003 [details] [diff] [review]
> bug-1046972.v1.patch
> 
> Review of attachment 8477003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> @@ +210,5 @@
> >      }
> >  
> >      @Override
> >      public void onSuggest(String query) {
> > +        editText.setText(query);
> 
> Converted 3 spaces to 4.

Thanks for clarifying (it's hard to notice the difference without looking at the raw diff :)

> @@ +239,5 @@
> >      /**
> >       * Animates search suggestion to search bar. This animation has 2 main parts:
> >       *
> > +     * 1) Vertically translate query text from suggestion card to search bar.
> > +     * 2) Expand suggestion card to fill the results view area.
> 
> Ugh, Android studio kept removing this indent when re-tabbing the code, and
> I couldn't find a way around it. I can hand remove this if you don't like
> the new formatting.

This is fine.
Comment on attachment 8477003 [details] [diff] [review]
bug-1046972.v1.patch

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

Looking good! I'd just like to see us clean up the updateSettingsButtonVisibililty method to make this less confusing for future developers (ourselves included!).

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +337,5 @@
> +     * button with alpha zero will not capture clicks.
> +     *
> +     * @param animate whether to animate the transition.
> +     */
> +    private void updateSettingsButtonVisibility(boolean animate) {

This is a confusing method signature! Reading where it's used, I thought the boolean was whether or not to make it visible. This is the main reason boolean paramters are confusing.

I think it would be easier to understand if we passed in two parameters (one for the visibility and one for the animation), or maybe we should have two methods: updateSettingsButtonVisibility() and animateSettingsButtonVisibility(). But since we'd want to share some code, we'd need to make another helper method for the shared bits.

@@ +355,5 @@
> +
> +            // Setting alpha here isn't strictly required, but a future call
> +            // that goes from hidden -> visible would show a quick flicker on
> +            // during the next block's call to setVisibility.
> +            ViewHelper.setAlpha(settingsButton, endAlpha);

Instead of setting both the visibiility and the alpha, could we just change the alpha value? Is there a way to do that without interfering with clicks?

@@ +360,5 @@
> +            return;
> +        }
> +
> +        if (shouldBeVisible) {
> +            settingsButton.setVisibility(newVisiblity);

You could just directly pass View.VISIBLE here instead of newVisibiilty. It would make it a bit easier to understand why we're only doing this in the shouldBeVisible case.

@@ +369,5 @@
> +        objectAnimator.addListener(new AnimatorListenerAdapter() {
> +            @Override
> +            public void onAnimationEnd(Animator animation) {
> +                // Retain the final alpha value.
> +                ViewHelper.setAlpha(settingsButton, endAlpha);

The animation doesn't automatically do this for us?

@@ +371,5 @@
> +            public void onAnimationEnd(Animator animation) {
> +                // Retain the final alpha value.
> +                ViewHelper.setAlpha(settingsButton, endAlpha);
> +                if (!shouldBeVisible) {
> +                    settingsButton.setVisibility(newVisiblity);

Same thing here, I would just directly pass View.GONE.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ -118,5 @@
> -        // Apply click handler to settings button.
> -        mainView.findViewById(R.id.settings_button).setOnClickListener(new View.OnClickListener() {
> -            @Override
> -            public void onClick(View v) {
> -                startActivity(new Intent(getActivity(), SearchPreferenceActivity.class));

Double check to remove any unused imports.
Attachment #8477003 - Flags: review?(margaret.leibovic) → feedback+
Comment on attachment 8477003 [details] [diff] [review]
bug-1046972.v1.patch

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

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +337,5 @@
> +     * button with alpha zero will not capture clicks.
> +     *
> +     * @param animate whether to animate the transition.
> +     */
> +    private void updateSettingsButtonVisibility(boolean animate) {

I'm not a huge fan of boolean params either.  I'd be fine with enum params (eg: ANIMATE, NO_ANIMATE) though.

Regarding passing in the visibility, the visibility depends on both the edit state and the search state. So the visibility logic would spew into setEditState and setSearchState, and those methods would now know about each other's state. Although messy, the visibility logic can be contained in a single line here:

  searchState == SearchState.PRESEARCH && editState == EditState.WAITING

@@ +355,5 @@
> +
> +            // Setting alpha here isn't strictly required, but a future call
> +            // that goes from hidden -> visible would show a quick flicker on
> +            // during the next block's call to setVisibility.
> +            ViewHelper.setAlpha(settingsButton, endAlpha);

The only other way that I could think of would be to toggle some other property, such as clickable. The benefit of using View.GONE -- albeit small in this case -- is that the button won't participate in layout calls when it's hidden.

@@ +360,5 @@
> +            return;
> +        }
> +
> +        if (shouldBeVisible) {
> +            settingsButton.setVisibility(newVisiblity);

sounds good

@@ +369,5 @@
> +        objectAnimator.addListener(new AnimatorListenerAdapter() {
> +            @Override
> +            public void onAnimationEnd(Animator animation) {
> +                // Retain the final alpha value.
> +                ViewHelper.setAlpha(settingsButton, endAlpha);

Good call -- I didn't realize that had changed from the older animations (http://developer.android.com/reference/android/view/animation/Animation.html#setFillAfter%28boolean%29)

@@ +371,5 @@
> +            public void onAnimationEnd(Animator animation) {
> +                // Retain the final alpha value.
> +                ViewHelper.setAlpha(settingsButton, endAlpha);
> +                if (!shouldBeVisible) {
> +                    settingsButton.setVisibility(newVisiblity);

Sounds good

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ -118,5 @@
> -        // Apply click handler to settings button.
> -        mainView.findViewById(R.id.settings_button).setOnClickListener(new View.OnClickListener() {
> -            @Override
> -            public void onClick(View v) {
> -                startActivity(new Intent(getActivity(), SearchPreferenceActivity.class));

good catch :)
Attached patch bug-1046972.v2.patch (obsolete) — Splinter Review
Alright! Your suggestions definitely helped make this easier to understand.
Attachment #8477003 - Attachment is obsolete: true
Attachment #8477998 - Flags: review?(margaret.leibovic)
Comment on attachment 8477998 [details] [diff] [review]
bug-1046972.v2.patch

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

r+ with comments addressed. However, you may need to post a new patch for review if you find this race issue is a problem.

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +348,5 @@
> +        final boolean shouldBeVisible = searchState == SearchState.PRESEARCH && editState == EditState.WAITING;
> +        final int newVisiblity = shouldBeVisible ? View.VISIBLE : View.GONE;
> +
> +        // Quit if this wouldn't change the visibility.
> +        if (newVisiblity == settingsButton.getVisibility()) {

I think there is a race condition here if you're animating the button from VISIBLE to GONE, since the view's visibility won't change until the animation is done. Do you see any weirdness if you quickly exit/enter editing mode? Setting the animation duration to a long time would probably help you reproduce the race if it exists.

If this race does exist, the button visibility could get into the wrong state. For example, if we're animating it to GONE when the keyboard opens, but the user closes the keyboard before the animation finishes, we would bail early and the button wouldn't get set to VISIBLE.

@@ +361,5 @@
> +
> +            // Setting alpha here isn't strictly required, but a future call
> +            // that goes from hidden -> visible would show a quick flicker on
> +            // during the next block's call to setVisibility.
> +            ViewHelper.setAlpha(settingsButton, endAlpha);

The comment here isn't totally right. Yes, this is important to avoid a flicker before the animation starts, but if you're trying to set the view to visible, but its alpha value is 0, it wouldn't appear. So this is actually necessary for making this visible.

You should update the comment to describe this, since if it was only necessary for avoiding a flicker, it could be moved down below right above the setVisibility call.
Attachment #8477998 - Flags: review?(margaret.leibovic) → review+
Good catch on the race condition. Let's avoid this for the moment by punting on the animation. With anthony's design to put branding on the bottom of the page as well, we'll need a more robust plan anyway (some kind of bottom-of-the-screen toolbar?)

(The follow-up bug for the animation is bug 1059443)
Attachment #8477998 - Attachment is obsolete: true
Attachment #8480056 - Flags: review?(margaret.leibovic)
Attachment #8480056 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a1e51bc80bdb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Verified as fixed in build 35.0a1 (2014-09-08);
Devices: 
Google Nexus 7 (Android 4.4.4);
Samsung Galaxy R (Android 2.3.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.