Looks like we need to update the item click listener to handle the search suggestion case: https://hg.mozilla.org/projects/fig/annotate/tip/mobile/android/base/home/BrowserSearch.java#l228

Actually, it looks like this should be handled in SearchEngineRow: https://hg.mozilla.org/projects/fig/annotate/tip/mobile/android/base/home/SearchEngineRow.java#l72 I wonder why it's not working.

Ooh, it works if you tap on the suggestion itself, but not on the whole row. Is that intentional?

(In reply to :Margaret Leibovic from comment #2) > Ooh, it works if you tap on the suggestion itself, but not on the whole row. > Is that intentional? Actually, this makes sense. I was just seeing rows with only only suggestion, so I didn't think about the fact that their could be multiple hit targets in the row. My bad!

On Nightly, it works so that if there's only one suggestion and you tap the row itself, it clicks the suggestion; if there's multiple suggestions, tapping the row does nothing. Does tapping the row not work for you if there's only one suggestion? If so, I think that should be fixed.

I see this too...this should be fixed. Also related: I see a CursorIndexOutOfBoundsException when there's only one suggestion shown and its row is long-pressed.

Thanks for the extra info. I'll look into fixing this.

This patch makes the click behavior match mozilla-central. In onItemClick, if the item is a SearchEngineRow, it will never have a URL to open, so we can always just let SearchEngineRow handle the click itself and bail early. I feel it makes the most sense for SearchEngineRow to handle this click itself, since it needs information about the search engine and suggestions. While working on this, I also noticed another bug that taps on filtered history items will have an off-by-one error if we're showing search suggestions. I'll file another bug about that, too. I'll also handle the CursorIndexOutOfBoundsException in another bug.

Comment on attachment 784652 [details] [diff] [review] patch Review of attachment 784652 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/SearchEngineRow.java @@ +119,5 @@ > + public void onClick() { > + // Don't do anything if we are showing suggestions. > + if (mSearchEngine.suggestions.size() > 0) { > + return; > + } Drive by: this check may not be necessary since the row should be enabled only if there are no suggestions (http://hg.mozilla.org/projects/fig/file/329abf307997/mobile/android/base/home/BrowserSearch.java#l666).

Comment on attachment 784652 [details] [diff] [review] patch Review of attachment 784652 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but I wonder: this ad-hoc onClick method bugs me a bit because it requires you to typecast stuff around i.e. instanceof diminishes our code's type-safety. It would be nice if SearchEngineRow simply added a click listener to itself that does exactly what you implemented in your onClick() method. But then in onItemClick you'd do something like this instead: @Override public void onItemClick(AdapterView<?> parent, View view, int position, long id) { if (mAdapter.getViewType(position) == SearchAdapter.ROW_SEARCH) { view.performClick(); return; } ... } No need to know the type of views involved. The drawback is that the code to handle clicks becomes a bit more indirect. Anyway, just throwing some thoughts here. Up to you. ::: mobile/android/base/home/BrowserSearch.java @@ +226,5 @@ > > mList.setOnItemClickListener(new AdapterView.OnItemClickListener() { > @Override > public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > + if (view instanceof SearchEngineRow) { I guess it would be more robust to check the view type for this position instead of instanceof. I don't feel strongly about it though. Up to you. ::: mobile/android/base/home/SearchEngineRow.java @@ +119,5 @@ > + public void onClick() { > + // Don't do anything if we are showing suggestions. > + if (mSearchEngine.suggestions.size() > 0) { > + return; > + } What bnicholson said :-) @@ +121,5 @@ > + if (mSearchEngine.suggestions.size() > 0) { > + return; > + } > + > + // Otherwise, perform a search for the user entered term. You should probably do a null check on the listener before using it.

(In reply to Brian Nicholson (:bnicholson) (gone through August 11) from comment #8) > Comment on attachment 784652 [details] [diff] [review] > patch > > Review of attachment 784652 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/SearchEngineRow.java > @@ +119,5 @@ > > + public void onClick() { > > + // Don't do anything if we are showing suggestions. > > + if (mSearchEngine.suggestions.size() > 0) { > > + return; > > + } > > Drive by: this check may not be necessary since the row should be enabled > only if there are no suggestions > (http://hg.mozilla.org/projects/fig/file/329abf307997/mobile/android/base/ > home/BrowserSearch.java#l666). I found that my click listener still fires even if there are multiple suggestions, so it looks like this check is still necessary.

This is a bit more elegant... if we set a listener directly on SearchEngineRow, it handles the click before the OnItemClickListener in BrowserSearch gets the chance to try to handle it. As I mentioned in my last comment, I found this was still firing if there were suggestions being shown, so I kept the check to bail in that case.

Comment on attachment 785213 [details] [diff] [review] alternate patch Review of attachment 785213 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/home/SearchEngineRow.java @@ +121,5 @@ > + return; > + } > + > + // Otherwise, perform a search for the user entered term. > + String searchTerm = getSuggestionTextFromView(mUserEnteredView); This could probably be moved inside the 'if' block.

https://hg.mozilla.org/projects/fig/rev/9972d89f09bb Oh shoot, I forgot to address the last comment, I can push a follow-up to do that.