Refine interaction for exiting editing mode

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
Search Activity
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Splitting this off from bug 1045655.
(Assignee)

Comment 1

3 years ago
Created attachment 8473372 [details] [diff] [review]
Don't capture clicks to dismiss search bar

This patch removed the main view click handler, as well as the suggestions container.

Because the click handler is gone, I had to add a focus change listener to make sure to exit editing mode when the search bar loses focus.

The edit state in MainActivity still keeps track of whether or not the suggestions fragment is shown, but the fragment itself makes sure to only show the list of suggestions when there are actually items in it. Without this check, the empty suggestions fragment view would cover up whatever is underneath it, preventing the user from doing things like clicking on search history terms while the search bar is active.

This patch also addresses bug 1051983 by updating the suggestions fragment to take up the whole screen.
Attachment #8473372 - Flags: review?(eric.edens)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1051983

Comment 3

3 years ago
Created attachment 8473378 [details]
NPE - onLoaderReset

This was thrown from a local build (against fx-team).

Repro steps:
  - Launch search activity
  - Click on settings button
(Assignee)

Comment 4

3 years ago
Comment on attachment 8473372 [details] [diff] [review]
Don't capture clicks to dismiss search bar

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

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
@@ +176,5 @@
>  
>          @Override
>          public void onLoaderReset(Loader<List<Suggestion>> loader) {
>              autoCompleteAdapter.update(null);
> +            suggestionsList.setVisibility(View.INVISIBLE);

Oh, autoCompleteAdapter and suggestionsList have different lifecycles.

I think I can just get rid of this call, since suggestionsList is destroyed in this case anyway.
(Assignee)

Comment 5

3 years ago
(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8473372 [details] [diff] [review]
> Don't capture clicks to dismiss search bar
> 
> Review of attachment 8473372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/
> SuggestionsFragment.java
> @@ +176,5 @@
> >  
> >          @Override
> >          public void onLoaderReset(Loader<List<Suggestion>> loader) {
> >              autoCompleteAdapter.update(null);
> > +            suggestionsList.setVisibility(View.INVISIBLE);
> 
> Oh, autoCompleteAdapter and suggestionsList have different lifecycles.
> 
> I think I can just get rid of this call, since suggestionsList is destroyed
> in this case anyway.

Actually, this is wrong. In fact, calling autoCompleteAdapter.update(null) is also wrong. onLoaderReset is just called when the loader wants to release its data. In other places in the code where we use CursorLoaders, we need to clean up references to the Cursor, but in our case, we're not actually holding onto the loader data anywhere, so we can remove the entire body of this method.
Flags: needinfo?(eric.edens)
(Assignee)

Comment 6

3 years ago
Created attachment 8473966 [details] [diff] [review]
Don't capture clicks to dismiss search bar

Updated to address loader issue.
Attachment #8473372 - Attachment is obsolete: true
Attachment #8473372 - Flags: review?(eric.edens)
Attachment #8473966 - Flags: review?(eric.edens)

Comment 7

3 years ago
Comment on attachment 8473966 [details] [diff] [review]
Don't capture clicks to dismiss search bar

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

LGTM
Attachment #8473966 - Flags: review?(eric.edens) → review+

Updated

3 years ago
Flags: needinfo?(eric.edens)
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/35343a2e0abd
https://github.com/mozilla/fennec-search/commit/d975f187ccccce07bced660df1c3ebf72bf842d9

Comment 9

3 years ago
https://hg.mozilla.org/mozilla-central/rev/35343a2e0abd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.