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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[arni2033]

Attachment: Location bar displays 12 annoying autocomplete suggestions without scrollbar.webm

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[arni2033]

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

Comment 3

[Marco Bonardo [:mak]]

it will be 8 single-line results, so it should take no more space than the current double-line 6 results.

Comment 5

[Drew Willcoxon :adw]

This is one of the most important changes since the new design has landed I think, so I'll work on this next.

Comment 6

[Drew Willcoxon :adw]

Attachment: MozReview Request: Bug 1195054 - The location bar should displays 10 autocomplete suggestions without scrollbar instead of 6. r=mak

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/

Comment 7

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f47e93d45f8

Comment 8

[Drew Willcoxon :adw]

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 9

[Drew Willcoxon :adw]

Fix for a test failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ddee51ed0f0

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

Btw, it's r=me without the telemetry changes and with a green Try run.

Comment 12

[Drew Willcoxon :adw]

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/

Comment 13

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a578ba4e4457

Comment 14

[Marco Bonardo [:mak]]

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 15

[Drew Willcoxon :adw]

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/

Comment 16

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce91fb3ce856

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Drew Willcoxon :adw]

I'm the one causing the noise with these sloppy patches. :-)

Comment 19

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/5815d4fc3043

Comment 20

[Phil Ringnalda (:philor)]

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.

Comment 21

[Drew Willcoxon :adw]

Sorry, Mac and Windows take forever to build on try recently.  Windows was fine.  I am not ashamed.

Comment 22

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #21)
> Windows was fine.

Linux I meant.

Comment 23

[Drew Willcoxon :adw]

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

That comment was tounge-in-cheek by the way. :-)

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34293bb847d1