1546301 - [de-xbl] convert the ruleactiontype-menulist binding to <menulist is="ruleactiontype-menulist">

Status: RESOLVED FIXED

(Thunderbird :: Filters, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mailnews/base/search/content/searchWidgets.xml#30

The ruleactiontype-menulist binding can be converted to a customized built-in.
Should be similar to e.g. menulist-charsetpicker-viewing.

Comment 1

[Arshad Khan [:arshad]]

Attachment: ruleactiontypemenulistWIP.patch

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1546301_de-xbl_ruleactiontype-menulist.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9071180 [details] [diff] [review]
Bug-1546301_de-xbl_ruleactiontype-menulist.patch

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

Seems to work, so r=mkmelin with the below

::: mailnews/base/search/content/searchWidgets.js
@@ +1038,5 @@
> +   */
> +  class MozRuleactiontypeMenulist extends customElements.get("menulist") {
> +    connectedCallback() {
> +      super.connectedCallback();
> +      if (this.delayConnectedCallback()) {

should protect against being run more than once, so add this.hasChildNodes()

@@ +1087,5 @@
> +      let scope = getScopeFromFilterList(gFilterList);
> +
> +      // walk through the list of filter actions and hide any actions which aren't valid
> +      // for our given scope (news, imap, pop, etc) and context.
> +      let elements, i;

please just declare the i everywhere it's used, not here

@@ +1138,5 @@
> +      }
> +    }
> +
> +    /**
> +     * returns a hash containing all of the filter actions which are currently

R

Comment 4

[Khushil Mistry [:khushil324]]

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

should protect against being run more than once, so add this.hasChildNodes()

We are not inserting the elements in the connected callback, the child elements are inserted in the XUL file only. So When connectedCallback will run, there will be child elements in the menulist. Any other to prevent connected callback for running 2nd time?

Comment 5

[Khushil Mistry [:khushil324]]

(In reply to Khushil Mistry [:khushil324] from comment #4)

We are not inserting the elements in the connected callback, the child elements are inserted in the XUL file only. So When connectedCallback will run, there will be child elements in the menulist. Any other to prevent connected callback for running 2nd time?

Got this with this.hasConnected.

Comment 6

[Magnus Melin [:mkmelin]]

Ok, you can use that. (But even if we don't insert children we should protect against running twice so we don't double up event handlers and such.)

Comment 7

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=90208ee5abbbd56fef045db022ecf9f97b329fda&selectedJob=252765073

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1546301_dexbl_ruleactiontype-menulist.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e9bc2e1580a0
[de-xbl] convert the ruleactiontype-menulist binding to <menulist is='ruleactiontype-menulist'>. r=mkmelin DONTBUILD

Comment 10

[Jorg K (CEST = GMT+2)]

I pushed that with DONTBUILD since there was a green try and Daily will take care of it.

Comment 11

[:aceman]

Comment on attachment 9073159 [details] [diff] [review]
Bug-1546301_dexbl_ruleactiontype-menulist.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global MozXULElement, gFilter, gFilterList, onEnterInSearchTerm, convertDateToString, initializeTermFromId,
> +   convertPRTimeToString, convertStringToPRTime, UpdateAfterCustomHeaderChange, checkActionsReorder,
> +   initializeTermFrom, IdcheckActionsReorder, getScopeFromFilterList, gCustomActions, gFilterType */

Aren't the lines too long?

@@ +9,2 @@
>  
> +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");

Why are the spaces better?

@@ +91,4 @@
>  
>      this.appendChild(menulist);
>  
> +    document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "is", "ruleactiontype-menulist")

Long line.

@@ +1051,5 @@
> +      });
> +
> +      this.addEventListener("popupshowing", (event) => {
> +        let unavailableActions = this.usedActionsList();
> +        for (let index = 0; index < this.menuitems.length; index++) {

Wouldn't this take 'for (let menu of this.menuitems)' ?

@@ +1068,5 @@
> +      // and creating a row which will be initialized with an action.
> +      if (!this.parentNode.hasAttribute("initialActionIndex")) {
> +        let unavailableActions = this.usedActionsList();
> +        // Select the first one that's not in the list.
> +        for (let index = 0; index < this.menuitems.length; index++) {

Again.