[de-xbl] remove addrbooks-menupopup binding: refactor to using <menulist is="menulist-addrbooks" > instead
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 6 obsolete files)
|
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
Screenshot from 2019-04-22 17.54.30.png
Looks like the addrbooks-menupopup binding can now be converted to a customized built-in. See bug 1531296 for samples.
Consumers: https://searchfox.org/comm-central/search?q=addrbooksPopup&case=false®exp=false&path=
|
Reporter | |
Comment 1•6 years ago
|
||
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
|
|
Reporter | |
Comment 3•6 years ago
|
||
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)
|
Assignee | |
Comment 4•6 years ago
|
||
(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.
|
|
Reporter | |
Comment 5•6 years ago
|
||
I think you can include customElements.js in the address book
|
|
Assignee | |
Comment 6•6 years ago
|
||
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.
|
|
Reporter | |
Comment 7•6 years ago
|
||
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
|
|
Reporter | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 8•6 years ago
|
||
Updated the patch accordingly. Thanks for the heads up on the arrow function to prevent changing the scope.
|
|
Assignee | |
Comment 9•6 years ago
|
||
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?
|
||
Comment 10•6 years ago
|
||
The item is there but not visible. It seems this is an issue since the menuitem deXBLification like we have in AppMenu too.
|
|
Assignee | |
Comment 11•6 years ago
|
||
(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?
|
|
Assignee | |
Comment 12•6 years ago
|
||
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?
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 13•6 years ago
|
||
(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.
|
|
Reporter | |
Comment 14•6 years ago
|
||
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.
|
|
Reporter | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 15•6 years ago
|
||
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?
|
|
Assignee | |
Comment 17•6 years ago
|
||
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.
|
|
Reporter | |
Comment 18•6 years ago
|
||
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.
|
|
Reporter | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 19•6 years ago
|
||
Uh, sorry about that, I fixed that and run all the mochitests locally successfully.
|
|
Reporter | |
Comment 20•6 years ago
|
||
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
|
|
Reporter | |
Comment 21•6 years ago
|
||
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.
|
||
Comment 22•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0b4ea11f26f8
[de-xbl] remove addrbooks-menupopup binding. r=mkmelin
|
||
Updated•6 years ago
|
|
||
Comment 23•6 years ago
|
||
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.
|
|
Reporter | |
Comment 24•6 years ago
|
||
Well, we aren't using any of that so seems pointless to make it special.
menulist.menupopup: looks like that still exists?
|
|
||
Comment 25•6 years ago
|
||
(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.
|
|
Reporter | |
Comment 26•6 years ago
|
||
(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
|
|
Assignee | |
Comment 27•6 years ago
|
||
(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.
Description
•