701374 - Show go or search icon in awesomescreen field, as appropriate

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Madhava Enros [:madhava]]

We want to make it very clear to people, as they type in the url/search field on the awesomescreen, that they can both search and enter URLS.

People are used to entering URLs, so our focus has to be more on making it clear that they can also search from here.

An idea that's come up before, but that we should really do this time (given that we're only supporting on search engine at a time, so the solution is sufficient), is to show a go icon ( -> ) at the end of the field when what the user has entered is actually a URL. Othewise, as they type, we should show a search icon (a magnifier) to show that when they hit enter, or tap the icon, they will instead be initiating a search.

Comment 2

[Lucas Rocha (:lucasr)]

Taking as I had bug 701329 anyway.

Comment 3

[Lucas Rocha (:lucasr)]

Ian and Madhava, I need assets to implement this.

Comment 4

[Ian Barlow (:ibarlow)]

Attachment: Assets for GO and Search icons, for GB and ICS

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #4)
> Created attachment 574923 [details]
> Assets for GO and Search icons, for GB and ICS

Missing the mdpi images for ICS?

Comment 6

[Lucas Rocha (:lucasr)]

Madhava and Ian, if this icon is clickable, shouldn't it have a pressed state as well? Or are you ok with just keeping the same image, even when pressed?

Comment 7

[Ian Barlow (:ibarlow)]

Attachment: mdpi assets for ICS

Lucas, using the same image for both states (off and touch) is fine. Whatever focus area we would add would be covered by your finger, so it wouldn't be particularly useful.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #7)
> Created attachment 575435 [details]
> mdpi assets for ICS
> 
> Lucas, using the same image for both states (off and touch) is fine.
> Whatever focus area we would add would be covered by your finger, so it
> wouldn't be particularly useful.

Yeah, I assumed so. Just checking. Thanks.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Add macros for wrappers with return value

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Add native method to check creation of fixup URIs

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Show go or search icon in awesomescreen field

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 576130 [details] [diff] [review]
(2/3) Add native method to check creation of fixup URIs

You shouldn't need the class-level mURIFixup. Getting a service each time should be fast. It's already a singleton.

Also, I wonder if it would be more expedient to actually return the "fixed up" URL as a string (or null). That way we could send the proper string back to Gecko when we decide to navigate or search. No big deal I suppose, just a thought.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 576130 [details] [diff] [review] [diff] [details] [review]
> (2/3) Add native method to check creation of fixup URIs
> 
> You shouldn't need the class-level mURIFixup. Getting a service each time
> should be fast. It's already a singleton.

Given that we're calling this function as the user types on awesomebar, we want this call to be as fast as possible. I assumed it was not fast enough because of the way nsDocShell uses nsURIFixup (i.e. gets service instance once and reuse it everywhere).

Anyway, I can change canCreateFixupURI() to get service on each call if it doesn't impact performance.

> Also, I wonder if it would be more expedient to actually return the "fixed
> up" URL as a string (or null). That way we could send the proper string back
> to Gecko when we decide to navigate or search. No big deal I suppose, just a
> thought.

Returning the URL as string would add a little overhead in canCreateFixupURI to convert the nsURI to a jstring. Returning a boolean allows us to return slightly faster and Gecko will fixup the URL before loading it anyway.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Faster is better :)

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 576130 [details] [diff] [review]
(2/3) Add native method to check creation of fixup URIs

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

lets see one more patch.

Note - the widget changes and APKOpen changes need to be put on m-c.

::: widget/src/android/AndroidBridge.cpp
@@ +367,5 @@
>  {
>      ALOG_BRIDGE("AndroidBridge::NotifyAppShellReady");
>      mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jNotifyAppShellReady);
> +
> +    //CallGetService(NS_URIFIXUP_CONTRACTID, &mURIFixup);

remove comment

@@ +593,5 @@
> +
> +    if (!mURIFixup)
> +        return false;
> +
> +    nsCOMPtr<nsIURI> targetURI = nsnull;

you do not need to null a nsCOMPtr

