626338 - Searchbox doesn't get the focus if it's activated using the search button

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Raymond Lee [:raymondlee]]

Steps to reproduce
1. Go the panorama view and press the search button
2. The searchbox is displayed

Actual:
The searchbox doesn't get the focus

Expected:
The searchbox should have the focus in it

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Luke Iliffe (Harlequin99)]

Hourly regression range:
works	2011-01-14 20110114051941 616a453859c6
broken	2011-01-14 20110114054626 c02435684f58

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=616a453859c6&tochange=c02435684f58

bug 610821

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 504391 [details] [diff] [review]
v1

>-    setTimeout(function dispatchTabViewSearchEnabledEvent() {
>+    let dispatchTabViewSearchEnabledEvent = function() {
>       let newEvent = document.createEvent("Events");
>       newEvent.initEvent("tabviewsearchenabled", false, false);
>       dispatchEvent(newEvent);
>-    }, 0);
>+    };
>+
>+    if (activatedByKeypress) {
>+      $searchbox[0].focus();
>+      setTimeout(function dispatchSearchEnabledEvent() { 
>+        dispatchTabViewSearchEnabledEvent(); 
>+      }, 0);
>+    } else {
>+      setTimeout(function setFocusAndDispatchSearchEnabledEvent() {
>+        $searchbox[0].focus();
>+        dispatchTabViewSearchEnabledEvent();
>+      }, 0);
>+    }

Perhaps you could add a comment here for why the different code paths?

>+                 browser_tabview_bug626338.js \

Seems like this test could just be rolled into browser_tabview_search.js?

