Last Comment Bug 599048 - Search results lose focus and search is closed on switch back to window with search results (window tab matching)
: Search results lose focus and search is closed on switch back to window with ...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P3 normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-23 11:32 PDT by Aaron Train [:aaronmt]
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (6.36 KB, patch)
2011-08-11 02:12 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (4.89 KB, patch)
2011-08-15 09:41 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Splinter Review
v3 (4.94 KB, patch)
2011-09-06 05:19 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (5.20 KB, patch)
2011-10-10 01:10 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2010-09-23 11:32:58 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre

Presently, if one were to tab match to a tab in a separate window, and switch back to the primary window the results, results are lost as the window takes focus again.

Panorama search should retain focus with results on focus of the browser window

STR:
1. Open a browser window, open panorama, 'about:home' should be visible 
2. Type 'Mine'
3. Open a new window, cmd/ctrl-n -- see tab matches from new window at bottom "Minefield"
4. Click to focus the new window
5. Click to focus the window with the search results

AR: Search results are lost as window regains focus
Comment 1 Aaron Train [:aaronmt] 2010-09-23 11:37:48 PDT
I suppose the search results should retain on focus so a user whom perhaps clicked a wrong tab-match will not have to query again, especially if the search query is long and convoluted
Comment 2 Aza Raskin [:aza] 2010-10-12 11:58:00 PDT
Aaron, how often do you find yourself encountering this case? My inclination is to push this off (just because of timing) to post Firefox 4 land, especially as it appears that simply switching between windows doesn't cause this bug to activate.

