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 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 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 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 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", ...

addressed magnus's comments.

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

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 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;

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.

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 on attachment 9025258 [details] [diff] [review] ruleactionbindingsfix.patch Review of attachment 9025258 [details] [diff] [review]: ----------------------------------------------------------------- Yeah looks like this works now. r=mkmelin

(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?

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

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/c7cda013b0b4 Remove ruleactiontarget bindings. r=mkmelin

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?

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

Backout seems to fix the problem: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d57b0d35db291e3fb1b73fc09ff8bd5b6b355701 Aceman, can you please check as well.

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

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

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 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 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

const immutability will not be voided*

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 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.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/6e49a3ec00cd Replace ruleactiontarget bindings with custom element. r=mkmelin,aceman