No separator between datalist and form history entries with richlistbox implementation

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

({regression})

Trunk
mozilla52
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 2

2 years ago
Created attachment 8802610 [details]
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
Turns out that the transparent border is only needed for the AwesomeBar, and margins work fine for our purposes here.
Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

2 years ago
Attachment #8802610 - Attachment is obsolete: true
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.
Attachment #8802605 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
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/﷒0> 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 hidden (mozreview-request)

Comment 8

2 years ago
Pushed by mconley@mozilla.com:
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 hidden (mozreview-request)

Comment 11

2 years ago
Pushed by mconley@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3f252dc20b5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Assignee: nobody → mconley
(Assignee)

Updated

2 years ago
Flags: needinfo?(mconley)
Should we consider this for 51 still?
status-firefox50: --- → wontfix
status-firefox51: --- → affected
Flags: needinfo?(mconley)
Ignore that, some confusion over the versions set in the bug metadata.
status-firefox50: wontfix → ---
status-firefox51: affected → ---
Flags: needinfo?(mconley)
Version: 49 Branch → Trunk

Updated

2 years ago
Blocks: 1328557
You need to log in before you can comment on or make changes to this bug.