1311189 - No separator between datalist and form history entries with richlistbox implementation

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Bug 1296638 switched us from using a <xul:tree> to a <xul:richlistbox> to render the list of items in the autocomplete popup.

When a field has autocomplete results including both datalist and form history entries, we generally put the form history entries at the top, then a horizontal separator, and then the datalist entries.

We should probably still do that.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

Review commit: https://reviewboard.mozilla.org/r/86964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86964/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: autocomplete-separator-bug.png

So there's a slight problem with this patch in that there's a left (and right) transparent border that's interrupting the border-top that I've placed here. See screenshot.

I _think_ the transparent borders are to add a type of margin without messing with how the hover state looks.

Any suggestions on how to proceed here, MattN?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802605 [details]
Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86964/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Turns out that the transparent border is only needed for the AwesomeBar, and margins work fine for our purposes here.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8802605 [details]
Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

https://reviewboard.mozilla.org/r/86964/#review87110

Thanks Mike

::: toolkit/components/satchel/nsFormAutoComplete.js
(Diff revision 2)
> -        if (values.length) {
> -            comments[0] = "separator";
> -        }

I forgot how the separator was previously styled but if it was via CSS I wonder if that rule can be removed now?

::: toolkit/components/satchel/nsFormAutoCompleteResult.jsm:140
(Diff revision 2)
>        return "fromhistory";
>      }
>  
> -    if (!this._comments[index]) {
> -      return null;  // not a category label, so no special styling
> +    if (this._formHistResult.matchCount > 0 &&
> +        index == this._formHistResult.matchCount) {
> +      return "separator";

A more semantic name that doesn't dictate styling may be nicer (e.g. datalistfirst) but I don't feel strongly about this.

::: toolkit/components/satchel/nsFormAutoCompleteResult.jsm:143
(Diff revision 2)
> -    if (index == 0) {
> +    return null;
> -      return "suggestfirst";  // category label on first line of results
> -    }
> -
> -    return "suggesthint";   // category label on any other line of results

I kinda wonder if any add-on relied on these but on the other hand we can't leave cruft around forever so this is fine.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802605 [details]
Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

https://reviewboard.mozilla.org/r/86964/#review87110

> I forgot how the separator was previously styled but if it was via CSS I wonder if that rule can be removed now?

At least for trees, nope, the separator is another piece of native magic: http://searchfox.org/mozilla-central/rev/3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/layout/xul/tree/nsTreeBodyFrame.cpp#3185

> A more semantic name that doesn't dictate styling may be nicer (e.g. datalistfirst) but I don't feel strongly about this.

Sure, I like datalist-first. I'll use that.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802605 [details]
Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86964/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2849f953881f
Put back the separator between form history and datalist entries in the autocomplete popup. r=MattN

Comment 9

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

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5600039&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/2060475601bb

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802605 [details]
Bug 1311189 - Put back the separator between form history and datalist entries in the autocomplete popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86964/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3f252dc20b5
Put back the separator between form history and datalist entries in the autocomplete popup. r=MattN

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e3f252dc20b5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for 51 still?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Ignore that, some confusion over the versions set in the bug metadata.