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

VERIFIED FIXED in Firefox 55

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: simona.marcu, Assigned: mak)

Tracking

({regression})

55 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

[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
Assignee

Comment 1

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

Updated

2 years ago
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.
Assignee

Updated

2 years ago
Blocks: 1368470
Assignee

Comment 3

2 years ago
I confirmed that delaying the "focus" event handler with a setTimeout(0) fixes things, so it's really just the order of events.
Assignee

Comment 4

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

Comment 5

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(mconley)

Comment 8

2 years ago
mozreview-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

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+
Assignee

Comment 9

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 11

2 years ago
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
https://hg.mozilla.org/mozilla-central/rev/650ad20e9fea
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.