431842 - Awesome-bar URL entry including search

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Christian Sejersen]

Comment 1

[Neil Deakin]

Attachment: basic autocomplete and find implementation

Comment 2

[Neil Deakin]

Attachment: change to toolkit/content/widgets/autocomplete.xml used to get search engines to appear across bottom of autocomplete

This implementation relies on the search service for displaying the items. Not sure how to include the search service in the build though.

Comment 3

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

The find implementation in attachment 325414 [details] [diff] [review] will probably "fix" bug 436074

Comment 4

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

the autocomplete.xml changes in attachment 325415 [details] [diff] [review] can be pushed into mozilla-central, right? They don't need to be mobile-specific?

Comment 5

[Neil Deakin]

The second patch isn't mobile specific. Once we have the right desired UI here, we can do more.

Comment 6

[Neil Deakin]

The find code here add the findbar in a popup window. The issues:

1. Focus isn't moved to the textbox
2. A panel-style findbar will cover any found text near the bottom of the page.

Comment 7

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

Attachment: Add "components" folder to Fennec

This patch adds makefiles to add a "components" folder to the application. I also added "search" components, but no UI, in case Neil needed them.

Comment 8

[Neil Deakin]

Attachment: base changes updated

Comment 9

[Neil Deakin]

Attachment: updated patch

- supports search buttons in the autocomplete
- autocomplete popup doesn't hide when there are no results

Issues:

- add search engines, but doesn't build them as I don't know how to do this.
- the browser/dirservice also needs to be included somehow.

Comment 10

[Axel Hecht [:Pike]]

We shouldn't copy over region.properties from firefox without UE review. I could imagine that doing webcal is different on mobile devices compared to the desktop, similar things for mailto. Not that I really knew with my crap phone. Not sure if now is the right time for this, but we should do that before we do l10n for real.

Is search.properties a copy? I wonder if all the strings in there would be actually used. I'm afraid they're as used as the corresponding code in the copied search components?

Comment 11

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

Axel - Looks like we are thinking about moving the search service to the toolkit (bug 444735), which would keep us from needing a fork.

Comment 12

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

Comment on attachment 326394 [details] [diff] [review]
Add "components" folder to Fennec

bug 437949 has enough of this patch to make this obsolete

Comment 13

[Johnathan Nightingale [:johnath]]

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

Neil says that this can land before the search service moves to toolkit, the search results just won't work.  Flagging for review.

Comment 14

[Neil Deakin]

Attachment: updated patch

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 327113 [details] [diff] [review]
base changes updated

>diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

