Closed
Bug 1489172
Opened 6 years ago
Closed 6 years ago
[de-xbl] Remove ruleactiontarget bindings.
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 65.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(3 files, 21 obsolete files)
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Assignee | ||
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Remove ruleactiontarget-base binding. → [de-xbl] Remove ruleactiontarget bindings.
Assignee | ||
Comment 5•6 years ago
|
||
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
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #9018924 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018925 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018926 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018930 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018932 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018934 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9018933 -
Flags: review?(mkmelin+mozilla)
Comment 13•6 years ago
|
||
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•6 years ago
|
||
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", ...
Assignee | ||
Comment 15•6 years ago
|
||
addressed magnus's comments.
Attachment #9021805 -
Attachment is obsolete: true
Attachment #9021805 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aeb1cab24c568a3d4e978e905473e835b76241d8
Attachment #9018924 -
Attachment is obsolete: true
Attachment #9018925 -
Attachment is obsolete: true
Attachment #9018926 -
Attachment is obsolete: true
Attachment #9018930 -
Attachment is obsolete: true
Attachment #9018932 -
Attachment is obsolete: true
Attachment #9018933 -
Attachment is obsolete: true
Attachment #9018934 -
Attachment is obsolete: true
Attachment #9022176 -
Attachment is obsolete: true
Attachment #9018924 -
Flags: review?(mkmelin+mozilla)
Attachment #9018925 -
Flags: review?(mkmelin+mozilla)
Attachment #9018926 -
Flags: review?(mkmelin+mozilla)
Attachment #9018930 -
Flags: review?(mkmelin+mozilla)
Attachment #9018932 -
Flags: review?(mkmelin+mozilla)
Attachment #9018933 -
Flags: review?(mkmelin+mozilla)
Attachment #9018934 -
Flags: review?(mkmelin+mozilla)
Attachment #9022259 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #9022259 -
Attachment is obsolete: true
Attachment #9022259 -
Flags: review?(mkmelin+mozilla)
Attachment #9022368 -
Flags: review?(mkmelin+mozilla)
Comment 18•6 years ago
|
||
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
Attachment #9022368 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 19•6 years ago
|
||
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?
Attachment #9022368 -
Attachment is obsolete: true
Attachment #9023594 -
Flags: review?(mkmelin+mozilla)
Comment 20•6 years ago
|
||
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;
Attachment #9023594 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 22•6 years ago
|
||
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.
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
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•6 years ago
|
||
Comment on attachment 9025258 [details] [diff] [review] ruleactionbindingsfix.patch Review of attachment 9025258 [details] [diff] [review]: ----------------------------------------------------------------- Yeah looks like this works now. r=mkmelin
Attachment #9025258 -
Flags: review+
Assignee | ||
Comment 26•6 years ago
|
||
(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?
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #9025258 -
Attachment is obsolete: true
Attachment #9025309 -
Flags: review+
Comment 28•6 years ago
|
||
Well it says the server responded with "temporary system problem" so I don't think it's related.
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 29•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=01dbc444e9e0c4f6e0c2b6a87ee0914031ea05af
Comment 30•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/c7cda013b0b4 Remove ruleactiontarget bindings. r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 65.0
![]() |
||
Comment 31•6 years ago
|
||
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?
Flags: needinfo?(jorgk)
Comment 32•6 years ago
|
||
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
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)
Flags: needinfo?(arshdkhn1)
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 33•6 years ago
|
||
Backout seems to fix the problem: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d57b0d35db291e3fb1b73fc09ff8bd5b6b355701 Aceman, can you please check as well.
Flags: needinfo?(acelists)
Comment 34•6 years ago
|
||
This is the patch that landed, but rebased after the backout. Arshad, you can start with this patch and fix the issues we found.
Flags: needinfo?(acelists)
Updated•6 years ago
|
Keywords: leave-open
Comment 35•6 years ago
|
||
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
Assignee | ||
Comment 36•6 years ago
|
||
Attachment #9027353 -
Attachment is obsolete: true
Attachment #9027357 -
Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Assignee | ||
Comment 37•6 years ago
|
||
Attachment #9025309 -
Attachment is obsolete: true
Attachment #9027458 -
Attachment is obsolete: true
Assignee | ||
Comment 39•6 years ago
|
||
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.
Attachment #9027460 -
Flags: review?(acelists)
Updated•6 years ago
|
Flags: needinfo?(mkmelin+mozilla)
![]() |
||
Comment 40•6 years ago
|
||
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.
Attachment #9027460 -
Flags: feedback+
Assignee | ||
Comment 41•6 years ago
|
||
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
Assignee | ||
Comment 43•6 years ago
|
||
Attachment #9027460 -
Attachment is obsolete: true
Attachment #9027460 -
Flags: review?(acelists)
Attachment #9028113 -
Flags: review?(acelists)
Attachment #9028113 -
Flags: feedback+
![]() |
||
Comment 44•6 years ago
|
||
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.
Attachment #9028113 -
Flags: feedback+
Assignee | ||
Comment 45•6 years ago
|
||
Attachment #9028113 -
Attachment is obsolete: true
Attachment #9028113 -
Flags: review?(acelists)
Attachment #9028812 -
Flags: review?(acelists)
Attachment #9028812 -
Flags: feedback+
Assignee | ||
Comment 46•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=245f37ad7629695abc2269d66ceec578fd6fd780
![]() |
||
Comment 47•6 years ago
|
||
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.
Attachment #9028812 -
Flags: review?(acelists)
Attachment #9028812 -
Flags: review+
Attachment #9028812 -
Flags: feedback+
Comment 48•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/6e49a3ec00cd Replace ruleactiontarget bindings with custom element. r=mkmelin,aceman
Updated•6 years ago
|
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•