Closed Bug 1542711 Opened 6 years ago Closed 6 years ago

[de-xbl] remove addrbooks-menupopup binding: refactor to using <menulist is="menulist-addrbooks" > instead

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 6 obsolete files)

de-xbl-addrbooks-menupopup.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

Or, actually, it seems like it could be just refactored to be a customized <menulist> instead, like https://searchfox.org/comm-central/source/mailnews/base/content/menulist-charsetpicker.js

Summary: [de-xbl] remove addrbooks-menupopup binding: convert to <menupopup is="addrbooks-menupopup"> → [de-xbl] remove addrbooks-menupopup binding
Attached patch de-xbl-addrbooks-menupopup.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058848 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9058848 [details] [diff] [review] de-xbl-addrbooks-menupopup.patch Review of attachment 9058848 [details] [diff] [review]: ----------------------------------------------------------------- This is used so often it seems better to put it into our customElements.js Can you hg mv --after mailnews/addrbook/content/addrbookWidgets.xml mailnews/addrbook/content/menulist-addrbook.js to preserve some history ::: mail/base/content/ABSearchDialog.xul @@ +62,5 @@ > <vbox> > <hbox align="center"> > <label value="&abSearchHeading.label;" accesskey="&abSearchHeading.accesskey;" control="abPopup"/> > + <menulist is="menulist-addrbooks" > + id="abPopup" we've usually put the is and id on the same line for greppability (and is is kind of part of the definition) <menulist is="menulist-addrbooks" id="abPopup" ::: mail/base/content/messenger.css @@ +79,2 @@ > -moz-binding: url("chrome://messenger/content/addressbook/addrbookWidgets.xml#addrbooks-menupopup"); > +} */ remove ::: mail/components/addrbook/content/menulist-addrbook.js @@ +7,5 @@ > + const { fixIterator } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm"); > + /** > + * MozMenulistAddressBookList is a menulist widget that is automatically > + * populated with the complete address book list. > + * @implements {nsIAbListener} hmm, you can't usually have the implementation directly on the CE. Does this really work? (Usually we've used a helper object, though Paul found another trick)
Attachment #9058848 - Flags: review?(mkmelin+mozilla)

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

This is used so often it seems better to put it into our customElements.js

The address book section doesn't seem to have access to the customElements.js file, and if I register the menulist-addrbook.js, I get this error:

JavaScript error: chrome://messenger/content/addressbook/menulist-addrbook.js, line 272: NotSupportedError: Operation is not supported

Do you want me the include the customElements.js in the address book where it's missing or should we keep including the menulist-addrbook.js only where it's used.

Flags: needinfo?(mkmelin+mozilla)

I think you can include customElements.js in the address book

Flags: needinfo?(mkmelin+mozilla)
Attached patch de-xbl-addrbooks-menupopup.patch (obsolete) (deleted) — — Splinter Review

I managed to implement the interface with this method:

Cc["@mozilla.org/abmanager;1"]
        .getService(Ci.nsIAbManager)
        .addAddressBookListener(this.AddressBookListener, Ci.nsIAbListener.all);

I tested everything and it works, but I'm not sure this is the right and cleaner approach to implement it. It feels a bit hacky.

I included the customElements.js file where necessary, and tested all the menu lists to be sure are working.

Attachment #9058848 - Attachment is obsolete: true
Attachment #9059128 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9059128 [details] [diff] [review] de-xbl-addrbooks-menupopup.patch Review of attachment 9059128 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/addrbook/content/menulist-addrbook.js @@ +13,5 @@ > + const { fixIterator } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm"); > + /** > + * MozMenulistAddressBookList is a menulist widget that is automatically > + * populated with the complete address book list. > + * @implements {nsIAbListener} I think you should remove this from here since it's not this object that is implementing it @@ +16,5 @@ > + * populated with the complete address book list. > + * @implements {nsIAbListener} > + * @extends {MozMenuList} > + */ > + class MozMenulistAddressBookList extends customElements.get("menulist") { Can you change the class name to correspond to the attribute. I mean like MozMenulistAddrbooks @@ +32,5 @@ > + this._value = this.getAttribute("value") || "URI"; > + > + this._rebuild(); > + > + this.AddressBookListener = { with a lowercase a please. I'd add this above: // @implements {nsIAbListener}, and then not have the comment for each method @@ +33,5 @@ > + > + this._rebuild(); > + > + this.AddressBookListener = { > + menulist: null, please remove this and use this instead. it will work when you do => as below @@ +36,5 @@ > + this.AddressBookListener = { > + menulist: null, > + > + // nsIAbListener > + onItemAdded(aParentDir, aItem) { onItemAdded: (aParentDir, aItem) => { @@ +44,5 @@ > + } > + }, > + > + // nsIAbListener > + onItemRemoved(aParentDir, aItem) { same for these of course @@ +87,5 @@ > + } > + } > + }; > + > + this.AddressBookListener.menulist = this; and this can then go
Attachment #9059128 - Flags: review?(mkmelin+mozilla) → feedback+
Status: NEW → ASSIGNED
Attached patch de-xbl-addrbooks-menupopup.patch (obsolete) (deleted) — — Splinter Review

Updated the patch accordingly. Thanks for the heads up on the arrow function to prevent changing the scope.

Attachment #9059128 - Attachment is obsolete: true
Attachment #9059988 - Flags: review?(mkmelin+mozilla)
Attached image Screenshot from 2019-04-22 17.54.30.png (obsolete) (deleted) —

I noticed that the File -> New menu item is not available anymore in the address book dialog, even if the code of the menu is present in the addressbook.xul file.
I popped my patch to see if I was affecting it in some way, but I'm not.
Was it removed on purpose or is this a bug?

The item is there but not visible. It seems this is an issue since the menuitem deXBLification like we have in AppMenu too.

(In reply to Richard Marti (:Paenglab) from comment #10)

The item is there but not visible. It seems this is an issue since the menuitem deXBLification like we have in AppMenu too.

Is there a bug already filed for this?

Magnus, is this patch ready for checkin?

Flags: needinfo?(mkmelin+mozilla)
Attached patch de-xbl-addrbooks-menupopup.patch (obsolete) (deleted) — — Splinter Review

I fixed a small linting problem.

I did a try run and got some failed mochitests: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46c7f39df58b7d2105ef7eab4cace216c6524ea3

I can't seem to figure out where are these coming from as the error message says:

TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_mailTabs.js
| uncaught exception - TypeError: this.delayConnectedCallback is not a function at
connectedCallback@chrome://global/content/elements/menu.js:275:14

Where is this chrome://global/content/elements/menu.js file?

Attachment #9059988 - Attachment is obsolete: true
Attachment #9059988 - Flags: review?(mkmelin+mozilla)
Attachment #9060279 - Flags: review+
Attachment #9060279 - Flags: review+ → review?(mkmelin+mozilla)

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

(In reply to Richard Marti (:Paenglab) from comment #10)

The item is there but not visible. It seems this is an issue since the menuitem deXBLification like we have in AppMenu too.

Is there a bug already filed for this?

Maybe there is a TB de-XBL bug open that fixes it. Magnus may know more, I hope.

Comment on attachment 9060279 [details] [diff] [review] de-xbl-addrbooks-menupopup.patch Review of attachment 9060279 [details] [diff] [review]: ----------------------------------------------------------------- I think it basically looks good, but apparently some (general?) customElements problems on trunk. I get JavaScript error: chrome://messenger/content/preferences/compose.js, line 152: TypeError: dirList.menupopup is null and a bunch of JavaScript error: chrome://global/content/customElements.js, line 341: TypeError: list is not iterable ::: mailnews/base/search/content/searchWidgets.js @@ +656,4 @@ > </menupopup> > </menulist> > <textbox flex="1" class="search-value-textbox" inherits="disabled"></textbox> > + <menulist is="menulist-addrbooks" flex="1" class="search-value-menulist" inherits="disabled" localonly="true"/> I notice now, this CE should for correctness really react to changes in localonly, remoteonly. But we can do that as a followup.
Flags: needinfo?(mkmelin+mozilla)
Attached patch de-xbl-addrbooks-menupopup.patch (obsolete) (deleted) — — Splinter Review

I fixed the mochitest error:
JavaScript error: chrome://messenger/content/preferences/compose.js, line 152: TypeError: dirList.menupopup is null

Those were happening because after converting the menulist to a CE, the menupopup is not accessible anymore as it was with XBL.

Doing menulist.menupopup.querySelector() doesn't work. It needed to be updated to menulist.querySelector('menupopup ...').

Where are you getting those customElements.js errors?

Attachment #9060279 - Attachment is obsolete: true
Attachment #9060279 - Flags: review?(mkmelin+mozilla)
Attachment #9060480 - Flags: review?(mkmelin+mozilla)

Those are from bug 1546607.

The try push was successful, yay!
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4ee8af26815fce190c74ee85ebf0061241198179

If the patch gets r+, it'll be ready for a check-in.

Comment on attachment 9060480 [details] [diff] [review] de-xbl-addrbooks-menupopup.patch Review of attachment 9060480 [details] [diff] [review]: ----------------------------------------------------------------- Something is wrong - in the contacts sidebar (when composing: F9), changing the addressbook doesn't work. In the native console I get an error, and apparently GetDirectoryFromURI gets passed undefined for an uri. Can you rename it to menulist-addrbooks.js with an s on the end for consistency? Other than that it looks good I think.
Attachment #9060480 - Flags: review?(mkmelin+mozilla) → review-
Summary: [de-xbl] remove addrbooks-menupopup binding → [de-xbl] remove addrbooks-menupopup binding: refactor to using <menulist is="menulist-addrbooks" > instead

Uh, sorry about that, I fixed that and run all the mochitests locally successfully.

Attachment #9059989 - Attachment is obsolete: true
Attachment #9060480 - Attachment is obsolete: true
Attachment #9060830 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9060830 [details] [diff] [review] de-xbl-addrbooks-menupopup.patch Review of attachment 9060830 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now, thx! r=mkmelin
Attachment #9060830 - Flags: review?(mkmelin+mozilla) → review+

I don't think this needs another try run since the change was small and the error wasn't caught by tests the first time either.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0b4ea11f26f8
[de-xbl] remove addrbooks-menupopup binding. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

Why is this reimplemented on the menulist level, and not menupopup as it was? You can't now put it into a different parent like e.g. <menu>.
Also, why is menulist.menupopup not accessible? That was really handy.

Well, we aren't using any of that so seems pointless to make it special.

menulist.menupopup: looks like that still exists?

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

Well, we aren't using any of that so seems pointless to make it special.

It would not be special, just preserving the current state. Also folder picker is implemented on the popup level to allow it to be used in various places. Why lose the possibility for the AB list?

menulist.menupopup: looks like that still exists?

It is said in comment 15 that it doesn't work and also calls to it are removed in the patch.

(In reply to :aceman from comment #25)

Why lose the possibility for the AB list?

That would have been possible, but we only using this widget as a list of address books. I don't see where we'd add it as a menu, so if it's only used as one thing, it's convenient to just have it be one thing.

menulist.menupopup: looks like that still exists?

It is said in comment 15 that it doesn't work and also calls to it are
removed in the patch.

Not sure why it wouldn't work though. Maybe the popup wasn't built yet when called? It's there: https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/toolkit/content/widgets/menulist.js#179

(In reply to :aceman from comment #23)

Also, why is menulist.menupopup not accessible? That was really handy.

Is only accessible if the popup is built.
Before, having the xbl version hardcoded this way <menulist...><menupopup../></menulist> allowed to use the menulist.menupopup selector no matter if the popup was built or not.

Since the menupopup element is now lazily generated with the CE, we can't give for granted that the menupopup selector is always available.

Regressions: 1549257
Regressions: 1560751
Blocks: 1563931
Regressions: 1574712
Regressions: 1591364
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: