Use richlistbox autocomplete by default

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
We should remove the last uses of tree-based autocomplete, for example in the preferences window, so we can remove its implementation from the tree.
Comment hidden (mozreview-request)
Priority: -- → P2
(Assignee)

Updated

a year ago
Depends on: 1429142
(Assignee)

Updated

a year ago
No longer depends on: 1429142
(Assignee)

Updated

a year ago
Depends on: 1284391
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
Once bug 1284391 is fixed, I think the preferences window is the only consumer left of the autogenerated richlistbox autocomplete in mozilla-central, however I kept the generic code we currently use to create the panel.
why is tags not autogenerated? Just for ac-site-icon?
(Assignee)

Comment 5

a year ago
Yep, I don't know of another way to assign an ID to the panel that we can use for styling it.
(Assignee)

Updated

a year ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED

Comment 6

a year ago
mozreview-review
Comment on attachment 8939093 [details]
Bug 1427366 - Use richlistbox autocomplete by default.

https://reviewboard.mozilla.org/r/209512/#review221666

with the premise that the code is just wrong and these things should be part of urlbarbinding, not of the basic ac (so anything we do is "wrong" from an ideal point of view); I wonder if we could invert the rule, display:none these things by default, and show them only where it matters. In the end I think the only one using all of these is the urlbar.

Thus I'd probably avoid "autogenerated", make it the default style, and add urlbar-only rules to fix it (those can use css selectors to apply only the urlbar without the need for an attribute).
Would this be crazy or hard?
(Assignee)

Comment 7

a year ago
(In reply to Marco Bonardo [::mak] from comment #6)
> Thus I'd probably avoid "autogenerated", make it the default style, and add
> urlbar-only rules to fix it (those can use css selectors to apply only the
> urlbar without the need for an attribute).
> Would this be crazy or hard?

That's just laborious, there are quite a few sites to audit and re-test after the change, including in-content:

https://dxr.mozilla.org/mozilla-central/search?q=.ac-&redirect=true

Maybe we can do this in a follow-up?
(In reply to :Paolo Amadini from comment #7)
> That's just laborious, there are quite a few sites to audit and re-test
> after the change, including in-content:

Is it not actually the same for the current change? It is touching the same widgets, and thus those same widgets need to be tested.
The main difference from your patch would be the removal of [autogenerated]. For most autocomplete fields it would work the same as your patch, since autocomplete.xml sets autogenerated = true by default in your patch.
Afaik, the only widgets showing an icon are the urlbar, the search boxes and the homepage in about:preferences (though I'm not sure what the new autofill does). The only one that overrides "popup" (where you set autogenerated = true) looks like the search widget. But maybe I'm over-simplifying the thing, you looked into it with more depth.
The list is a little bit shorter btw, since you are also including ac- things that won't change:
https://searchfox.org/mozilla-central/search?q=%5C.ac-(type-icon%7Ctags%7Csepatator%7Curl%7Caction)&case=false&regexp=true&path=

Alternatively, we could just file a follow-up to do the right thing, that is to remove these boxes completely from autocomplete.xml and move them to urlbarbindings.xml. I just fear it won't actually ever happen, for lack of resources, and this patch as-is will add some technical debt we'll never be able to pay.

For the tags autocomplete differences, maybe we could reuse autocompletepopup="PopupAutoComplete"?

There is some rules overlapping here, https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/browser/base/content/browser.css#625 and https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/toolkit/themes/shared/extensions/extensions.inc.css#1078. This again looks like it should be the default, and consumers should set ac-site-icon to display: -moz-box when they need it :( The whole thing is a mess.
(Assignee)

Comment 9

a year ago
(In reply to Marco Bonardo [::mak] from comment #8)
> (In reply to :Paolo Amadini from comment #7)
> > That's just laborious, there are quite a few sites to audit and re-test
> > after the change, including in-content:
> 
> Is it not actually the same for the current change? It is touching the same
> widgets, and thus those same widgets need to be tested.

Not exactly. Since we're not changing the default styles applied to panels that are not autogenerated, it's less likely we'll break any of them, like the in-content form autofill and the address bar, so we don't need to test them extensively.

The reason I'd do this in a follow-up is that this patch blocks a number of other binding removals. In the follow-up I don't think we have to remove the boxes, we can just make them hidden as you suggest and remove the "autogenerated" style. It's roughly the same amount of work, but without blocking the XBL removals.
(Assignee)

Updated

a year ago
Flags: needinfo?(mak77)

Comment 10

a year ago
mozreview-review
Comment on attachment 8939093 [details]
Bug 1427366 - Use richlistbox autocomplete by default.

https://reviewboard.mozilla.org/r/209512/#review222738

Ok, please file a follow-up bug for the removal of autogenerated, with as much details as possible. I still think this bug may end up increasing our technical debt, bug I don't want to block the binding removals momentum.

Though, you didn't answer my question about using autocompletepopup="PopupAutoComplete" for tags instead of defining separate rules.
Flags: needinfo?(mak77)
(Assignee)

Comment 11

a year ago
(In reply to Marco Bonardo [::mak] from comment #10)
> Though, you didn't answer my question about using
> autocompletepopup="PopupAutoComplete" for tags instead of defining separate
> rules.

Sorry, I missed the question. I'll look into this and see if PopupAutoComplete can be easily reused.
Blocks: 1434873
(Assignee)

Updated

a year ago
Blocks: 1434877
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8939093 [details]
Bug 1427366 - Use richlistbox autocomplete by default.

https://reviewboard.mozilla.org/r/209512/#review223566

The changes look good to me, though Try is showing an a11y failure that looks related to this change
Attachment #8939093 - Flags: review?(mak77)
(Assignee)

Updated

a year ago
Blocks: 1435711
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
Comment on attachment 8939093 [details]
Bug 1427366 - Use richlistbox autocomplete by default.

https://reviewboard.mozilla.org/r/209512/#review224224

Thanks
Attachment #8939093 - Flags: review?(mak77) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8939093 [details]
Bug 1427366 - Use richlistbox autocomplete by default.

https://reviewboard.mozilla.org/r/209512/#review224314

::: accessible/tests/mochitest/tree/test_txtctrl.xul:138
(Diff revision 5)
>        var txc = document.getElementById("txc_autocomplete");
>        SimpleTest.ok(txc, "Testing (New) Toolkit autocomplete widget.");
>  
>        // Dumb access to trigger popup lazy creation.
>        dump("Trigget popup lazy creation");
>        waitForEvent(EVENT_REORDER, txc, test_AutocompleteControl);

it'd be nice to inline the function like
() => {
  testAccessibleTree("txc_autocomplete", accTree);
  SimpleTest.finish();
};
Attachment #8939093 - Flags: review?(surkov.alexander) → review+

Comment 20

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c152ad62b0
Use richlistbox autocomplete by default. r=mak,surkov
https://hg.mozilla.org/mozilla-central/rev/63c152ad62b0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 22

a year ago
Marco, will TB be affected by this?
Flags: needinfo?(mak77)
(In reply to Jorg K (GMT+1) from comment #22)
> Marco, will TB be affected by this?

It's likely, you may want to check your autocomplete fields and verify they still appear correctly. Otherwise it may need some css adjustments.
Flags: needinfo?(mak77)

Comment 24

a year ago
Thanks Marco, yes, this needs some CSS changes, bug 1436764.

Updated

a year ago
Depends on: 1437222
No longer depends on: 1449018
You need to log in before you can comment on or make changes to this bug.