Last Comment Bug 655584 - awesomebar suggestions don't update after tab is closed
: awesomebar suggestions don't update after tab is closed
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 7
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-08 06:48 PDT by test_test_testing
Modified: 2011-06-02 04:22 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Close auto suggset popup on urlbar when tab is closing (1.06 KB, patch)
2011-05-14 16:13 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Close popup if text is changed programmatically (1.02 KB, patch)
2011-05-16 20:05 PDT, Brian R. Bondy [:bbondy]
gavin.sharp: review-
Details | Diff | Splinter Review
Close popup on updateCurrentBrowser (1.06 KB, patch)
2011-05-17 12:50 PDT, Brian R. Bondy [:bbondy]
gavin.sharp: review+
Details | Diff | Splinter Review
Explicitly close the popup if the URL bar retains focus (2.65 KB, patch)
2011-05-17 20:54 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
If urlbar was focused on tab switching to, close popup (2.76 KB, patch)
2011-05-19 18:51 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Close popup when switching to tab that previously had urlbar focused (3.04 KB, patch)
2011-05-20 13:47 PDT, Brian R. Bondy [:bbondy]
gavin.sharp: review+
Details | Diff | Splinter Review

Description test_test_testing 2011-05-08 06:48:33 PDT
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110506 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110506 Firefox/6.0a1

The list of suggestions presented by the awesomebar does not update after I close a tab with the Ctrl-W shortcut. (see STR for details)

Reproducible: Always

Steps to Reproduce:
1. Open a blank tab.
2. Type something generic in the URL bar (e.g. 'a'). The suggestions list for 'a' pops up.
3. Open another blank tab (Ctrl-T).
4. Type something else into the URL bar (e.g. 'b'). The suggestions list for 'b' pops up.
5. Close the tab WITH THE CTRL-W SHORTCUT. The 'a' tab is now active.
6. Although 'a' is now in the URL bar, the awesomebar still suggests sites for 'b'.

Actual Results:  
Suggestions for previously closed tab remain displayed.

Expected Results:  
Suggestions are updated for newly visible tab.

Reproduceable on this machine on Firefox 4.0 and nightly (06-May-2010).
Comment 1 Tyler Downer [:Tyler] 2011-05-08 07:31:29 PDT
yeah, seeing it with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110507 Firefox/6.0a1
Comment 2 Brian R. Bondy [:bbondy] 2011-05-10 21:31:12 PDT
I think the Expected results should be:
Suggestions for the closed tab should not be visible.

This is because the old auto suggest was already closed, and we do not currently maintain a stack of such information on if the auto suggest was open or not at tab creation time.  In Firefox 3.* by following the same steps the auto suggest would not show up again on tab 1 after the 2nd tab is closed.  I think we should work in the same way as FF3.*.  

When a new tab is created we are actually currently closing the auto suggest.  You can verify this by typing in some text so auto suggest comes up, then creating a tab (Ctrl+T), then going back to the previous tab (CTRL+TAB)
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-13 11:41:32 PDT
Closing the popup after the tab close makes sense to me.
Comment 4 Brian R. Bondy [:bbondy] 2011-05-13 11:42:36 PDT
Thanks, I'll create a patch that way for review.
Comment 5 Brian R. Bondy [:bbondy] 2011-05-14 16:13:45 PDT
Created attachment 532476 [details] [diff] [review]
Close auto suggset popup on urlbar when tab is closing

The newer tab's auto complete should not be present on the previous tab if the user pressed Ctrl+W to close the tab

