426329 - Search box - pressing enter does nothing

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Seems that perhaps the checkin to 'fix' the right-click of the 'Search Glass' has broken search.  
Pressing Enter, or Alt Enter does nothing.

I see this in the Console2
Error: textBox is undefined
Source file: chrome://browser/content/search/search.xml
Line: 431

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040101 Minefield/3.0pre Firefox/3.0 ID:2008040101 <- Latest hourly

Caused by: https://bugzilla.mozilla.org/show_bug.cgi?id=424690 ?

Comment 2

[sevenfurnace]

Attachment: fix

sorry for mistake...

Comment 3

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

Comment on attachment 312900 [details] [diff] [review]
fix

Ugh, I should have caught this. Can you write a browser chrome test for this? Should be pretty easy, could just call BrowserSearch.searchBar.handleSearchCommand  with various fake events (or synthetic events) and check that the right thing happens. See http://developer.mozilla.org/en/docs/Browser_chrome_tests for more details, and feel free to ask if you have any questions.

Comment 5

[sevenfurnace]

(In reply to comment #3)
> Can you write a browser chrome test for this?

I don't know how to write a test in a right way, so It may take a lot of time.
It would be nice if someone could do this for me..

Comment 6

[Steve Lee]

The about:config option 'browser.search.openintab' is broken too.
Should this be a new bug or was it caused by the same checkin?

Comment 7

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

(In reply to comment #5)
> I don't know how to write a test in a right way, so It may take a lot of time.
> It would be nice if someone could do this for me..

Are you on IRC? I could walk you through it. It really isn't very hard, assuming you have a Firefox build.

Comment 8

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

I checked this in to fix the bustage, but it still needs a test.
(We'll probably respin to fix this and the crashes caused by bug 382392.)

mozilla/browser/components/search/content/search.xml 	1.123

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #6)
> The about:config option 'browser.search.openintab' is broken too.
> Should this be a new bug or was it caused by the same checkin?

If you're still seeing this after gavin's checkin, please file a new bug.

Comment 12

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040110 Minefield/3.0pre ID:2008040110

verified/fixed on win32

Comment 15

[sevenfurnace]

Attachment: mochitest

I'm not sure this is an appropriate test.

Comment 16

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040204 Minefield/3.0pre, Win XP nightly, and Linux Ubuntu nightly.

Comment 17

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

Comment on attachment 313032 [details] [diff] [review]
mochitest

This is excellent! Just a few minor nits...

>Index: browser/components/search/test/browser_426329.js

>+  var gBrowser = getBrowser();

No need for this, just use gBrowser directly.

>+  var searchBar = BrowserSearch.searchBar;
>+  var textBox = document.getAnonymousElementByAttribute(searchBar,
>+                "anonid", "searchbar-textbox");

>+  textBox.value = queryString;

Could just use searchBar._textbox, but you don't need it anyhow because searchBar.value should work fine (the search bar binding forwards the value property to the textbox).

>+  var searchButton = document.getAnonymousElementByAttribute(searchBar,
>+                     "anonid", "search-go-button");

Use searchBar.searchButton

>+  var queryString = "blah";
>+  var currentTab = gBrowser.selectedTab;
>+  var currentBrowser = gBrowser.getBrowserForTab(currentTab);

gBrowser.selectedBrowser

>+  function init() {
>+    for (var i = gBrowser.mTabs.length - 1; i >= 0; i--) {
>+      if (gBrowser.mTabs[i] != currentTab)
>+        gBrowser.removeTab(gBrowser.mTabs[i]);
>+    }

Is there a reason you need to clear other tabs? It's generally best if tests clean up after themselves (removing any tabs they create, etc.), so if there are any tests that don't do that we should probably fix them.

>+  function test2() {
>+    init();
>+    EventUtils.synthesizeKey("VK_RETURN", { altKey: true });
>+    gBrowser.addEventListener("DOMContentLoaded", function(event) {
>+      gBrowser.removeEventListener("DOMContentLoaded", arguments.callee, true);
>+      isnot(event.originalTarget, currentBrowser.contentDocument,
>+            "Alt+Return key loaded results in new tab");
>+      test3();
>+    }, true);
>+  }

Could you also check that the number of tabs is 1 greater than the previous number of tabs? Perhaps also add a comment about adding a test in the future for background/foreground tab opening (with/without Shift) - or just add one now. Same applies to test4().

>+  function simulateClick(aEvent, aTarget) {

You should be able to use EventUtils.sendMouseEvent here - http://lxr.mozilla.org/seamonkey/source/testing/mochitest/tests/SimpleTest/EventUtils.js is loaded into the "EventUtils" object for browser chrome tests.

Comment 18

[sevenfurnace]

Attachment: browser chrome test, v2

Gavin, thanks for your comment.

Comment 19

[sevenfurnace]

(In reply to comment #17)
> >+  var searchButton = document.getAnonymousElementByAttribute(searchBar,
> >+                     "anonid", "search-go-button");
> 
> Use searchBar.searchButton

But searchBar.searchButton refers to "searchbar-engine-button", not "search-go-button".

> Perhaps also add a comment about adding a test in the future
> for background/foreground tab opening (with/without Shift) - or just add one
> now. Same applies to test4().

I added tests for Shift key, but as for keyboard event, Shift key seems to do nothing
in the current implementation, so skipped a test for Shift+Alt+Return.
Is this ok?

> >+  function simulateClick(aEvent, aTarget) {
> 
> You should be able to use EventUtils.sendMouseEvent here -
> http://lxr.mozilla.org/seamonkey/source/testing/mochitest/tests/SimpleTest/EventUtils.js
> is loaded into the "EventUtils" object for browser chrome tests.

I didn't use EventUtils.sendMouseEvent because it expects target element's id, not element itself.

Comment 20

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

(In reply to comment #19)
> But searchBar.searchButton refers to "searchbar-engine-button", not
> "search-go-button".

Ah, right.

> I added tests for Shift key, but as for keyboard event, Shift key seems to do
> nothing
> in the current implementation, so skipped a test for Shift+Alt+Return.
> Is this ok?

Oh, no, I guess that is expected (though perhaps we should consider changing that).

> I didn't use EventUtils.sendMouseEvent because it expects target element's id,
> not element itself.

Ah, we should probably fix that too (make it except either an ID or an element).

Comment 21

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

Comment on attachment 313288 [details] [diff] [review]
browser chrome test, v2

Thanks again, this is great!

Comment 23

[Reed Loden [:reed]]

Checking in browser/components/search/test/Makefile.in;
/cvsroot/mozilla/browser/components/search/test/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/browser/components/search/test/browser_426329.js,v
done
Checking in browser/components/search/test/browser_426329.js;
/cvsroot/mozilla/browser/components/search/test/browser_426329.js,v  <--  browser_426329.js
initial revision: 1.1
done