Alice0775 White Reporter
	Description • 10 years ago

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Steps To Reproduce:
Steps To Reproduce:
1. Open about:home and search from search filed
2. Repeat step2 more than seven times with different search term)

3. Open about:home, focus search filed and press downarrow key

Actual Results:
Form history doropdown shows only 2 items

Expected Results:
Form history doropdown should show 6 items and vertical scrollbar if necessary

	Alice0775 White Reporter
	Comment 1 • 10 years ago

Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/6551323d96e6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140801134649
Bad:
https://hg.mozilla.org/mozilla-central/rev/eed9fe35a00d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140830030204
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6551323d96e6&tochange=eed9fe35a00d


Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/f08fb8275e5b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140801112450
Bad:
https://hg.mozilla.org/integration/fx-team/rev/fe53a2b0e7d0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140801120154
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f08fb8275e5b&tochange=fe53a2b0e7d0

Regressed by: Bug 612453 and Bug 1028985

	Drew Willcoxon :adw
	Comment 2 • 10 years ago

Thanks for filing.

SearchSuggestionController limits the number of form history entries here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm?rev=8cb9028743ac#203

That should probably be changed to return maxLocalResults + maxRemoteResults when the search string passed to fetch is empty.  (The search string is empty when you press the down key in about:home/newtab's search input.)

Bug 1046943 is kind of related in that it also changes maxLocalResults and maxRemoteResults to be more like hints than hard limits.

Marking this as blocking bug 1007979 as well, which really isn't fair because it's more bug 612453's fault, but as I mentioned the relevant code is in SearchSuggestionController.jsm.

	Drew Willcoxon :adw
	Comment 3 • 10 years ago

Circling back to this -- bug 1066787 is the long-term plan for suggestions on about:home/newtab, so I will add a dependency on it.  When that is fixed, about:home/newtab will show the same number of form history results as the search bar does (currently seven, but see bug 1060675).

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 4 • 10 years ago

Looks like accidental flagging on esr24, clearing.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 5 • 10 years ago

Bryan - 2 items for search history seems pretty limiting. What's your take on the importance of fixing this bug?

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 6 • 10 years ago

Yeah, this isn't good.  Would be nice if those two history search terms were the most recent, doesn't necessarily appear so from my testing.  We should get this fixed but it doesn't feel like a blocker to me.  You can still access the full list from the search input so we have a work around for now.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 7 • 10 years ago

Based on comment 6, I'm going to drop tracking for 34. However, I would be happy to take a safe patch early in beta if the desktop team wants to accept this into the backlog and prioritize it.

	Matthew N. [:MattN]
	Comment 8 • 10 years ago

(In reply to Drew Willcoxon :adw (Away 9/29–10/14) from comment #2)
> That should probably be changed to return maxLocalResults + maxRemoteResults
> when the search string passed to fetch is empty.  (The search string is
> empty when you press the down key in about:home/newtab's search input.)

This is also necessary when search suggestions are disabled by the user via the context menu in the search box. See bug 612453 comment 105.

	Matthew N. [:MattN]
	Comment 9 • 10 years ago

Do you have remote (search provider) suggestions disabled? See browser.search.suggest.enabled.

If so, a problem is that https://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=f7e5136269fa&mark=252#251 should take that pref into account.

	Matthew N. [:MattN]
	Comment 10 • 10 years ago

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #6)
> Yeah, this isn't good.  Would be nice if those two history search terms were
> the most recent, doesn't necessarily appear so from my testing.

It's sorted by frecency.

	Alice0775 White Reporter
	Comment 11 • 10 years ago

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #9)
> Do you have remote (search provider) suggestions disabled? See
> browser.search.suggest.enabled.
> 
> If so, a problem is that
> https://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.
> jsm?rev=f7e5136269fa&mark=252#251 should take that pref into account.


This is about form-history drop down.
Not suggestion. not related browser.search.suggest.enabled thing.

	Matthew N. [:MattN]
	Comment 12 • 10 years ago

It is related because we sometimes take different code paths depending on whether remote results are enabled.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 13 • 10 years ago

My patch in bug 1060675 produces the expected results for comment 0 (6 suggestions appear with those STR).

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 14 • 10 years ago

Though I'm not sure I understand why _onMessageGetSuggestions's maxRemoteResults/maxLocalResults behavior depends on whether suggestions are provided. Shouldn't we just remove that and rely on SearchSuggestionController's default values all the time?

	Matthew N. [:MattN]
	Comment 15 • 10 years ago

I think the reason was that we only show 6 results total on the about page (not ideal either) and so the default of maxLocalResults: 7 would mean no remote results would be visible in some cases.

One of the reasons for only 6 results total is because we would have to do more work to deal with adding scrolling to the result popup and adjusting its size based on the viewport since the dropdown is just HTML and must be constrained by the browser window content area. Bug 1066787 would do this for free.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 16 • 10 years ago

OK, let's call this fixed by bug 1060675 then. The "we should show a scrollbar" part should be split off to a new bug as needed.

	Petruta Horea [:phorea], Desktop QA
	Comment 17 • 10 years ago

Marking as verified based on verification of bug 1060675#c28.