This matches the expected results as posted in Comment #2
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-14 16:40:15 PDT
(In reply to comment #2)
> In Firefox 3.* by following the same
> steps the auto suggest would not show up again on tab 1 after the 2nd tab is
> closed.  I think we should work in the same way as FF3.*.

Did you investigate why our behavior changed?
Comment 7 Brian R. Bondy [:bbondy] 2011-05-14 17:12:20 PDT
I didn't see any related code changed directly that would cause the bug.

I looked back in the history 12 months for tabbrowser.xml but didn't see a reason why.  Perhaps it could be even older than that?
It could be some kind of focus related fix somewhere else as the autocomplete closes when focus goes away, maybe related to all the tabcandy changes.  For example if you click the tab x with your mouse that didn't reproduce the regression.

It seems to work like the 3.x series now though with the attached patch.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-16 13:55:16 PDT
It's probably more likely to be something in autocomplete.xml or the underlying panel code that caused the behavior change. I'd just find a regression range using nightlies and the pushlog.
Comment 9 Brian R. Bondy [:bbondy] 2011-05-16 14:55:40 PDT
Thanks for the info Gavin, I'll try your suggestion and figure out when the regression was introduced.
Comment 10 Brian R. Bondy [:bbondy] 2011-05-16 16:51:30 PDT
Thanks for the suggestions Gavin, I learned how to track down regressions better now :)

I narrowed down via manual binary search on the nightly builds that:
 
Works: 2010-11-08 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-08-04-mozilla-central/firefox-4.0b8pre.en-US.win32.installer.exe

Broken: 2010-11-09
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-09-07-mozilla-central/firefox-4.0b8pre.en-US.win32.installer.exe

I then went through the push log and reviewed the commits in that range.
The regression seems to have been introduced on 2010-11-09:

Bug 565575 - Retain location bar focus when switching tabs
https://bugzilla.mozilla.org/show_bug.cgi?id=565575
http://hg.mozilla.org/mozilla-central/rev/dda010000be1

I think the attached patch still makes sense in order to not affect Bug 565575, but please let me know how to proceed.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-16 18:01:49 PDT
Ah right, that makes sense - the focus switch is what would have caused the panel to be dismissed. I wonder whether it would make more sense to have the panel be dismissed any time the URL bar value changes programmatically (i.e. from the value setter) rather than specifically on tab switch.
Comment 12 Brian R. Bondy [:bbondy] 2011-05-16 18:12:04 PDT
Seems logical to me.  I'll give that a try instead and post a patch when done.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-16 18:20:21 PDT
I guess the other option is to actually update the results popup as though the text was entered by the user. That would probably have to be a lower-level adjustment to the autocomplete widget itself, though, and I'm not sure offhand whether that would break people.
Comment 14 Brian R. Bondy [:bbondy] 2011-05-16 18:27:33 PDT
Your suggestion in Comment 11 I think is best for this reason:

1) Open a tab and navigate to slashdot.org (auto complete is not open)
2) Open another tab and type in a letter or two so autocomplete comes up
3) Ctrl+Tab to go to the previous tab
4) autocomplete would be displayed still but with updated content, but shouldn't be shown.  We would have to store per tab info of if autocomplete is open or not.
Comment 15 Brian R. Bondy [:bbondy] 2011-05-16 20:05:53 PDT
Created attachment 532853 [details] [diff] [review]
Close popup if text is changed programmatically

This fix works better than the last as it fixes both tab closes and tab switches, whereas the old fix only handled tab closing.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-17 11:11:37 PDT
Comment on attachment 532853 [details] [diff] [review]
Close popup if text is changed programmatically

Sorry, I think I misled you - this breaks navigating through the popup using the arrow keys, since the value setter is called when you select an entry. I think we should revert to a more isolated approach of closing the popup on tab switch (e.g. from updateCurrentBrowser in tabbrowser.xml).
Comment 17 Brian R. Bondy [:bbondy] 2011-05-17 11:19:10 PDT
No problem, I'll submit a new patch with your suggestion.  I was thinking about doing a fix in updateCurrentBrowser as well but wanted to submit one in the setter for review.
Comment 18 Brian R. Bondy [:bbondy] 2011-05-17 12:50:28 PDT
Created attachment 533042 [details] [diff] [review]
Close popup on updateCurrentBrowser

