Closed Bug 1370518 Opened 7 years ago Closed 7 years ago

The search suggestions hint is displayed on focusing the Awesome bar only the first time

Categories

(Firefox :: Address Bar, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- verified

People

(Reporter: sbadau, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

[Affected versions]: - Nightly 55.0a1 [Affected platforms]: - All [Steps to reproduce]: 1. Open Firefox 2. Ctrl+L to focus the Awesome Bar 3. Click on the content to make the Awesome Bar lose focus 4. Ctrl+T to open a new tab [Expected result]: - In steps 2 and 4, the search suggestions hint should be displayed [Actual result]: - In step 4, the search suggestions hint is not displayed (even if the Awesome bar is focused) [Regression range]: - https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94906c37940c6b1c371dc7c22ed2098face96d8b&tochange=7fb3d9dfa8e684d5258a3d96b7176a1fa95fe205
The problem is that we get the "focus" event BEFORE the "TabSelect" event, so apparently we focus the locationbar before switching the tab. It should be the opposite. The focus happens in _adjustFocusAfterTabSwitch, but it doesn't seem to really happen "after". Mike, any insights?
Blocks: 1362866
Component: Search → Location Bar
Flags: needinfo?(mconley)
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Please be aware that the issue is also reproducible if after opening Firefox with a new profile, a new tab is opened. Steps to reproduce: 1. Launch Firefox with a new profile 2. Open a new tab Expected results: In step 2 - the search suggestions hint should be displayed (as an animation). Actual results: In step 2 - the search suggestions hint is not displayed.
Blocks: 1368470
I confirmed that delaying the "focus" event handler with a setTimeout(0) fixes things, so it's really just the order of events.
This should have an assignee, so taking it. Discussing with Mike we decided to investigate different solutions in parallel, I will look into possibly replacing the current detach/attach approach with a new resetInternalState API in the autocomplete controller, while Mike will investigate what we can do in tabbrowser to either restore the events as before, or provide a way to listen for a BeforeTabSelect (or something like that).
Assignee: nobody → mak77
Status: NEW → ASSIGNED
I'm testing a patch on Try for the resetInternalState implementation. From manual testing it works well, but it needs to pass Try and I should write a test for showing the onboarding notification when we open a new tab.
Summary: The search suggestions is displayed on focusing the Awesome bar only the first time → The search suggestions hint is displayed on focusing the Awesome bar only the first time
Flags: needinfo?(mconley)
Comment on attachment 8875218 [details] Bug 1370518 - Don't completely detach/attach the autocomplete controller on TabSelect. https://reviewboard.mozilla.org/r/146624/#review151846 Thanks for fixing this mak. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:207 (Diff revision 2) > +{ > + // Clear out the current search context > + if (mInput) { > + nsAutoString value; > + mInput->GetTextValue(value); > + nsCOMPtr<nsIAutoCompleteInput> input(mInput); I don't think `input` is being used. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:209 (Diff revision 2) > + StopSearch(); > + ClearResults(); Should we be sending the return values of these to Unused as well? ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:217 (Diff revision 2) > + } > + mPlaceholderCompletionString.Truncate(); > + mDefaultIndexCompleted = false; > + mProhibitAutoFill = false; > + mSearchStatus = nsIAutoCompleteController::STATUS_NONE; > + mCompletedSelectionIndex = -1; I get that ClearResults(); will set mRowCount to 0 if we had an input... should we worry about cases where there is no input element, but the row count hasn't been altered? Would it make more sense to unilaterally set mRowCount to 0 here, or is there no point? Same goes for mSearchesOngoing. I only ask because we were clearing them before, and we're skipping them now.
Attachment #8875218 - Flags: review?(mconley) → review+
Comment on attachment 8875218 [details] Bug 1370518 - Don't completely detach/attach the autocomplete controller on TabSelect. https://reviewboard.mozilla.org/r/146624/#review151846 > I get that ClearResults(); will set mRowCount to 0 if we had an input... should we worry about cases where there is no input element, but the row count hasn't been altered? Would it make more sense to unilaterally set mRowCount to 0 here, or is there no point? > > Same goes for mSearchesOngoing. I only ask because we were clearing them before, and we're skipping them now. mSearchesOngoing matters only when a search is running, for that to happen we must have an input. I'm not 100% sure about mRowCount, it's unlikely that it will matter since if there's no input we are detached from the controller, but for safety I'll clear it always as it was before.
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/650ad20e9fea Don't completely detach/attach the autocomplete controller on TabSelect. r=mconley
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170612030208) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: