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.

Taking as I had bug 701329 anyway.

Ian and Madhava, I need assets to implement this.

(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?

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?

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.

(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 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.

(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.

Faster is better :)

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?

(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.

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

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

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

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

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.

(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.

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?

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

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.

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' ?"

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 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.

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.

A diff between patches to help see the changes

Comment on attachment 581256 [details] [diff] [review] updated patch 2 Review of attachment 581256 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

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

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.