>@@ -1289,16 +1289,23 @@ nsAutoCompleteController::PostSearchClea
> {  
>   NS_ENSURE_STATE(mInput);
>   nsCOMPtr<nsIAutoCompleteInput> input(mInput);
>   if (mRowCount) {
>     OpenPopup();
>     mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_MATCH;
>   } else {
>     mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
>+    PRUint32 minResults;
>+    mInput->GetMinResultsForPopup(&minResults);
>+    if (minResults == 0) {
>+      // check if the popup may be opened even when there are no results
>+      mIsOpen = PR_TRUE;
>+      return mInput->SetPopupOpen(PR_TRUE);
>+    }
>     ClosePopup();

Wouldn't it be clearer to just call OpenPopup() unconditionally? It already does the GetMinResultsForPopup check.

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>       <property name="minResultsForPopup"
>                 onset="this.setAttribute('minresultsforpopup', val); return val;"
>-                onget="return parseInt(this.getAttribute('minresultsforpopup')) || 0;"/>
>+                onget="var m = this.getAttribute('minresultsforpopup');
>+                       return m == '0' ? 0 : parseInt(m) || 1;"/>

I find "var m = parseInt(this.getAttribute('minresultsforpopup')); return isNaN(num) ? 1 : num;" easier to understand (and it's consistent with the older behavior for m==" 0", not that it really matters).

Comment 16

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/app/mobile.js b/app/mobile.js

>+pref("browser.search.searchEnginesURL",      "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/firefox/%VERSION%/search-engines/");

>+pref("browser.search.openintab", false);

These are actually only read by front end code in Firefox so we don't actually need them. The fact that there are both frontend and backend prefs in the "browser.search" branch is kind of unfortunate, because it means confusion when we move the backend to toolkit, but that's not the end of the world I guess.

>diff --git a/chrome/Makefile.in b/chrome/Makefile.in

>+SEARCH_PLUGINS = $(shell cat @srcdir@/locale/$(MOZ_UI_LOCALE)/searchplugins/list.txt)
>+
>+#libs:: $(addsuffix .xml,$(SEARCH_PLUGINS)) $(SYSINSTALL) $(IFLAGS1) $^ dist/bin/searchplugins
>+
>+#install:: $(addsuffix .xml,$(SEARCH_PLUGINS)) $(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(mozappdir)/searchplugins
>+
> include $(topsrcdir)/config/rules.mk

Why are these commented out?

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>  openDefaultHistory : function () {

>    if (!this._edit.value) {
>      document.getElementById("autocomplete_navbuttons").hidden = true;

Wouldn't hurt to keep a reference to this element since it's commonly referred to.

>+  updateSearchEngines : function () {

>+    try {
>+    var searchService = Cc["@mozilla.org/browser/search-service;1"].
>+                          getService(Ci.nsIBrowserSearchService);
>+    } catch (ex) {

Add a FIXME comment and file a bug to remove this once the search service is moved?

>+      case "input":
>+        if (this._edit.value) {
>+          this.updateSearchEngines();
>+          document.getElementById("autocomplete_navbuttons").hidden = false;
>+        }

Missing "break;" here (resulted in a strange bug caused by the fact that the keypress case compares aEvent.keyCode to aEvent.DOM_VK_*, which are both undefined for input events, which means that we hit both of those cases and end up loading everything you type as a URI, then quickly switching back to edit mode, which selects what you typed, which means the next event clobbers it, etc.)
  
>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+  doFind: function (aEvent) {
>+    var findbar = document.getElementById("findbar");
>+    if (Browser.findState == 0)
>+      findbar.onFindCommand();
>+    else
>+      findbar.onFindAgainCommand(Browser.findState == 2);

Would be nice to use named constants for findState, bit hard to follow otherwise.

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+        <textbox id="urlbar-edit" type="autocomplete" autocompletesearch="history" enablehistory="false"
>+                 maxrows="6" completeselectedindex="true" minresultsforpopup="0" flex="1" hidden="true"
>+                 autocompletepopup="popup_autocomplete"
>+                 onclick="BrowserUI.openDefaultHistory()"

>+                 ontextentered="BrowserUI.goToURI();"/>

This should eliminate the need for the DOM_VK_RETURN check in handleEvent's keypress case.

>diff --git a/chrome/locale/en-US/region.properties b/chrome/locale/en-US/region.properties
>diff --git a/chrome/locale/en-US/search.properties b/chrome/locale/en-US/search.properties

We don't really need most of the entries in these file - I guess you kept them to make it easier to keep the file in sync?

>diff --git a/chrome/skin/browser.css b/chrome/skin/browser.css

>+   list-style-image: url("chrome://browser/skin/images/page-starred.png");

>+   list-style-image: url("chrome://browser/skin/places/tag.png");

are you going to copy over these images from Firefox?

>+#findpanel findbar {

Could just use "findbar", right? Don't think we'll have any other findbars in our chrome!

Still need to figure out how to get this to look like the existing URL edit panel, though. Ideally we can change the panel's position/size/style and have it look just like the existing autocomplete-like UI?

Comment 17

[Axel Hecht [:Pike]]

Regarding region.properties, I think it's largely undecided on whether we're intending to keep that file in sync with Firefox or not.

One way to work around the decision process here is to just not make that thing localizable for now, i.e., to drop the references to region.properties in mobile.js and to hardcode the en-US values, with a follow-up bug to make that localizable.

Comment 18

[Neil Deakin]

> Still need to figure out how to get this to look like the existing URL edit
> panel, though. Ideally we can change the panel's position/size/style and have
> it look just like the existing autocomplete-like UI?

Well, my implementation uses the autocomplete popup for both autocomplete results and the search bar, and the person who implemented the existing 'url edit panel' seemed to think their panel would be used for both. Either one is ok but someone should decide which UI to use.

Comment 19

[Neil Deakin]

> >+
> >+#libs:: $(addsuffix .xml,$(SEARCH_PLUGINS)) $(SYSINSTALL) $(IFLAGS1) $^ dist/bin/searchplugins
> >+
> >+#install:: $(addsuffix .xml,$(SEARCH_PLUGINS)) $(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(mozappdir)/searchplugins
> >+
> > include $(topsrcdir)/config/rules.mk
> 
> Why are these commented out?

I want to build the search engines but couldn't get it to work. It isn't needed for this bug though.

Comment 20

[Neil Deakin]

> >+    try {
> >+    var searchService = Cc["@mozilla.org/browser/search-service;1"].
> >+                          getService(Ci.nsIBrowserSearchService);
> >+    } catch (ex) {
> 
> Add a FIXME comment and file a bug to remove this once the search service is
> moved?

Why? The search service would still be used.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> Well, my implementation uses the autocomplete popup for both autocomplete
> results and the search bar, and the person who implemented the existing 'url
> edit panel' seemed to think their panel would be used for both. Either one is
> ok but someone should decide which UI to use.

Yes, I think we should remove the url edit panel.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> Why? The search service would still be used.

Remove the try/catch, I meant.

Comment 23

[Neil Deakin]

(In reply to comment #15)
> (From update of attachment 327113 [details] [diff] [review])
> >diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete
> Wouldn't it be clearer to just call OpenPopup() unconditionally? It already
> does the GetMinResultsForPopup check.

If minResults is non-zero, it should fall through and call ClosePopup. Otherwise, it should not.

Comment 24

[Neil Deakin]

Attachment: style the autocomplete better

- makes the popup the size of the browser
- address other review comments

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 329834 [details] [diff] [review]
style the autocomplete better 

>diff --git a/chrome/Makefile.in b/chrome/Makefile.in

>+SEARCH_PLUGINS = $(shell cat @srcdir@/locale/$(MOZ_UI_LOCALE)/searchplugins/list.txt)

Might as well remove this for now and include it in whatever followup patch fixes building the search plugins.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+const FINDSTATE_FIND = 0;
>+const FINDSTATE_FIND_AGAIN = 1;
>+const FINDSTATE_FIND_PREVIOUS = 2;

>+  doFind: function (aEvent) {

>+    if (Browser.findState == this.FINDSTATE_FIND)
>+      findbar.onFindCommand();
>+    else
>+      findbar.onFindAgainCommand(Browser.findState == this.FINDSTATE_FIND_PREVIOUS);

Does this actually work? Wouldn't expect you to need the "this." before those find constants.

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>     <vbox id="urllist-container" flex="1" hidden="true">
>       <hbox id="urllist-items-container" flex="1">
>         <richlistbox id="urllist-items" flex="1"/>
>+     </hbox>
>+     <separator class="thin"/>
>+     <hbox id="urllist-search">
>+       <image id="tool-search"/>
>+     </hbox>

Messed up whitespace here?

Could wait to copy over the search plugins until the build parts are working and the search service is moved.

>diff --git a/chrome/skin/browser.css b/chrome/skin/browser.css

>-#tool-search {
>+.tool-search {

What's this about? Still have <image id="tool-search"/> above.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Pushed the mozilla-central part:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ca442067af90

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I backed out what gavin pushed because it caused test_417798.js to hang, turning the unit-test boxes orange:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/341f9199d4d3
http://hg.mozilla.org/mozilla-central/index.cgi/rev/233d692e9f75

Comment 28

[Ed Lee :Mardak]

Comment on attachment 329834 [details] [diff] [review]
style the autocomplete better 

>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
>@@ -1289,16 +1289,23 @@ nsAutoCompleteController::PostSearchClea
> {  
>   NS_ENSURE_STATE(mInput);
>   nsCOMPtr<nsIAutoCompleteInput> input(mInput);
>   if (mRowCount) {
>     OpenPopup();
>     mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_MATCH;
>   } else {
>     mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
>+    PRUint32 minResults;
>+    mInput->GetMinResultsForPopup(&minResults);
>+    if (minResults == 0) {
>+      // check if the popup may be opened even when there are no results
>+      mIsOpen = PR_TRUE;
>+      return mInput->SetPopupOpen(PR_TRUE);
>+    }
>     ClosePopup();
>   }
>   
>   // notify the input that the search is complete
>   input->OnSearchComplete();
I'm assuming the test is hanging because input->OnSearchComplete doesn't get called when hitting the added return statement.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Thanks, dbaron. I think Mardak's right, I will investigate further tomorrow.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Attachment: fixed patch

Passes mochi/mochichrome/make check.

Comment 31

[Ed Lee :Mardak]

Comment on attachment 329973 [details] [diff] [review]
fixed patch

>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
> nsAutoCompleteController::PostSearchCleanup()
> {  
>+  if (mRowCount || minResults == 0) {
>     OpenPopup();
>-    mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_MATCH;
>   } else {
>-    mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
>     ClosePopup();
>   }
>+  mSearchStatus = mRowCount ? nsIAutoCompleteController::STATUS_COMPLETE_MATCH
>+                              nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
Curious if the ordering of setting mSearchStatus before/after ClosePopup matters... I ran into some strangeness when writing my Show Keywords add-on relating to ClosePopup causing the input's popup to tell the controller to stop searching because it was closing. StopSearch will check for mSearchStatus before doing work.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Attachment: fixed fixed patch

Didn't notice that the previous patch failed to compile - this one does and still passes mochi/mochichrome/make check!

Comment 33

[Neil Deakin]

Comment on attachment 329980 [details] [diff] [review]
fixed fixed patch

Looks OK, but I think maintaining the order in which mSearchStatus is set is a good idea as well, to avoid any potential obscure regressions.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Attachment: fixed fixed fixed patch

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Pushed that patch to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/860fd53ec15c

That makes this FIXED, I think!

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

this has been in for a while and verified with beta3