If you feel strongly, let me know.
Comment 3 Aaron Train [:aaronmt] 2010-10-12 12:03:48 PDT
(In reply to comment #2)
> Aaron, how often do you find yourself encountering this case? My inclination is
> to push this off (just because of timing) to post Firefox 4 land, especially as
> it appears that simply switching between windows doesn't cause this bug to
> activate.
> 
> If you feel strongly, let me know.

Not something I feel strongly about, no worries. Hardly something I've been encountering, still worthy of investigating in the future imo.
Comment 4 Ian Gilman [:iangilman] 2011-03-03 16:06:25 PST
bugspam: returning Sean's bugs to the pool
Comment 5 Raymond Lee [:raymondlee] 2011-08-11 02:12:55 PDT
Created attachment 552327 [details] [diff] [review]
v1
Comment 6 Raymond Lee [:raymondlee] 2011-08-11 19:03:15 PDT
Passed try with random orange.
http://tbpl.mozilla.org/?tree=Try&rev=29a6d94acec5
http://tbpl.mozilla.org/?tree=Try&rev=20b07a5e3ef7
Comment 7 Tim Taubert [:ttaubert] 2011-08-15 01:38:36 PDT
Comment on attachment 552327 [details] [diff] [review]
v1

Review of attachment 552327 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

The one problem with current approach is that we're listening for "blur" events. If I switch to the inactive Panorama window via Alt-Tab I need to double-click for the search layer to fade away. Could you please integrate that in your test? I think you'd just need to call inactiveWindow.focus(), waitForFocus() and ensure that the search shade fades away after a single click.

What we should do is listen for the focus/activate event, only. Unfortunately these are fired before the click event itself and there is no way (I tried several, please correct me if I'm wrong) to determine whether we just received a click that additionally focused the inactive window. So what we could do is block the search shade for some milliseconds (50-100?) after we received the activate/focus event. So we don't fade away the search shade when clicking to activate a window and we allow to leave search mode with a single click after using alt-tab.
Comment 8 Raymond Lee [:raymondlee] 2011-08-15 09:41:42 PDT
Created attachment 553201 [details] [diff] [review]
v2

Switched to listen for focus event and block the search shade form some 200 milliseconds (shorter time wouldn't work).

Added test to call inactiveWindow.focus() and then ensure that the search shade fades away after a single click.  Tried to use waitForFocus() but it requires me to add setTimeout since we block the search shade for some milliseconds, therefore, I added code to fire "tabviewsearchclickunblocked" event when the search is unblocked so the test can detect that.
Comment 9 Tim Taubert [:ttaubert] 2011-09-06 01:48:10 PDT
Comment on attachment 553201 [details] [diff] [review]
v2

Review of attachment 553201 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I know that was my own suggestion but I don't think we can implement it that way. If 50ms are enough on my system but your system needs 200ms (I think it also depends on the OS and window manager) then that seems very flaky to me. We would implement an unstable fix and a probably intermittent test failure. I have no specific idea but what we'd need is a more underlying fix to determine whether that click we just got also focused the window.
Comment 10 Raymond Lee [:raymondlee] 2011-09-06 03:37:22 PDT
(In reply to Tim Taubert [:ttaubert] from comment #9)
> Comment on attachment 553201 [details] [diff] [review]
> v2
> 
> Review of attachment 553201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I know that was my own suggestion but I don't think we can implement
> it that way. If 50ms are enough on my system but your system needs 200ms (I
> think it also depends on the OS and window manager) then that seems very
> flaky to me. We would implement an unstable fix and a probably intermittent
> test failure. I have no specific idea but what we'd need is a more
> underlying fix to determine whether that click we just got also focused the
> window.

Looked into it again.  The click event is fired after user left mouse button presses and then releases the button. The timeout varies because we are detecting "click" event on the searchshade, and different user releases the mouse button for a click in different speed.  

We can change to listen to the "mousedown" so when user clicks on an unfocused window, the window "focus" event would be fired and then follow immediately by "mousedown" event.  This way we don't have to wait for arbitrary timeout.

What do you think?
Comment 11 Raymond Lee [:raymondlee] 2011-09-06 05:19:10 PDT
Created attachment 558451 [details] [diff] [review]
v3

Here is the patch which I suggested in comment 10
Comment 12 Tim Taubert [:ttaubert] 2011-09-08 00:12:23 PDT
(In reply to Raymond Lee [:raymondlee] from comment #10)
> Looked into it again.  The click event is fired after user left mouse button
> presses and then releases the button. The timeout varies because we are
> detecting "click" event on the searchshade, and different user releases the
> mouse button for a click in different speed.  
> 
> We can change to listen to the "mousedown" so when user clicks on an
> unfocused window, the window "focus" event would be fired and then follow
> immediately by "mousedown" event.  This way we don't have to wait for
> arbitrary timeout.

I really like that idea and tried the patch but unfortunately it doesn't work for me on Linux (although the test passes). When I have two browser windows and click the inactive one then I have the mousedown event dispatched even after the setTimeout() fires. If I open the error console as well everything works fine... No idea what's going on. Did you have a chance to try your patch on Windows and Mac? Maybe it's just my window manager that does some weird stuff. (And if we can't fix it on Linux we should land it anyway.)
Comment 13 Raymond Lee [:raymondlee] 2011-09-08 02:50:36 PDT
> I really like that idea and tried the patch but unfortunately it doesn't
> work for me on Linux (although the test passes). When I have two browser
> windows and click the inactive one then I have the mousedown event
> dispatched even after the setTimeout() fires. If I open the error console as
> well everything works fine... No idea what's going on. Did you have a chance
> to try your patch on Windows and Mac? Maybe it's just my window manager that
> does some weird stuff. (And if we can't fix it on Linux we should land it
> anyway.)

It works for me on Mac.  I've pushed it to try and will test them once they are built.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2cecace4085e
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/raymond@raysquare.com-2cecace4085e
Comment 14 Raymond Lee [:raymondlee] 2011-09-08 09:29:20 PDT
Passed Try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2cecace4085e

I have tested that on Mac, winXP and win 7.  They all work fine.

It might be the mousedown event doesn't fire immediately after the window is focused on Linux.  Lets me try it with a setTimeout with 10ms and see what happens on Linux
Comment 15 Raymond Lee [:raymondlee] 2011-09-08 23:21:13 PDT
I have checked on Linux.  The mousedown event is fired around 5-12ms on my machine.

Tim: is it OK to use setTimeout 50ms for that?  We are sure that the mousedown event would be fired soon after the focus event but there is a small delay on Linux.  Or we stick with setTimeout = 0 in v3 patch?
Comment 16 Tim Taubert [:ttaubert] 2011-10-09 13:03:50 PDT
Comment on attachment 558451 [details] [diff] [review]
v3

Review of attachment 558451 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Raymond Lee [:raymondlee] from comment #15)
> Tim: is it OK to use setTimeout 50ms for that?  We are sure that the
> mousedown event would be fired soon after the focus event but there is a
> small delay on Linux.  Or we stick with setTimeout = 0 in v3 patch?

I think we should stick with setTimeout(, 0). If we can't solve Linux window manager weirdness so we should at least fix this for Windows and Mac. Thanks!

r=me with the following:

::: browser/base/content/tabview/search.js
@@ +357,5 @@
> +          self._blockClick = false;
> +
> +          let newEvent = document.createEvent("Events");
> +          newEvent.initEvent("tabviewsearchclickunblocked", false, false);
> +          dispatchEvent(newEvent);

Do we really need this additional event?

::: browser/base/content/test/tabview/browser_tabview_bug599048.js
@@ +42,5 @@
> +  // focus inactive window
> +  window.focus();
> +
> +  cw.addEventListener("tabviewsearchclickunblocked", function clickUnblocked() {
> +    cw.removeEventListener("tabviewsearchclickunblocked", clickUnblocked, false);

I think the search layer would have been closed at this point without the patch, so we don't need this extra event.
Comment 17 Raymond Lee [:raymondlee] 2011-10-10 01:10:18 PDT
Created attachment 565877 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Comment on attachment 558451 [details] [diff] [review] [diff] [details] [review]
> v3
> 
> Review of attachment 558451 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Raymond Lee [:raymondlee] from comment #15)
> > Tim: is it OK to use setTimeout 50ms for that?  We are sure that the
> > mousedown event would be fired soon after the focus event but there is a
> > small delay on Linux.  Or we stick with setTimeout = 0 in v3 patch?
> 
> I think we should stick with setTimeout(, 0). If we can't solve Linux window
> manager weirdness so we should at least fix this for Windows and Mac. Thanks!
> 
> r=me with the following:
> 
> ::: browser/base/content/tabview/search.js
> @@ +357,5 @@
> > +          self._blockClick = false;
> > +
> > +          let newEvent = document.createEvent("Events");
> > +          newEvent.initEvent("tabviewsearchclickunblocked", false, false);
> > +          dispatchEvent(newEvent);
> 
> Do we really need this additional event?

Removed

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug599048.js
> @@ +42,5 @@
> > +  // focus inactive window
> > +  window.focus();
> > +
> > +  cw.addEventListener("tabviewsearchclickunblocked", function clickUnblocked() {
> > +    cw.removeEventListener("tabviewsearchclickunblocked", clickUnblocked, false);
> 
> I think the search layer would have been closed at this point without the
> patch, so we don't need this extra event.

Removed

Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=f6ae17c9e85a
Comment 18 Raymond Lee [:raymondlee] 2011-10-10 04:46:45 PDT
> Pushed to try and waiting for the results
> https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=f6ae17c9e85a

Passed Try!
Comment 20 Marco Bonardo [::mak] 2011-10-11 02:26:54 PDT
https://hg.mozilla.org/mozilla-central/rev/e0e9529bdc84

Note You need to log in before you can comment on or make changes to this bug.