Closed Bug 1041604 Opened 6 years ago Closed 6 years ago

Update search bar when user taps a suggestion in the web view results

Categories

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

All
Android
defect

Tracking

(firefox34 affected)

RESOLVED FIXED
Firefox 35
Tracking Status
firefox34 --- affected

People

(Reporter: eedens, Assigned: Margaret)

References

Details

(Whiteboard: shovel-ready)

Attachments

(1 file, 1 obsolete file)

The text in the search bar should always represent the user's current query. Users can initiate a search in three ways:
  1. From the search bar
  2. From a history card
  3. From a suggestion within the WebView itself.

Cases (2) and (3) do not currently update the text in the search bar.
Case (2) is fixed by bug 1041026.

The WebView case is tricky, since we would need to get something out of the web content. Have we thought about how we'd want to do that?
In the case of Yahoo, the query is embedded in the URL as a query param. We already inspect the URL during a navigation event [1]; we'd need to add a step to extract the actual query.

Potential issue: the query param could change. We already need a mechanism to update CSS selectors for hiding the page's text input. So we could also update the URL pattern OTA.

[1] https://lxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java#74
Status: NEW → ASSIGNED
Updating this bug to cover the scope of what's left here.

Let's see if we can just inspect the URL that we're loading in the web view when the user clicks another one of these results pages.
Priority: -- → P1
Summary: Ensure that search bar always has the correct query → Update search bar when user taps a suggestion in the web view results
Depends on: 1056292
Note: We can extract the user's current query by using the {searchTerms} param from the open search files, http://dxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/searchplugins/bing.xml#20
Back into the pool ..

One approach: watch for navigation events in the webview; if search term's query param changes, then update the search bar.