Also might be good to verify that entering with a key also activates focus (if we're not already doing that somewhere).

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #3)
> Comment on attachment 504391 [details] [diff] [review]
> v1
> 
> >-    setTimeout(function dispatchTabViewSearchEnabledEvent() {
> >+    let dispatchTabViewSearchEnabledEvent = function() {
> >       let newEvent = document.createEvent("Events");
> >       newEvent.initEvent("tabviewsearchenabled", false, false);
> >       dispatchEvent(newEvent);
> >-    }, 0);
> >+    };
> >+
> >+    if (activatedByKeypress) {
> >+      $searchbox[0].focus();
> >+      setTimeout(function dispatchSearchEnabledEvent() { 
> >+        dispatchTabViewSearchEnabledEvent(); 
> >+      }, 0);
> >+    } else {
> >+      setTimeout(function setFocusAndDispatchSearchEnabledEvent() {
> >+        $searchbox[0].focus();
> >+        dispatchTabViewSearchEnabledEvent();
> >+      }, 0);
> >+    }
> 
> Perhaps you could add a comment here for why the different code paths?

Done.

> 
> >+                 browser_tabview_bug626338.js \
> 
> Seems like this test could just be rolled into browser_tabview_search.js?

Merged the test into browser_tabview_search.js

> 
> Also might be good to verify that entering with a key also activates focus (if
> we're not already doing that somewhere).

Added that in browser_tabview_bug595560.js

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 505635 [details] [diff] [review]
v2

>+    if (activatedByKeypress) {
>+      $searchbox[0].focus();
>+      setTimeout(function dispatchSearchEnabledEvent() { 
>+        dispatchTabViewSearchEnabledEvent(); 
>+      }, 0);
>+    } else {
>+      // marshal the focusing, otherwise it ends up with searchbox[0].focus gets
>+      // called before the search button gets the focus after being pressed.
>+      setTimeout(function setFocusAndDispatchSearchEnabledEvent() {
>+        $searchbox[0].focus();
>+        dispatchTabViewSearchEnabledEvent();
>+      }, 0);
>+    }

Thanks for adding the comment. What about the other code path? Why the setTimeout for the event? Can you add a comment for that? Just trying to make sure future generations know what we were up to. :)

Folding the test into those other tests looks great!

r+ with the additional comment

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v3

I've removed the setTimeout in other code path but it breaks one of the tests.  I've fixed that test now so request another review.

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Created attachment 505710 [details] [diff] [review]
> v3
> 
> I've removed the setTimeout in other code path but it breaks one of the tests. 
> I've fixed that test now so request another review.

Passed Try!

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 505710 [details] [diff] [review]
v3

(I'm doing some feedback to help Ian on reviews.)

>-function ensureSearchShown(){
>+function ensureSearchShown(activatedByKeypress) {

Please add a standard format comment above the function.

>-    setTimeout(function dispatchTabViewSearchEnabledEvent() {
>+    let dispatchTabViewSearchEnabledEvent = function() {

Please actually name the function, too, so that we can see it in debug stacks.

>       let searchBox = contentWindow.iQ("#searchbox");
>+
>+      ok(searchBox[0].focus, "The search box has focus");

element.focus is a DOM method... it will always be not false.

You need to check both document.hasFocus() and that document.activeElement == searchBox[0].

http://www.whatwg.org/specs/web-apps/current-work/#focus-management

>   let onSearchEnabled = function() {
>-    let searchBox = contentWindow.document.getElementById("searchbox");
>-    is(searchBox.value, number, "The seach box matches the number: " + number);
>-    contentWindow.hideSearch(null);
>+    // ensure the dom changes (textbox get focused with number entered) complete 
>+    // before doing a check.
>+    executeSoon(function() { 
>+      let searchBox = contentWindow.document.getElementById("searchbox");
>+      is(searchBox.value, number, "The seach box matches the number: " + number);
>+      contentWindow.hideSearch(null);
>+    });

This probably works, but why not bind a "change" event listener to the searchBox?

>diff --git a/browser/base/content/test/tabview/browser_tabview_search.js b/browser/base/content/test/tabview/browser_tabview_search.js
>+    let searchBox = contentWindow.document.getElementById("searchbox");
>+    ok(searchBox.focus, "The search box has focus");

Same here.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to comment #8)
> Comment on attachment 505710 [details] [diff] [review]
> v3
> 
> (I'm doing some feedback to help Ian on reviews.)
> 
> >-function ensureSearchShown(){
> >+function ensureSearchShown(activatedByKeypress) {
> 
> Please add a standard format comment above the function.
> 

Done

> >-    setTimeout(function dispatchTabViewSearchEnabledEvent() {
> >+    let dispatchTabViewSearchEnabledEvent = function() {
> 
> Please actually name the function, too, so that we can see it in debug stacks.

Done

> 
> >       let searchBox = contentWindow.iQ("#searchbox");
> >+
> >+      ok(searchBox[0].focus, "The search box has focus");
> 
> element.focus is a DOM method... it will always be not false.
> 
> You need to check both document.hasFocus() and that document.activeElement ==
> searchBox[0].
> 
> http://www.whatwg.org/specs/web-apps/current-work/#focus-management

Updated.  Thanks for the info.

> 
> >   let onSearchEnabled = function() {
> >-    let searchBox = contentWindow.document.getElementById("searchbox");
> >-    is(searchBox.value, number, "The seach box matches the number: " + number);
> >-    contentWindow.hideSearch(null);
> >+    // ensure the dom changes (textbox get focused with number entered) complete 
> >+    // before doing a check.
> >+    executeSoon(function() { 
> >+      let searchBox = contentWindow.document.getElementById("searchbox");
> >+      is(searchBox.value, number, "The seach box matches the number: " + number);
> >+      contentWindow.hideSearch(null);
> >+    });
> 
> This probably works, but why not bind a "change" event listener to the
> searchBox?
> 

I will keep it is since this test works fine.

> >diff --git a/browser/base/content/test/tabview/browser_tabview_search.js b/browser/base/content/test/tabview/browser_tabview_search.js
> >+    let searchBox = contentWindow.document.getElementById("searchbox");
> >+    ok(searchBox.focus, "The search box has focus");
> 
> Same here.

Updated.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 505855 [details] [diff] [review]
v4

Looks great!

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 505855 [details] [diff] [review]
v4

Lovely

Comment 12

[Raymond Lee [:raymondlee]]

Sent it to try and waiting for the result

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to comment #12)
> Sent it to try and waiting for the result

Passed Try!

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 505855 [details] [diff] [review]
v4

a=beltzner, thanks for the tests!

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c83167f1e7da

Comment 18

[Tracy Walker [:tracy]]

verified with nightly build of minefield.