Closed Bug 1195054 Opened 4 years ago Closed 4 years ago

The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
firefox43 --- affected
firefox48 --- fixed
firefox-esr38 --- affected

People

(Reporter: arni2033, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

User Story

The location bar should display 10 results without a scrollbar.

Attachments

(2 files)

STR:   (Win7, Nightly 43.0a1 (2015-08-15))
1. Open new tab.
2. Open Scratchpad (Shift+F4), paste "for(i=1;i<=12;i++)window.open('https://www.google.com/search?q='+i)" without outer quotes.
3. Run script (so that popups opened and saved in History).
4. Open new tab. Drag it from Tabs Toolbar to create new window.
5. Click textarea in location bar [it will become focused]
6. Press Alt+Down and hold for ~5 seconds
7. Type "a" (without quotes) in location bar

Result:       Location bar displays 12 annoying autocomplete suggestions.
              That behavior will stay for that window even if you open new tabs.
Expectations: It should display a box (#PopupAutoCompleteRichResult) with scrollbar
              where you only see 6 suggestions at a time

Note: It's a bit easier to reproduce on ESR38 - Beta41 (which don't have unified autocomplete).
      Modify Step 4 and follow all other steps:
4.* Open new tab.
See Also: → 1060340
I was able to reproduce this is in the past, there is a series of action that break autocomplete scrollbar.

I didn't try to debug it though cause the plan is to move to 8 results without a scrollbar, so it may be pointless double work.
Well thanks for info. Though 8 results are covering all screen as well as 12.
Acutally 6 results is too much too. Can't even see the text from page when typing it.
// I wonder if there's add-on that shows suggestions only when you press "Down" in location/search bar
Summary: Location bar displays 12 annoying autocomplete suggestions without scrollbar instead of displaying a box (#PopupAutoCompleteRichResult) with scrollbar where you only see 6 suggestions at a time → Location bar displays 12 autocomplete suggestions without scrollbar instead of 6 (don't fix - see comment 1)
it will be 8 single-line results, so it should take no more space than the current double-line 6 results.
Depends on: 1181078
Blocks: 1262507
User Story: (updated)
Summary: Location bar displays 12 autocomplete suggestions without scrollbar instead of 6 (don't fix - see comment 1) → The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6
No longer depends on: 1181078
Priority: -- → P1
Whiteboard: [fxsearch]
Blocks: 1251015
Duplicate of this bug: 1259575
This is one of the most important changes since the new design has landed I think, so I'll work on this next.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Here's what I think we should do, you might disagree.  People complain that not enough results are shown.  But they also complain that there's scrollbar, and I think they'll complain about that regardless of the maxRows of the popup, unless we make maxRichResults >= 2*maxRows or some large number like that.  Because otherwise you feel like the popup is unnecessarily hiding rows from you.

So this patch sets both maxRows and browser.urlbar.maxRichResults to 10.  If you disagree with browser.urlbar.maxRichResults, then I think 20 is the next smallest number that we should make it.  Incidentally the maxResults getter in autocomplete happens to return 20.

This actually sets defaultMaxRows instead of maxRows because the new smaller item height is part of toolkit, so I think any toolkit consumer of autocomplete would probably want a larger maxRows.

Review commit: https://reviewboard.mozilla.org/r/44673/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44673/
Attachment #8738719 - Flags: review?(mak77)
Blocks: 1060340
Comment on attachment 8738719 [details]
MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44673/diff/1-2/
Comment on attachment 8738719 [details]
MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

https://reviewboard.mozilla.org/r/44673/#review41581

I agree with you. we don't want a scrollbar in general.

::: browser/base/content/test/general/browser_autocomplete_autoselect.js:49
(Diff revision 2)
> -  info("Key Down 11 times should wrap around all the way around");
> -  repeat(11, () => EventUtils.synthesizeKey("VK_DOWN", {}));
> +  info("Key Down 10 times should wrap around all the way around");
> +  repeat(10, () => EventUtils.synthesizeKey("VK_DOWN", {}));
>    is_selected(1);
>  
> -  info("Key Up 11 times should wrap around the other way");
> -  repeat(11, () => EventUtils.synthesizeKey("VK_UP", {}));
> +  info("Key Up 10 times should wrap around the other way");
> +  repeat(10, () => EventUtils.synthesizeKey("VK_UP", {}));

I'd say to move this to a named const... or even better, read the value from maxRichrResults pref.

::: toolkit/components/places/UnifiedComplete.js:64
(Diff revision 2)
>  // "comment" back into the title and the tag.
>  const TITLE_TAGS_SEPARATOR = " \u2013 ";
>  
>  // Telemetry probes.
>  const TELEMETRY_1ST_RESULT = "PLACES_AUTOCOMPLETE_1ST_RESULT_TIME_MS";
> -const TELEMETRY_6_FIRST_RESULTS = "PLACES_AUTOCOMPLETE_6_FIRST_RESULTS_TIME_MS";
> +const TELEMETRY_10_FIRST_RESULTS = "PLACES_AUTOCOMPLETE_10_FIRST_RESULTS_TIME_MS";

This is not going to work unless you change histograms.json...

at this point the histogram would be "time needed to fetch ALL results", not just time for the first N results...

I'm not sure whether it-s still a good measure, I think we should retain it as it is so we have a baseline to compare to once we have the first performance fixes in.
So yeah, I'd not change it here.

::: toolkit/components/places/UnifiedComplete.js:1442
(Diff revision 2)
>                                 match.style,
>                                 match.finalCompleteValue);
>  
> -    if (this._result.matchCount == 6)
> -      TelemetryStopwatch.finish(TELEMETRY_6_FIRST_RESULTS, this);
> +    if (this._result.matchCount == 10) {
> +      TelemetryStopwatch.finish(TELEMETRY_10_FIRST_RESULTS, this);
> +    }

ditto
Attachment #8738719 - Flags: review?(mak77)
Btw, it's r=me without the telemetry changes and with a green Try run.
Comment on attachment 8738719 [details]
MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44673/diff/2-3/
Attachment #8738719 - Attachment description: MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r?mak → MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak
Attachment #8738719 - Flags: review?(mak77)
Attachment #8738719 - Flags: review?(mak77) → review+
Sorry, I just missed the fact the tree-based autocomplete is also affected by defaultMaxRows... Thus I'm not sure it's a good idea to change it in toolkit, cause we will affect all autocomplete widgets around...

I fear we'll have to keep defining maxRows in the browser binding.
Comment on attachment 8738719 [details]
MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44673/diff/3-4/
Attachment #8738719 - Flags: review+ → review?(mak77)
Attachment #8738719 - Flags: review?(mak77) → review+
Comment on attachment 8738719 [details]
MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

https://reviewboard.mozilla.org/r/44673/#review41595

yeah, sorry for the noise.
Attachment #8738719 - Flags: review+
I'm the one causing the noise with these sloppy patches. :-)
Those bc7 failures in browser_urlbarEnterAfterMouseOver.js on Mac and Windows on your try push? Yeah, those were yours. Backed out in https://hg.mozilla.org/integration/fx-team/rev/e7466050cdd2.
Sorry, Mac and Windows take forever to build on try recently.  Windows was fine.  I am not ashamed.
(In reply to Drew Willcoxon :adw from comment #21)
> Windows was fine.

Linux I meant.
https://hg.mozilla.org/integration/fx-team/rev/34293bb847d1

That comment was tounge-in-cheek by the way. :-)
https://hg.mozilla.org/mozilla-central/rev/34293bb847d1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 583683
Blocks: 1222432
You need to log in before you can comment on or make changes to this bug.