Query param: https://lxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java#35
Webview nav event: https://lxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java#104
Whiteboard: shovel-ready
Assignee: eric.edens → nobody
Assignee: nobody → margaret.leibovic
(In reply to Eric Edens [:eedens] from comment #5)
> Back into the pool ..
> 
> One approach: watch for navigation events in the webview; if search term's
> query param changes, then update the search bar.

I have a rough version of this approach working for Yahoo and Bing, but it looks like Google is using JS to change their page contents, so we're not getting a navigation event to update the search bar :(

I'm not sure if there's a good way around this, so maybe we should just go ahead with the current approach, and just try to address it if we ever make Google the default provider.
I built this on top of my patch for bug 1064152.

wesj, what do you think of this patch? I don't really like the way it pokes back into MainActivity from PostSearchFragment, but I couldn't think of another way to do it. Maybe make some sort of event?
Attachment #8494068 - Flags: feedback?(wjohnston)
Comment on attachment 8494068 [details] [diff] [review]
WIP - Update query in search bar when user navigates to new results page

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

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +211,5 @@
> +     */
> +    public String queryForResultsUrl(String url) {
> +        final Set<String> names = resultsUri.getQueryParameterNames();
> +        for (String name : names) {
> +            if (resultsUri.getQueryParameter(name).equals("{searchTerms}")) {

Is there a reason this "{searchTerms}" isn't a const somewhere?
Attachment #8494068 - Flags: feedback?(wjohnston) → feedback+
(In reply to Wesley Johnston (:wesj) from comment #8)
> Comment on attachment 8494068 [details] [diff] [review]
> WIP - Update query in search bar when user navigates to new results page
> 
> Review of attachment 8494068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
> @@ +211,5 @@
> > +     */
> > +    public String queryForResultsUrl(String url) {
> > +        final Set<String> names = resultsUri.getQueryParameterNames();
> > +        for (String name : names) {
> > +            if (resultsUri.getQueryParameter(name).equals("{searchTerms}")) {
> 
> Is there a reason this "{searchTerms}" isn't a const somewhere?

Mostly because this was just a WIP :)

We have a regex we use for formatting the results URI, I can try using that here as well:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java#35
Let's just try going with this for now.
Attachment #8494068 - Attachment is obsolete: true
Attachment #8496141 - Flags: review?(wjohnston)
Attachment #8496141 - Flags: review?(wjohnston) → review+
Comment on attachment 8496141 [details] [diff] [review]
Update query in search bar when user navigates to new results page

># HG changeset patch
># User Margaret Leibovic <margaret.leibovic@gmail.com>
># Date 1411757745 25200
>#      Fri Sep 26 11:55:45 2014 -0700
># Node ID aef150240525051c51aabc3c74c9190de3c0c8ba
># Parent  26f99dd8b6520e52bfa296c547beb05446d1fd7b
>Bug 1041604 - Update query in search bar when user navigates to new results page
>
>diff --git a/mobile/android/search/java/org/mozilla/search/AcceptsSearchQuery.java b/mobile/android/search/java/org/mozilla/search/AcceptsSearchQuery.java
>--- a/mobile/android/search/java/org/mozilla/search/AcceptsSearchQuery.java
>+++ b/mobile/android/search/java/org/mozilla/search/AcceptsSearchQuery.java
>@@ -28,14 +28,21 @@ public interface AcceptsSearchQuery {
>      * Starts a search and animates a suggestion.
>      *
>      * @param query
>      * @param suggestionAnimation
>      */
>     void onSearch(String query, SuggestionAnimation suggestionAnimation);
> 
>     /**
>+     * Handles a change to the current search query.
>+     *
>+     * @param query
>+     */
>+    void onQueryChange(String query);
>+
>+    /**
>      * Interface to specify search suggestion animation details.
>      */
>     public interface SuggestionAnimation {
>         public Rect getStartBounds();
>     }
> }
>diff --git a/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java b/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
>--- a/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
>+++ b/mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
>@@ -90,18 +90,28 @@ public class PostSearchFragment extends 
>         @Override
>         public void onPageStarted(WebView view, final String url, Bitmap favicon) {
>             // Reset the error state.
>             networkError = false;
>         }
> 
>         @Override
>         public boolean shouldOverrideUrlLoading(WebView view, String url) {
>-            // We keep URLs in the webview that are either about:blank or a search engine result page.
>-            if (TextUtils.equals(url, Constants.ABOUT_BLANK) || engine.isSearchResultsPage(url)) {
>+            // Ignore about:blank URL loads.
>+            if (TextUtils.equals(url, Constants.ABOUT_BLANK)) {
>+                return false;
>+            }
>+
>+            // If the URL is a results page, don't override the URL load, but
>+            // do update the query in the search bar if possible.
>+            if (engine.isSearchResultsPage(url)) {
>+                final String query = engine.queryForResultsUrl(url);
>+                if (!TextUtils.isEmpty(query)) {
>+                    ((AcceptsSearchQuery) getActivity()).onQueryChange(query);
>+                }
>                 return false;
>             }
> 
>             try {
>                 // If the url URI does not have an intent scheme, the intent data will be the entire
>                 // URI and its action will be ACTION_VIEW.
>                 final Intent i = Intent.parseUri(url, Intent.URI_INTENT_SCHEME);
> 
>diff --git a/mobile/android/search/java/org/mozilla/search/SearchActivity.java b/mobile/android/search/java/org/mozilla/search/SearchActivity.java
>--- a/mobile/android/search/java/org/mozilla/search/SearchActivity.java
>+++ b/mobile/android/search/java/org/mozilla/search/SearchActivity.java
>@@ -242,16 +242,21 @@ public class SearchActivity extends Loca
>             animateSuggestion(suggestionAnimation);
>         } else {
>             // Otherwise immediately switch to the results view.
>             setEditState(EditState.WAITING);
>             setSearchState(SearchState.POSTSEARCH);
>         }
>     }
> 
>+    @Override
>+    public void onQueryChange(String query) {
>+        searchBar.setText(query);
>+    }
>+
>     private void startSearch(final String query) {
>         if (engine != null) {
>             postSearchFragment.startSearch(engine, query);
>             return;
>         }
> 
>         // engine will only be null if startSearch is called before the getEngine
>         // call in onCreate is completed.
>diff --git a/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java b/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
>--- a/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
>+++ b/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
>@@ -9,16 +9,17 @@ import android.util.Log;
> import android.util.Xml;
> 
> import org.xmlpull.v1.XmlPullParser;
> import org.xmlpull.v1.XmlPullParserException;
> 
> import java.io.IOException;
> import java.io.InputStream;
> import java.util.Locale;
>+import java.util.Set;
> 
> /**
>  * Extend this class to add a new search engine to
>  * the search activity.
>  */
> public class SearchEngine {
>     private static final String LOG_TAG = "SearchEngine";
> 
>@@ -198,16 +199,32 @@ public class SearchEngine {
>      * Determine whether a particular url belongs to this search engine. If not,
>      * the url will be sent to Fennec.
>      */
>     public boolean isSearchResultsPage(String url) {
>         return resultsUri.getAuthority().equalsIgnoreCase(Uri.parse(url).getAuthority());
>     }
> 
>     /**
>+     * Finds the search query encoded in a given results URL.
>+     *
>+     * @param url Current results URL.
>+     * @return The search query, or an empty string if a query couldn't be found.
>+     */
>+    public String queryForResultsUrl(String url) {
>+        final Set<String> names = resultsUri.getQueryParameterNames();
>+        for (String name : names) {
>+            if (resultsUri.getQueryParameter(name).matches(OS_PARAM_USER_DEFINED)) {
>+                return Uri.parse(url).getQueryParameter(name);
>+            }
>+        }
>+        return "";
>+    }
>+
>+    /**
>      * Create a uri string that can be used to fetch the results page.
>      *
>      * @param query The user's query. This method will escape and encode the query.
>      */
>     public String resultsUriForQuery(String query) {
>         if (resultsUri == null) {
>             Log.e(LOG_TAG, "No results URL for search engine: " + identifier);
>             return "";
https://hg.mozilla.org/mozilla-central/rev/c3793a1dd33b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Depends on: 1114589
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.