@@ +595,5 @@
> +        return false;
> +
> +    nsCOMPtr<nsIURI> targetURI = nsnull;
> +
> +    nsresult rv = mURIFixup->CreateFixupURI(aURIText,

No need to check for result per contract.  "Returns a wellformed URI or nsnull if it can't."

::: widget/src/android/AndroidBridge.h
@@ +408,5 @@
>      jclass jEGLContextClass;
>      jclass jEGL10Class;
>  
> +    // Needed for canCreateFixupURI()
> +    nsCOMPtr<nsIURIFixup> mURIFixup;

Does this really need to be saved?  is it called that much?

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Doug Turner (:dougt) from comment #15)
> Comment on attachment 576130 [details] [diff] [review] [diff] [details] [review]
> (2/3) Add native method to check creation of fixup URIs
> 
> Review of attachment 576130 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> lets see one more patch.
> 
> Note - the widget changes and APKOpen changes need to be put on m-c.

Ok.

> ::: widget/src/android/AndroidBridge.cpp
> @@ +367,5 @@
> >  {
> >      ALOG_BRIDGE("AndroidBridge::NotifyAppShellReady");
> >      mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jNotifyAppShellReady);
> > +
> > +    //CallGetService(NS_URIFIXUP_CONTRACTID, &mURIFixup);
> 
> remove comment

Done.

> @@ +593,5 @@
> > +
> > +    if (!mURIFixup)
> > +        return false;
> > +
> > +    nsCOMPtr<nsIURI> targetURI = nsnull;
> 
> you do not need to null a nsCOMPtr

Fixed.

> @@ +595,5 @@
> > +        return false;
> > +
> > +    nsCOMPtr<nsIURI> targetURI = nsnull;
> > +
> > +    nsresult rv = mURIFixup->CreateFixupURI(aURIText,
> 
> No need to check for result per contract.  "Returns a wellformed URI or
> nsnull if it can't."

Fixed.

> ::: widget/src/android/AndroidBridge.h
> @@ +408,5 @@
> >      jclass jEGLContextClass;
> >      jclass jEGL10Class;
> >  
> > +    // Needed for canCreateFixupURI()
> > +    nsCOMPtr<nsIURIFixup> mURIFixup;
> 
> Does this really need to be saved?  is it called that much?

Yep. It's called each time user types on awesomebar text entry. So, calls to canCreateFixupURI() have to be really fast.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Add native method to check creation of fixup URIs

Doug, shall I land patches 1/3 and 2/3 on m-c? Or just 1/3?

Comment 18

[Doug Turner (:dougt)]

I pushed the m-c bits and merged back to birch:

https://hg.mozilla.org/mozilla-central/rev/0ea84b44a7f1
https://hg.mozilla.org/mozilla-central/rev/ae734929219b

Comment 19

[Doug Turner (:dougt)]

for example:

Dougs-MacBook-Air:birch dougt$ hg qimport https://bug701374.bugzilla.mozilla.org/attachment.cgi?id=576131
hgadding attachment.cgi?id=576131 to series file
Dougs-MacBook-Air:birch dougt$ hg qpush
applying attachment.cgi?id=576131
unable to find 'embedding/android/AwesomeBar.java' for patching
4 out of 4 hunks FAILED -- saving rejects to file embedding/android/AwesomeBar.java.rej
unable to find 'embedding/android/Makefile.in' for patching
3 out of 3 hunks FAILED -- saving rejects to file embedding/android/Makefile.in.rej
unable to find 'embedding/android/resources/layout/awesomebar_search.xml' for patching
1 out of 1 hunks FAILED -- saving rejects to file embedding/android/resources/layout/awesomebar_search.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=576131

Comment 20

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/projects/birch/rev/88679504d1df

Comment 21

[Camelia Urian]

Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111128 Firefox/11.0a1 Fennec/11.0a1
Device: HTC Desire Z
OS: Android 2.3

Only go icon(->) is displayed in awesomescreen field and if url is correctly entered then page is loaded.
Search icon is not displayed. If user types a random string("vbhdxbtet") and presses the go icon(->), the browser will try to load www.vbhdxbtet.com, but since the page does not exist: "Server not found" is displayed.

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Camelia Urian from comment #21)
> Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111128
> Firefox/11.0a1 Fennec/11.0a1
> Device: HTC Desire Z
> OS: Android 2.3
> 
> Only go icon(->) is displayed in awesomescreen field and if url is correctly
> entered then page is loaded.
> Search icon is not displayed. If user types a random string("vbhdxbtet") and
> presses the go icon(->), the browser will try to load www.vbhdxbtet.com, but
> since the page does not exist: "Server not found" is displayed.

Camelia, this is a slightly different issue. The default behaviour of Gecko is to try to create a URL out of a single typed word. So the current behaviour in AwesomeBar is consistent with that (i.e. if you type only a word, say, "google", Gecko will then try loading it as "www.google.com"). In order to implement the feature as described., we'd need to change Gecko's behaviour in Fennec first so that if you only type a word, it would do a web search.

So, I suggest you to file a separate bug like "Perform web search when user types only a word in AwesomeBar" and keep this one as fixed.

Comment 23

[Madhava Enros [:madhava]]

Hi guys -

I think we stopped trying to fixup the url on desktop because it's kind of confusing for the user; regardless, it's _really_ not appropriate for something that is a URL *and* search bar. Every time I type "wombat", for example, the button stays as an arrow (even though it's not a URL) and hitting go takes me to some geocities page about wombats that doesn't exist any more. How would I do a search from this bar in this case?

Comment 24

[Madhava Enros [:madhava]]

For reference, when I type "google" into my location bar in desktop nightly, I end up on a google search results page for the string "google" -- it doesn't just take me to google.com

Comment 26

[Frank Yan (:fryn)]

A heads-up: We're going to changing a bunch of things in nsIURIFixup in bug 700470 to make this work better for Firefox desktop too.

There's no hard dependency here, but since they are closely related and this will likely land before the other bug, I'll make it block that one.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Updated Summary:

As the user types, we want to change the UI to indicate whether a navigation or search is the expected outcome

* The UI changes happen in Java
* The search/navigate happens in Gecko

The simplest thing we can think of for the moment:
* As the user types, attempt to convert the string into a URL using Java helper classes
  * If it succeeds, show the "go" button
  * If it fails, show the "search" button

* When the user commits to the action (taps button or presses GO keyboard button)
  * Send the string, as is, to Gecko
  * Gecko needs to perform the same "can I convert this string to a URL" test as Java
    * If it succeeds, call loadURI
    * If it fails
      * Get the current search engine
      * Build a search submission string using the text passed to Gecko
      * call loadURI with the search submission string (and possible postData)

This plan does not use nsIURIFixup at all. Any feedback? How badly have I screwed up?

Frank's bug/patch has some nice ideas for doing a background lookup on a "fixed up" version of the string. If that succeeded, we could show a doorhanger "did you mean 'www.bacon.com' ?"

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: updated patch

This patch takes the simpler approach noted above:
* Backs out the JNI canCreateURIFixup method
* Adds a simple isSearchUrl based on the logic in nsDefaultURIFixup::KeywordURIFixup
  * It uses the index location of ' ', '.', and ':' in the string to guess if the string is possibly a real URL address or not.
* Based on the guess, we show/hide the go/search images
* We use isSearchUrl to test any potential URL string, since pressing GO on the keyboard can also start a load.

Seems to work OK for me in my simple testing. Remember, isSearchUrl does not try to validate the URL. We don't really care if the text is a valid URL or not. We just use it to make a good guess about whether to search or navigate.

Typing 'ebay.c' will attempt to load ebay.c not do a search for ebay.c

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 581188 [details] [diff] [review]
updated patch

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

I'd like to see the suggested refactoring in and the questions answered before I give a r+.

::: mobile/android/base/AwesomeBar.java
@@ +208,5 @@
>          cancelAndFinish();
>          return true;
>      }
>  
> +    private boolean isSearchUrl(String text) {

Maybe trim the string before doing the indexOf() calls?

@@ +213,5 @@
> +        int colon = text.indexOf(':');
> +        int dot = text.indexOf('.');
> +        int space = text.indexOf(' ');
> +        
> +        boolean spacedOut = space > -1 && (space < colon || space < dot);

What's "(space < colon || space < dot)" checking for exactly? It would be good add a comment with example inputs and the expect outcomes here to make it clear what this methods is doing.

@@ +248,5 @@
>      private void openUrlAndFinish(String url) {
> +        if (isSearchUrl(mText.getText().toString())) {
> +            openSearchAndFinish(mText.getText().toString(), "__default__");
> +            return;
> +        }

I'd prefer to have a method like submitAndFinish() that has something like:

if (isSearchUrl(...))
    openSearchAndFinish(...)
else
    openUrlAndFinish(...)

And use it everywhere openUrlAndFinish() is being used now.

::: mobile/android/chrome/content/browser.js
@@ +546,5 @@
> +      else
> +        engine = Services.search.getEngineByName(args.engine);
> +
> +      if (engine)
> +        uri = engine.getSubmission(args.url).uri;

Don't you need to fixup the args.url here if engine is null? Otherwise you'll use args.url with no fixup as a result.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: updated patch 2

This patch makes the suggested changes, except the URI fixup if no search engine was found. I'm not sure we want to do that. If we do, a new bug can be filed.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: diff of updated patch 2 and patch 1

A diff between patches to help see the changes

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 581256 [details] [diff] [review]
updated patch 2

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

Looks good.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/mozilla-central/rev/aa0bb5bb8931

Comment 34

[Andreea Pod]

Testcase added in test run for Firefox 11 and 12:
- https://litmus.mozilla.org/show_test.cgi?id=44847
- https://litmus.mozilla.org/show_test.cgi?id=44848

Comment 35

[Carla Nadastean]

Verified with:
12.0a1 (2012-01-12) Device: Samsung Nexus S Android(2.3.6)
11.0a2 (2012-01-12) Device: Samsung Nexus S Android(2.3.6)

Search icon in displyed in url field for random string entered. If the string is an url then "go" icon (->) is displayed.