Awesome-bar URL entry including search

VERIFIED FIXED in fennec1.0m5

Status

Fennec Graveyard
General
P1
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Christian Sejersen, Assigned: Neil Deakin (away until Feb 4))

Tracking

Trunk
fennec1.0m5
Other
Maemo
Bug Flags:
blocking-xul-fennec1.0 +

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

10 years ago
 
(Reporter)

Updated

10 years ago
Assignee: nobody → enndeakin
Priority: -- → P1
Target Milestone: --- → Fennec M5

Updated

10 years ago
Flags: blocking-fennec1.0+
(Assignee)

Comment 1

10 years ago
Created attachment 325414 [details] [diff] [review]
basic autocomplete and find implementation
(Assignee)

Comment 2

10 years ago
Created attachment 325415 [details] [diff] [review]
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.
The find implementation in attachment 325414 [details] [diff] [review] will probably "fix" bug 436074
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?
(Assignee)

Comment 5

10 years ago
The second patch isn't mobile specific. Once we have the right desired UI here, we can do more.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

10 years ago
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.
Created attachment 326394 [details] [diff] [review]
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.
Attachment #326394 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

10 years ago
Created attachment 327113 [details] [diff] [review]
base changes updated
Attachment #325415 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 327114 [details] [diff] [review]
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.
Attachment #325414 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
OS: All → Linux (embedded)

Comment 10

10 years ago
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?
Axel - Looks like we are thinking about moving the search service to the toolkit (bug 444735), which would keep us from needing a fork.
Attachment #326394 - Flags: review?(gavin.sharp)
Comment on attachment 326394 [details] [diff] [review]
Add "components" folder to Fennec

bug 437949 has enough of this patch to make this obsolete
Attachment #326394 - Attachment is obsolete: true
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.
Attachment #327114 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

10 years ago
Created attachment 329667 [details] [diff] [review]
updated patch
Attachment #327114 - Attachment is obsolete: true
Attachment #329667 - Flags: review?(gavin.sharp)
Attachment #327114 - Flags: review?(gavin.sharp)
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).
Attachment #327113 - Flags: review+
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?
Attachment #329667 - Flags: review?(gavin.sharp) → review+

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
> 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.

(Assignee)

Comment 19

10 years ago
> >+
> >+#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.
(Assignee)

Comment 20

10 years ago
> >+    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.
(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.

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

Remove the try/catch, I meant.
(Assignee)

Comment 23

10 years ago
(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.
(Assignee)

Comment 24

10 years ago
Created attachment 329834 [details] [diff] [review]
style the autocomplete better 

- makes the popup the size of the browser
- address other review comments
Attachment #327113 - Attachment is obsolete: true
Attachment #329667 - Attachment is obsolete: true
Attachment #329834 - Flags: review?(gavin.sharp)
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.
Attachment #329834 - Flags: review?(gavin.sharp) → review+
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

10 years ago
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.
Thanks, dbaron. I think Mardak's right, I will investigate further tomorrow.
Created attachment 329973 [details] [diff] [review]
fixed patch

Passes mochi/mochichrome/make check.
Attachment #329973 - Flags: review?(enndeakin)
Attachment #329973 - Attachment is patch: true
Attachment #329973 - Attachment mime type: application/octet-stream → text/plain
Attachment #329973 - Attachment is obsolete: true
Attachment #329973 - Flags: review?(enndeakin)

Comment 31

10 years ago
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.
Created attachment 329980 [details] [diff] [review]
fixed fixed patch

Didn't notice that the previous patch failed to compile - this one does and still passes mochi/mochichrome/make check!
Attachment #329980 - Flags: review?(enndeakin)
(Assignee)

Comment 33

10 years ago
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.
Attachment #329980 - Flags: review?(enndeakin) → review+
Created attachment 329986 [details] [diff] [review]
fixed fixed fixed patch
Attachment #329980 - Attachment is obsolete: true
Pushed that patch to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/860fd53ec15c

That makes this FIXED, I think!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
this has been in for a while and verified with beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.