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

VERIFIED FIXED in Firefox 4.0b11

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

({regression})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ux][polish][good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

9 years ago
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
Assignee

Updated

9 years ago
Assignee: nobody → raymond
Assignee

Comment 1

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #504391 - Flags: review?(ian)
Assignee

Updated

8 years ago
Blocks: 608028
Assignee

Updated

8 years ago
Keywords: regression
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 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).
Attachment #504391 - Flags: review?(ian) → review-
Blocks: 627096
No longer blocks: 608028
Whiteboard: [ux][polish][good first bug]
Assignee

Comment 4

8 years ago
Posted patch v2 (obsolete) — Splinter Review
(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
Attachment #504391 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #505635 - Flags: review?(ian)
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
Attachment #505635 - Flags: review?(ian) → review+
Assignee

Comment 6

8 years ago
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #505635 - Attachment is obsolete: true
Attachment #505710 - Flags: review?(ian)
Assignee

Comment 7

8 years ago
(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 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.
Attachment #505710 - Flags: review?(ian) → feedback-
Assignee

Comment 9

8 years ago
Posted patch v4 (obsolete) — Splinter Review
(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.
Attachment #505710 - Attachment is obsolete: true
Attachment #505855 - Flags: review?(ian)
Comment on attachment 505855 [details] [diff] [review]
v4

Lovely
Attachment #505855 - Flags: review?(ian) → review+
Assignee

Updated

8 years ago
Attachment #505855 - Flags: approval2.0?
Assignee

Comment 12

8 years ago
Sent it to try and waiting for the result
Duplicate of this bug: 628628
Assignee

Comment 14

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

Passed Try!
Comment on attachment 505855 [details] [diff] [review]
v4

a=beltzner, thanks for the tests!
Attachment #505855 - Flags: approval2.0? → approval2.0+
Assignee

Comment 16

8 years ago
Attachment #505855 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c83167f1e7da
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.