This patch works for tab switching, tab closing, and down button works. Tried full screen and not full screen as well.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-17 13:28:23 PDT
Comment on attachment 533042 [details] [diff] [review]
Close popup on updateCurrentBrowser

Might be good to add a comment saying that we need to explicitly close the popup if the URL bar will potentially retain focus (in case we somehow add some other codepath where it could stay focused here in the future).

We should also have a test for this. A browser-chrome test should be easy to write - you could probably even just add a few lines to browser_tabfocus.js. Feel free to ping me on IRC if you need any advice.
Comment 20 Brian R. Bondy [:bbondy] 2011-05-17 13:30:22 PDT
Will do about the comment.
I'll let you know if I have any questions about the test case.

Thanks for the guidance.
Comment 21 Brian R. Bondy [:bbondy] 2011-05-17 20:54:20 PDT
Created attachment 533163 [details] [diff] [review]
Explicitly close the popup if the URL bar retains focus

- Added a comment describing why I am closing the popup
- Added a browser-chrome test into browser/base/content/test/browser_tabfocus.js to ensure across tab switches where urlbar is focused it will close the popup when open
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-18 21:31:52 PDT
Hmm, the test seems to be failing for me on Mac, with the patch applied, even though I confirmed that the bug as described in comment 0 is fixed.
Comment 23 Brian R. Bondy [:bbondy] 2011-05-18 21:35:19 PDT
test was working for me but I'll retry it and if I can't reproduce I'll try on mac.
Comment 24 Brian R. Bondy [:bbondy] 2011-05-19 18:51:30 PDT
Created attachment 533871 [details] [diff] [review]
If urlbar was focused on tab switching to, close popup

Found the reason why the test was failing.  I think it worked for me because I was testing out of objdir with a diff file.  I could reproduce the test fail consistently.

The reason the test was failing is that each tab saves if the urlbar was focused, so I needed to set the focus on the urlbar for the tab that will be switched to, not the tab that I was switching from.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-19 20:08:30 PDT
Comment on attachment 533871 [details] [diff] [review]
If urlbar was focused on tab switching to, close popup

a couple of things:
- nit: s/we need to// in the comment, and add a space after "//"
- I think I changed my mind about putting this in browser_tabfocus. putting it in its own test file is cleaner (just copy a simple test like browser_bug598923.js, and add it to the Makefile.in)

otherwise this looks great. I'll wait for an updated patch to r+
Comment 26 Brian R. Bondy [:bbondy] 2011-05-20 13:47:24 PDT
Created attachment 534093 [details] [diff] [review]
Close popup when switching to tab that previously had urlbar focused

- Added a new test file and put it in Makefile.in
- Fixed whitespace and line length problems
Comment 27 Brian R. Bondy [:bbondy] 2011-05-20 13:48:43 PDT
Comment on attachment 534093 [details] [diff] [review]
Close popup when switching to tab that previously had urlbar focused

Review of attachment 534093 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-20 13:50:09 PDT
Comment on attachment 534093 [details] [diff] [review]
Close popup when switching to tab that previously had urlbar focused

perfect! thank you.
Comment 29 Dão Gottwald [:dao] 2011-05-25 02:55:11 PDT
http://hg.mozilla.org/mozilla-central/rev/2667493a7c91
Comment 30 Vlad [QA] 2011-05-26 02:44:44 PDT
The problem is still reproducible for me on Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

If I close the second tab with with ctrl+w, the search for the letter "b" is present in the previous tab. (the letter "a" search tab) and the dropdown menu is present. If I press the "Esc" key, to get rid of the dropdown menu, I end up with "b" search instead of "a" search.
Comment 31 Brian R. Bondy [:bbondy] 2011-05-26 03:40:13 PDT
Hi Vlad, could you try a nightly from 2011-05-26?
Comment 32 Brian R. Bondy [:bbondy] 2011-06-01 13:45:07 PDT
Works for me in the Nightly build 2011-06-01.
Comment 33 Vlad [QA] 2011-06-02 04:22:15 PDT
Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110527 Firefox/7.0a1

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