1554637 - [de-xbl] convert the glodacomplete-rich-result-popup binding

Status: RESOLVED FIXED

(Thunderbird :: Search, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/mailnews/db/gloda/content/glodacomplete.xml#13

There doesn't appear to be an m-c bug for autocomplete-rich-result-popup + urlbar-rich-result-popup conversion yet.

Maybe bug 1534455 will remove it.

Comment 1

[Magnus Melin [:mkmelin]]

There is a patch removing autocomplete-rich-result-popup (and autocomplete) bindings in bug 1534455, not complete atm, but it might in a week or two(?), so we should get started with this soon.

Comment 2

[Tim Nguyen :ntim]

(In reply to Magnus Melin [:mkmelin] from comment #1)

There is a patch removing autocomplete-rich-result-popup (and autocomplete) bindings in bug 1534455, not complete atm, but it might in a week or two(?), so we should get started with this soon.

I suspect autocomplete-rich-result-popup will be removed as part of bug 1551227 (I was told sometime in the 69 cycle, but could be later) rather than bug 1534455 since Firefox's old URL bar still depends on it. I'm only planning to finish bug 1534455 once that's done.

Comment 3

[Tim Nguyen :ntim]

Just heads up that bug 1562274 removes urlbar-rich-result-popup which is Firefox's only usage of autocomplete-rich-result-popup.

Comment 4

[Tim Nguyen :ntim]

What you'd probably want to do here is:

• Combine autocomplete-rich-result-popup and glodacomplete-rich-result-popup in one single XBL binding
• Run that binding through the converter: https://bgrins.github.io/xbl-analysis/converter/
• Add/define the custom element as needed
• Fix any remaining issues

Comment 5

[Alessandro Castellani [:aleca]]

Thanks for the heads up, I've been tracking bug 1534455 to be sure to not miss the train.
I'll get on to it right away.

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1554637-glodacomplete-xbl-WIP.patch

This is a temporary patch which brings the autocomplete-rich-result-popup binding from toolkit to the glodacomplete.xml file.
Just in case it gets removed from toolkit before this new CE is ready, and we need to temporarily land this on trunk in order to not have a broken autocomplete.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1542720-dexbl-glodacomplete.patch

Here's an initial patch to de-xbl the glodacomplete richlist.

I need some help with this one to figure out why it doesn't work at all.
The autocomplete panel doesn't show up, no errors are triggered, nothing seems to work.

I don't know if it's related to some misconfiguration of the observers in bug 1542720, or something silly I'm doing here.

Also, I can't seem to figure out if and how the <hbox><children/></hbox> section was used at all.

Comment 8

[Tim Nguyen :ntim]

(In reply to Alessandro Castellani (:aleca) from comment #7)

Created attachment 9077308 [details] [diff] [review]
1542720-dexbl-glodacomplete.patch

Here's an initial patch to de-xbl the glodacomplete richlist.

I need some help with this one to figure out why it doesn't work at all.
The autocomplete panel doesn't show up, no errors are triggered, nothing seems to work.

If you have bug 1534455 applied, then the popup won't appear unless you make some changes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1534455#c5

Regardless, you should be able to work on this bug without bug 1534455 or bug 1542720 applied.

Comment 9

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #8)

If you have bug 1534455 applied, then the popup won't appear unless you make some changes, see https://bugzilla.mozilla.org/show_bug.cgi?id=1534455#c5

Btw, note that :aswan will be taking over bug 1534455, so I don't know what his plan is.

Comment 10

[Alessandro Castellani [:aleca]]

(In reply to Tim Nguyen :ntim from comment #8)

Regardless, you should be able to work on this bug without bug 1534455 or bug 1542720 applied.

Sweet, thank you so much, this helps me a lot.

Btw, note that :aswan will be taking over bug 1534455, so I don't know what his plan is.

Thanks for the heads up, I will bother him :D

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1542720-dexbl-glodacomplete.patch

All right, this one works with some hacky things.

If I use <panel is="autocomplete-rich-result-popup" it works, but I have to override the _appendCurrentResult() and _invalidate() method in the MozAutocompleteRichResultPopup class with what we need. (You can see I left the original methods commented out)

I'd like to extend the MozAutocompleteRichResultPopup class and use the CE <glodacomplete-rich-result-popup/> or <panel is="glodacomplete-rich-result-popup", but I can't seem to make it work.

Suggestions?

Comment 12

[Tim Nguyen :ntim]

(In reply to Alessandro Castellani (:aleca) from comment #11)

Created attachment 9077317 [details] [diff] [review]
1542720-dexbl-glodacomplete.patch

All right, this one works with some hacky things.

If I use <panel is="autocomplete-rich-result-popup" it works, but I have to override the _appendCurrentResult() and _invalidate() method in the MozAutocompleteRichResultPopup class with what we need. (You can see I left the original methods commented out)

I'd like to extend the MozAutocompleteRichResultPopup class and use the CE <glodacomplete-rich-result-popup/> or <panel is="glodacomplete-rich-result-popup", but I can't seem to make it work.

Suggestions?

Since glodacomplete-rich-result-popup is the only consumer of autocomplete-rich-result-popup right now in comm-central, can't you combine both ? This way you won't need to worry about overriding/extending, and the code will generally be shorter. If someone wants to bring back autocomplete-rich-result-popup for whatever reason in the future, they can look into the commit history.

What I'd do is just combine them both and have class MozGlodacompleteRichResultPopup extends MozPopupElement.

Also, if you want to use it as <panel is="glodacomplete-rich-result-popup" /> (which would make sense since you're extending MozPopupElement), you should do: customElements.define("glodacomplete-rich-result-popup", MozGlodacompleteRichResultPopup, { extends: "panel" });

Comment 13

[Alessandro Castellani [:aleca]]

(In reply to Tim Nguyen :ntim from comment #12)

Since glodacomplete-rich-result-popup is the only consumer of autocomplete-rich-result-popup right now in comm-central, can't you combine both ?

Sweet, thanks for confirming the approach I was aiming for.

Comment 14

[Tim Nguyen :ntim]

Oh btw, I've triggered bug 1564969 for landing, so it should be in mozilla-central sometime tomorrow.

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: 1542720-dexbl-glodacomplete.patch

This is ready for a full review.

I found a bug where the panel shows up at the bottom of the main window.
This happens in 60.7.2 as well, so it's not related to this de-xbl, but I can't figure out how to solve it.

To reproduce the bug:

• select the main searchbox, type something and with the arrow keys select the first entry of the autocomplete "Messages mentioning: ..."
• In the search results tab, type another letter in the search field and hit enter.
• Type another letter.

The panel will appear at the bottom right corner of the window.

Comment 16

[Alessandro Castellani [:aleca]]

(In reply to Tim Nguyen :ntim from comment #14)

Oh btw, I've triggered bug 1564969 for landing, so it should be in mozilla-central sometime tomorrow.

Thanks for the heads up, I saw that.
We should be able to land this today so it shouldn't be a problem.
Thank you so much for your help and support!

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9077443 [details] [diff] [review]
1542720-dexbl-glodacomplete.patch

Review of attachment 9077443 [details] [diff] [review]:
-----------------------------------------------------------------

This looks okay to me, although I haven't had a chance to test it works, my build is busted, and I have to go out now.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1542720-dexbl-glodacomplete.patch

This is ready to go as I fixed the issue I described in comment 15.

The issue persists with the XBL autocomplete.xml, but with my fix and the soon to land de-xbl autocomplete of bug 1534455, it's all good.

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8e9cd73b4c2f07685506e59e28ce928ab7efc468

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a48966e47592
[de-xbl] convert the glodacomplete-rich-result-popup. r=darktrojan