Closed
Bug 655584
Opened 13 years ago
Closed 13 years ago
awesomebar suggestions don't update after tab is closed
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 7
People
(Reporter: test_test_testing, Assigned: bbondy)
Details
Attachments
(1 file, 5 obsolete files)
3.04 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
yeah, seeing it with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110507 Firefox/6.0a1
URL: N/A
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Whiteboard: dupeme
Version: unspecified → Trunk
Updated•13 years ago
|
Assignee: nobody → netzen
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Closing the popup after the tab close makes sense to me.
Assignee | ||
Comment 4•13 years ago
|
||
Thanks, I'll create a patch that way for review.
Assignee | ||
Comment 5•13 years ago
|
||
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
Attachment #532476 -
Flags: review?(gavin.sharp)
Comment 6•13 years ago
|
||
(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?
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
Thanks for the info Gavin, I'll try your suggestion and figure out when the regression was introduced.
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
Seems logical to me. I'll give that a try instead and post a patch when done.
Comment 13•13 years ago
|
||
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.
Assignee | ||
Comment 14•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: dupeme
Assignee | ||
Comment 15•13 years ago
|
||
This fix works better than the last as it fixes both tab closes and tab switches, whereas the old fix only handled tab closing.
Attachment #532476 -
Attachment is obsolete: true
Attachment #532476 -
Flags: review?(gavin.sharp)
Attachment #532853 -
Flags: review?(gavin.sharp)
Comment 16•13 years ago
|
||
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).
Attachment #532853 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
This patch works for tab switching, tab closing, and down button works. Tried full screen and not full screen as well.
Attachment #532853 -
Attachment is obsolete: true
Attachment #533042 -
Flags: review?(gavin.sharp)
Comment 19•13 years ago
|
||
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.
Attachment #533042 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 20•13 years ago
|
||
Will do about the comment. I'll let you know if I have any questions about the test case. Thanks for the guidance.
Assignee | ||
Comment 21•13 years ago
|
||
- 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
Attachment #533042 -
Attachment is obsolete: true
Attachment #533163 -
Flags: review?(gavin.sharp)
Comment 22•13 years ago
|
||
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.
Assignee | ||
Comment 23•13 years ago
|
||
test was working for me but I'll retry it and if I can't reproduce I'll try on mac.
Assignee | ||
Comment 24•13 years ago
|
||
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.
Attachment #533163 -
Attachment is obsolete: true
Attachment #533163 -
Flags: review?(gavin.sharp)
Attachment #533871 -
Flags: review?(gavin.sharp)
Comment 25•13 years ago
|
||
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+
Attachment #533871 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 26•13 years ago
|
||
- Added a new test file and put it in Makefile.in - Fixed whitespace and line length problems
Attachment #533871 -
Attachment is obsolete: true
Assignee | ||
Comment 27•13 years ago
|
||
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]: -----------------------------------------------------------------
Attachment #534093 -
Flags: review?(gavin.sharp)
Comment 28•13 years ago
|
||
Comment on attachment 534093 [details] [diff] [review] Close popup when switching to tab that previously had urlbar focused perfect! thank you.
Attachment #534093 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Comment 29•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2667493a7c91
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Comment 30•13 years ago
|
||
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.
Assignee | ||
Comment 31•13 years ago
|
||
Hi Vlad, could you try a nightly from 2011-05-26?
Assignee | ||
Comment 32•13 years ago
|
||
Works for me in the Nightly build 2011-06-01.
Comment 33•13 years ago
|
||
Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110527 Firefox/7.0a1
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•