1489172 - [de-xbl] Remove ruleactiontarget bindings.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbase.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: patch-1_ruleactiontargetbase.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: patch-2_ruleactiontargettag.patch

Comment 4

[Arshad Khan [:arshad]]

Attachment: patch-3_ruleactiontargetpriority.patch

Comment 5

[Arshad Khan [:arshad]]

Comment on attachment 9018926 [details] [diff] [review]
patch-3_ruleactiontargetpriority.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +47,5 @@
>  
> +class MozRuleactiontargetPriority extends MozRuleactiontargetBase {
> +    connectedCallback() {
> +        super.connectedCallback();
> +        this.appendChild(MozXULElement.parseXULToFragment(`

using parseXULToFragment since menupopup has an xbl binding

Comment 6

[Arshad Khan [:arshad]]

Attachment: patch-4_ruleactiontargetjunkscore.patch

Comment 7

[Arshad Khan [:arshad]]

Attachment: patch-5_ruleactiontargetreplyto.patch

Comment 8

[Arshad Khan [:arshad]]

Attachment: patch-6_ruleactiontargetforwardto.patch

Comment 9

[Arshad Khan [:arshad]]

Attachment: patch-7_ruleactiontargetfolder.patch

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9018926 [details] [diff] [review]
patch-3_ruleactiontargetpriority.patch

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

For this and others, remember 2 space indent

::: mailnews/base/search/content/searchWidgets.js
@@ +55,5 @@
> +            <menuitem value="5" label="FROM-DTD-highPriorityCmd-label"></menuitem>
> +            <menuitem value="4" label="FROM-DTD-normalPriorityCmd-label"></menuitem>
> +            <menuitem value="3" label="FROM-DTD-lowPriorityCmd-label"></menuitem>
> +            <menuitem value="2" label="FROM-DTD-lowestPriorityCmd-label"></menuitem>
> +          </menupopup>

for the labels, I think you need to move to javascript. and to using Fluent. 

Shold be something like this:

let l10n = new Localization([
  "foo/bar.ftl",
]);
let labeles = await this._l10n.formatMessages([{
     {id: "highest-priority-cmd"},
     {id: "high-priority-cmd"},
}]);

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9018934 [details] [diff] [review]
patch-7_ruleactiontargetfolder.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +112,5 @@
> +        <menulist class="ruleactionitem folderMenuItem" displayformat="verbose" oncommand="this.parentNode.setPicker(event);">
> +          <menupopup type="folder" mode="filing" class="menulist-menupopup" showRecent="true" recentLabel="FROM-DTD-recentFolders-label" showFileHereLabel="true"></menupopup>
> +        </menulist>
> +        `));
> +        this.menulist = document.querySelector("menulist");

this.querySelector(...) ?

@@ +114,5 @@
> +        </menulist>
> +        `));
> +        this.menulist = document.querySelector("menulist");
> +
> +        ChromeUtils.import("resource:///modules/MailUtils.jsm", this);

you can import this at top of the file I think

Comment 12

[Arshad Khan [:arshad]]

Attachment: patch-8-ruleactionbindingsfix.patch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9018925 [details] [diff] [review]
patch-2_ruleactiontargettag.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +25,5 @@
> +
> +        menuList.appendChild(menuPopup);
> +
> +        let tagArray = MailServices.tags.getAllTags({});
> +        for (let i = 0; i < tagArray.length; ++i) {

you could switch this to 
for (let taginfo of MailServices.tags.getAllTags({}))

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9021805 [details] [diff] [review]
patch-8-ruleactionbindingsfix.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +167,5 @@
> +
> +    _updateAttributes() {
> +        const type = this.getAttribute("type");
> +
> +        if (type != null) {

for null, remote the node, and return early

@@ +169,5 @@
> +        const type = this.getAttribute("type");
> +
> +        if (type != null) {
> +            switch (type) {
> +                case "movemessage":

you could do just have a type-element mapping

 { 
   "movemessage" : "ruleactiontarget-folder",
   "copymessage" : "ruleactiontarget-folder",
...

Comment 15

[Arshad Khan [:arshad]]

Attachment: patch-8_ruleactionbindingsfix.patch

addressed magnus's comments.

Comment 16

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aeb1cab24c568a3d4e978e905473e835b76241d8

Comment 17

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9022368 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Some nits below. I tried it out and at least two of the actions are not working.

* ReplyWithTemplate: JavaScript error: chrome://messenger/content/searchWidgets.js, line 94: TypeError: this.closest(...).querySelector(...) is null; can't access its "getTemplates" property
* Tag message: no tag menu opens to select tag

::: mailnews/base/search/content/searchWidgets.js
@@ +33,5 @@
> +    }
> +
> +    // propagating a pre-existing hack to make the tag get displayed correctly in the menulist
> +    // now that we"ve changed the tags for each menu list. We need to use the current selectedIndex
> +    // (if its defined) to handle the case where we were initialized with a filter action already.

hacky indeed. can you check if this is still needed?

@@ +47,5 @@
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +        <menulist class="ruleactionitem" flex="1">

still 4 space indention

@@ +54,5 @@
> +            <menuitem value="5" label="&highPriorityCmd.label;"></menuitem>
> +            <menuitem value="4" label="&normalPriorityCmd.label;"></menuitem>
> +            <menuitem value="3" label="&lowPriorityCmd.label;"></menuitem>
> +            <menuitem value="2" label="&lowestPriorityCmd.label;"></menuitem>
> +            </menupopup>

and this indention is off

@@ +140,5 @@
> +
> +    updateParentNode(this.closest(".ruleaction"));
> +  }
> +
> +  setPicker(aEvent) {

please make this event instead of aEvent while we're here

@@ +179,5 @@
> +
> +  _updateAttributes() {
> +    const type = this.getAttribute("type");
> +
> +

extra empty lines here

::: mailnews/base/search/content/searchWidgets.xml
@@ +327,5 @@
>        <xul:hbox class="ruleactiontype"
>                      flex="&filterActionTypeFlexValue;"/>
> +      <xul:ruleactiontarget-wrapper xbl:inherits="type=value"
> +                                class="ruleactiontarget"
> +                                flex="&filterActionTargetFlexValue;"/>

nit: strange alignment

@@ +368,5 @@
>          <body>
>            <![CDATA[
>              var filterActionStr;
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionItem = actionTarget.ruleactiontargetElement;

you can make this "let" while you're touching this line

@@ +451,5 @@
>              ChromeUtils.import("resource:///modules/MailUtils.jsm", this);
>              var filterActionString = this.getAttribute('value');
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionTargetLabel = actionTarget.ruleactiontargetElement &&
> +                                    actionTarget.ruleactiontargetElement.childNodes[0].value;

let

@@ +509,5 @@
>              var filterAction = aFilter.createAction();
>              var filterActionString = this.getAttribute('value');
>              filterAction.type = gFilterActionStrings.indexOf(filterActionString);
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionItem = actionTarget.ruleactiontargetElement;

let

@@ +530,5 @@
>                case nsMsgFilterAction.Custom:
>                  filterAction.customId = filterActionString;
>                  // fall through to set the value
>                default:
> +                if (actionItem && actionItem.childNodes.length)

for counts I think it's nicer to write length > 0

@@ +551,5 @@
>              aActionStrings.push({
>                label: document.getAnonymousNodes(this.mRuleActionType)[0].label,
>                argument: actionItem ?
> +                        (actionItem.childNodes[0].label ?
> +                         actionItem.childNodes[0].label : actionItem.childNodes[0].value) : ""

yuck. perhaps define a variable for this above so we can avoid the double ternary

Comment 19

[Arshad Khan [:arshad]]

Attachment: ruleactionbindingsfix.patch

Hey, the replywithtemplate option still doesn't work. There is no error log but when you run the replywithtemplate filter it says that filter has failed. Could you please confirm other options and this one as well on your side?

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9023594 [details] [diff] [review]
ruleactionbindingsfix.patch

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

Yeah something is broken. Can't save the new filter with this patch applied

JavaScript error: actionTarget.ruleactiontargetElement is undefined; can't access its "childNodes" property @ searchWidgets.xml:455

JavaScript error: chrome://messenger/content/searchWidgets.xml, line 525: TypeError: actionItem is undefined; can't access its "childNodes" property
 --> this line: filterAction.targetFolderUri = actionItem.childNodes[0].value;

                
                break;

Comment 21

[Arshad Khan [:arshad]]

Attachment: ruleactionbindingsfix.patch

Comment 22

[Arshad Khan [:arshad]]

Magnus, I tried ruleaction bindings on linux as well and I can not only save filters but run them as well. Could you please check again ? This time the reply-to filter also worked.

Comment 23

[Arshad Khan [:arshad]]

Attachment: Ubuntu_filter_Screenshot 2018-11-15 at 11.11.04 AM.png

Comment 24

[Arshad Khan [:arshad]]

Attachment: replyto_filter_error.png

In the screenshot you can see the error that occurs sometimes when running replyto filter.. You can also see it succeded for some emails by looking at the notification popup on top right..

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9025258 [details] [diff] [review]
ruleactionbindingsfix.patch

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

Yeah looks like this works now. r=mkmelin

Comment 26

[Arshad Khan [:arshad]]

(In reply to Magnus Melin [:mkmelin] from comment #25)
> Comment on attachment 9025258 [details] [diff] [review]
> ruleactionbindingsfix.patch
> 
> Review of attachment 9025258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah looks like this works now. r=mkmelin

could you please confirm that the error that I see in the screenshot is not because of the patch?

Comment 27

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 28

[Magnus Melin [:mkmelin]]

Well it says the server responded with "temporary system problem" so I don't think it's related.

Comment 29

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=01dbc444e9e0c4f6e0c2b6a87ee0914031ea05af

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c7cda013b0b4
Remove ruleactiontarget bindings. r=mkmelin

Comment 31

[:aceman]

It seems this is all busted. All filters that have "move/copy to", "reply with template" or "tag message" actions display blank menulists next to these actions in the filter editor. Even when those values are set and the display worked in the past. Can you please confirm?
Is it caused by this patch or a new change in m-c?

Comment 32

[Jorg K (CEST = GMT+2)]

Yes, pretty busted. If you have a filter "Move Message to" there is no folder to choose from. If you then select "Forward Message to" a box appears and the action changed to "Add Star".

I'll see whether can back it out. That turns out a little difficult due to changes on mailnews/base/search/content/searchWidgets.js/xml:
https://hg.mozilla.org/comm-central/log/tip/mailnews/base/search/content/searchWidgets.xml
https://hg.mozilla.org/comm-central/log/tip/mailnews/base/search/content/searchWidgets.js

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: backout.patch

Backout seems to fix the problem:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d57b0d35db291e3fb1b73fc09ff8bd5b6b355701

Aceman, can you please check as well.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1489172.patch

This is the patch that landed, but rebased after the backout. Arshad, you can start with this patch and fix the issues we found.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57b3ea524f1d
Backed out changeset c7cda013b0b4 for making the filter editor unusable. a=jorgk

Comment 36

[Arshad Khan [:arshad]]

Attachment: ruleactionbindingsfix.patch

Comment 37

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 38

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 39

[Arshad Khan [:arshad]]

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Tested this patch on mac, everything seems to work even on editing filter.

Comment 40

[:aceman]

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

OK, this one looks better, but I still get some empty menulists in action filters.
I'm not sure if they I caused by the errors I get in the console:
TypeError: actionItem is null; can't access its "childNodes" property, searchWidgets.xml:409:17
The line number differs depending on action type, there is also 413 and 425.

::: mailnews/base/search/content/searchWidgets.js
@@ +18,5 @@
> +
> +class MozRuleactiontargetTag extends MozXULElement {
> +  connectedCallback() {
> +    const menulist = document.createElement("menulist");
> +    const menuPopup = document.createElement("menupopup");

'const' on mutable elements??? How did this pass? Use 'let' here and elsewhere.

@@ +39,5 @@
> +}
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`

We are not allowed to use .innerHTML but this tag blob suddenly passes...

@@ +86,5 @@
> +
> +    this.appendChild(menulist);
> +
> +    document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "class", "ruleactiontype")
> +      .getTemplates(true, menulist);

Dot under dot.

@@ +110,5 @@
> +class MozRuleactiontargetFolder extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +      <menulist class="ruleactionitem folderMenuItem" flex="1" displayformat="verbose">
> +        <menupopup type="folder" mode="filing" class="menulist-menupopup" showRecent="true" recentLabel="&recentFolders.label;" showFileHereLabel="true"></menupopup>

Long line, split into each attribute on a line as we have in XUL files.

@@ +131,5 @@
> +
> +    // An account folder is not a move/copy target; show "Choose Folder".
> +    folder = folder.isServer ? null : folder;
> +
> +    // The menupopup constructor needs to finish first.

You copied this comment from where it made sense as there was a timeout. But here, how do you ensure the element is built? If just reaching this place means that, the comment is useless.

@@ +173,5 @@
> +    return elementName ? document.createElement(elementName) : null;
> +  }
> +
> +  _updateAttributes() {
> +    const type = this.getAttribute("type");

Only this 'const' is acceptable.

Comment 41

[Arshad Khan [:arshad]]

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +18,5 @@
> +
> +class MozRuleactiontargetTag extends MozXULElement {
> +  connectedCallback() {
> +    const menulist = document.createElement("menulist");
> +    const menuPopup = document.createElement("menupopup");

const mutablity will not be voided if we dont change binding of that particuclar element. I can change attributes/properties of an object or a js element even if it is a const variable. Obviously error will be flagged when I reassign the variable, in other words change the binding of that element. e.g., `const a = {}; a.val = 1;` is valid use of const in js. If i try to do `const a = {}; a = {val: 1}` then error will be flagged.

@@ +39,5 @@
> +}
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`

this is not innerHTML, parseXULToFragment uses parseXML method which is I suppose ok to use like https://searchfox.org/comm-central/source/calendar/base/modules/utils/calXMLUtils.jsm#139. For more - https://searchfox.org/comm-central/source/mozilla/toolkit/content/customElements.js#135

Comment 42

[Arshad Khan [:arshad]]

const immutability will not be voided*

Comment 43

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 44

[:aceman]

Comment on attachment 9028113 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Looks good now, thanks. No errors and no empty menulists.

However, when the "See execution order" link is clicked, I get
TypeError: actionItem is null; can't access its "childNodes" property, searchWidgets.xml:545:17

and no dialog with the action list appears. Possibly this method needs to be updated for the new location of the action values.

Comment 45

[Arshad Khan [:arshad]]

Attachment: ruleactiontargetbindingsunified.patch

Comment 46

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=245f37ad7629695abc2269d66ceec578fd6fd780

Comment 47

[:aceman]

Comment on attachment 9028812 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Thanks, seems to work nicely now.

The commit message needs a fix to r=aceman and also to mention the actiontarget bindings are not just removed but replaced by custom elements.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6e49a3ec00cd
Replace ruleactiontarget bindings with custom element. r=mkmelin,aceman