Seen on Trunk 76.0a1 (2020-03-11) (64-bit); regressed by bug 440377; minor flaws and behaviour inconsistencies pre-existing. ## STR 1. Account Settings: Set up auto-CC/BCC on any sender account (identity1 = id1): ``` auto-CC: DisplayA <id1.cc@c.com> auto-BCC: DisplayB <id1.bcc@b.com> ``` And another one (identity2 = id2): ``` auto-CC: DisplayC <id2.cc@c.com> auto-BCC: [unchecked] ``` 2. Compose with identity1 (--> auto-CC/BCC correctly added from C++ code) 3. add more CCs / BCCs manually (or compose in reply to msg with many existing CCs, or use template with many CCs/BCCs), so it now looks like this: ``` CC: DisplayA <id1.cc@c.com>, ccX@c.com, ccY@c.com, ccZ@c.com BCC: DisplayB <id1.bcc@b.com> ``` 5. Change From sender identity to another account/identity (id2) with different or no auto-CC/BCC, and observe your CC/BCC recipient fields ## Actual Result ``` CC: id2.cc@c.com BCC: ``` - Boom! You just lost all of your CC/BCC recipients! ccX@c.com, ccY@c.com, ccZ@c.com are gone for good. Regression by changes of bug 440377 to awRemoveRecipients(). - Display name "DisplayC" of |DisplayC <id2.cc@c.com>| also gone (same for any identity you change to, including original identity1, because it's now controlled by JS code instead of C++ code). - Empty BCC field resulting from removing auto-BCC of previous identity (id1) is still shown, but shouldn't according to design ## Expected Result ``` CC: DisplayC <id2.cc@b.com>, ccX@c.com, ccY@c.com, ccZ@c.com ``` - Do not remove unrelated CCs/BCCs when changing identities - Do not remove auto-CC/BCC display names when changing identities - Remove empty CC/BCC fields after changing identity if the previous identity had Auto-CC/BBC ## Code review of awRemoveRecipients() after bug 440377 https://searchfox.org/comm-central/rev/9f02dd21f41f406480da1b0b3fa1d38626ed0492/mail/components/compose/content/addressingWidgetOverlay.js#317-354 ```` /** * Clear a specific recipient row if is visible and pills are present. This is * commonly used when loading a new identity. * * @param {Array} msgCompFields - The array containing all the recipient fields. * @param {string} recipientType - Which recipient needs to be cleared. * @param {Array} recipientsList - The array containing the old recipients. */ function awRemoveRecipients(msgCompFields, recipientType, recipientsList) { ```` - msgCompFields is an object, not an array. - recipientType only specifies where to look, not which recipients to delete. - recipientsList is a string, not an array, containing individual recipients to be deleted. - both msgCompFields and recipientsList parameters are unused except checking for their existence (red flag??) ```` let container = element.closest(".address-container"); for (let pill of container.querySelectorAll("mail-address-pill")) { pill.remove(); } ```` - the correct/original purpose of this function is to remove individual auto-CC/BCC/Reply-To recipients from previous identity upon identity change. The fact that recipients used to live in one row each does not warrant the deletion of an entire "row" aka recipient field and all of its contained pills in the new order of things. ```` // Reset the original input. let input = container.querySelector(`input[is="autocomplete-input"]`); input.value = ""; ```` - If user has started another unfinished recipient input (for which autocomplete didn't trigger), who are we to just delete that? ```` if (recipientType != "addr_to") { container.classList.add("hidden"); document.getElementById(recipientType).removeAttribute("collapsed"); } ```` - this attempts to remove the entire empty row (after the unwarranted removal of recipients), but has no effect because it's on the wrong element: .address-container cannot be hidden this way, only .address-row. What we want here is to hide rows if they were added by auto-CC/BCC of the previous identity AND they do not have any other pills nor user input. I'm not sure how this could escape review, QA, and tests, which is worrisome.
Bug 1623285 Comment 0 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Seen on Trunk 76.0a1 (2020-03-11) (64-bit); regressed by bug 440377; minor flaws and behaviour inconsistencies pre-existing. ## STR 1. Account Settings: Set up auto-CC/BCC on any sender account (identity1 = id1): ``` auto-CC: DisplayA <id1.cc@c.com> auto-BCC: DisplayB <id1.bcc@b.com> ``` And another one (identity2 = id2): ``` auto-CC: DisplayC <id2.cc@c.com> auto-BCC: [unchecked] ``` 2. Compose with identity1 (--> auto-CC/BCC correctly added from C++ code) 3. add more CCs / BCCs manually (or compose in reply to msg with many existing CCs, or use template with many CCs/BCCs), so it now looks like this: ``` CC: DisplayA <id1.cc@c.com>, ccX@c.com, ccY@c.com, ccZ@c.com BCC: DisplayB <id1.bcc@b.com> ``` 5. Change From sender identity to another account/identity (id2) with different or no auto-CC/BCC, and observe your CC/BCC recipient fields ## Actual Result ``` CC: id2.cc@c.com BCC: ``` - Boom! You just lost all of your CC/BCC recipients! ccX@c.com, ccY@c.com, ccZ@c.com are gone for good. Regression by changes of bug 440377 to awRemoveRecipients(). - Display name "DisplayC" of |DisplayC <id2.cc@c.com>| also gone (same for any identity you change to, including original identity1, because it's now controlled by JS code instead of C++ code). - Empty BCC field resulting from removing auto-BCC of previous identity (id1) is still shown, but shouldn't according to design ## Expected Result ``` CC: DisplayC <id2.cc@b.com>, ccX@c.com, ccY@c.com, ccZ@c.com ``` - Do not remove unrelated CCs/BCCs when changing identities - Do not remove auto-CC/BCC display names when changing identities - Remove empty CC/BCC fields after changing identity if the previous identity had Auto-CC/BBC ## Code review of awRemoveRecipients() after bug 440377 https://searchfox.org/comm-central/rev/9f02dd21f41f406480da1b0b3fa1d38626ed0492/mail/components/compose/content/addressingWidgetOverlay.js#317-354 ```` /** * Clear a specific recipient row if is visible and pills are present. This is * commonly used when loading a new identity. * * @param {Array} msgCompFields - The array containing all the recipient fields. * @param {string} recipientType - Which recipient needs to be cleared. * @param {Array} recipientsList - The array containing the old recipients. */ function awRemoveRecipients(msgCompFields, recipientType, recipientsList) { ```` - msgCompFields is an object, not an array. - recipientType only specifies where to look, not which recipients to delete. - recipientsList is a string, not an array, containing CSVs of individual recipients to be deleted. - both msgCompFields and recipientsList parameters are unused except checking for their existence (red flag??) ```` let container = element.closest(".address-container"); for (let pill of container.querySelectorAll("mail-address-pill")) { pill.remove(); } ```` - the correct/original purpose of this function is to remove individual auto-CC/BCC/Reply-To recipients from previous identity upon identity change. The fact that recipients used to live in one row each does not warrant the deletion of an entire "row" aka recipient field and all of its contained pills in the new order of things. ```` // Reset the original input. let input = container.querySelector(`input[is="autocomplete-input"]`); input.value = ""; ```` - If user has started another unfinished recipient input (for which autocomplete didn't trigger), who are we to just delete that? ```` if (recipientType != "addr_to") { container.classList.add("hidden"); document.getElementById(recipientType).removeAttribute("collapsed"); } ```` - this attempts to remove the entire empty row (after the unwarranted removal of recipients), but has no effect because it's on the wrong element: .address-container cannot be hidden this way, only .address-row. What we want here is to hide rows if they were added by auto-CC/BCC of the previous identity AND they do not have any other pills nor user input. I'm not sure how this could escape review, QA, and tests, which is worrisome.
Seen on Trunk 76.0a1 (2020-03-11) (64-bit); regressed by bug 440377; minor flaws and behaviour inconsistencies pre-existing. ## STR 1. Account Settings: Set up auto-CC/BCC on any sender account (identity1 = id1): ``` auto-CC: DisplayA <id1.cc@c.com> auto-BCC: DisplayB <id1.bcc@b.com> ``` And another one (identity2 = id2): ``` auto-CC: DisplayC <id2.cc@c.com> auto-BCC: [unchecked] ``` 2. Compose with identity1 (--> auto-CC/BCC correctly added from C++ code) 3. add more CCs / BCCs manually (or compose in reply to msg with many existing CCs, or use template with many CCs/BCCs), so it now looks like this: ``` CC: DisplayA <id1.cc@c.com>, ccX@c.com, ccY@c.com, ccZ@c.com BCC: DisplayB <id1.bcc@b.com> ``` 5. Change sender identity (From) to another account/identity (id2) with different or no auto-CC/BCC, and observe your CC/BCC recipient fields ## Actual Result ``` CC: id2.cc@c.com BCC: ``` - Boom! You just lost all of your CC/BCC recipients! ccX@c.com, ccY@c.com, ccZ@c.com are gone for good. Regression by changes of bug 440377 to awRemoveRecipients(). - Display name "DisplayC" of |DisplayC <id2.cc@c.com>| also gone (same for any identity you change to, including original identity1, because it's now controlled by JS code instead of C++ code). - Empty BCC field resulting from removing auto-BCC of previous identity (id1) is still shown, but shouldn't according to design ## Expected Result ``` CC: DisplayC <id2.cc@b.com>, ccX@c.com, ccY@c.com, ccZ@c.com ``` - Do not remove unrelated CCs/BCCs when changing identities - Do not remove auto-CC/BCC display names when changing identities - Remove empty CC/BCC fields after changing identity if the previous identity had Auto-CC/BBC ## Code review of awRemoveRecipients() after bug 440377 https://searchfox.org/comm-central/rev/9f02dd21f41f406480da1b0b3fa1d38626ed0492/mail/components/compose/content/addressingWidgetOverlay.js#317-354 ```` /** * Clear a specific recipient row if is visible and pills are present. This is * commonly used when loading a new identity. * * @param {Array} msgCompFields - The array containing all the recipient fields. * @param {string} recipientType - Which recipient needs to be cleared. * @param {Array} recipientsList - The array containing the old recipients. */ function awRemoveRecipients(msgCompFields, recipientType, recipientsList) { ```` - msgCompFields is an object, not an array. - recipientType only specifies where to look, not which recipients to delete. - recipientsList is a string, not an array, containing CSVs of individual recipients to be deleted. - both msgCompFields and recipientsList parameters are unused except checking for their existence (red flag??) ```` let container = element.closest(".address-container"); for (let pill of container.querySelectorAll("mail-address-pill")) { pill.remove(); } ```` - the correct/original purpose of this function is to remove individual auto-CC/BCC/Reply-To recipients from previous identity upon identity change. The fact that recipients used to live in one row each does not warrant the deletion of an entire "row" aka recipient field and all of its contained pills in the new order of things. ```` // Reset the original input. let input = container.querySelector(`input[is="autocomplete-input"]`); input.value = ""; ```` - If user has started another unfinished recipient input (for which autocomplete didn't trigger), who are we to just delete that? ```` if (recipientType != "addr_to") { container.classList.add("hidden"); document.getElementById(recipientType).removeAttribute("collapsed"); } ```` - this attempts to remove the entire empty row (after the unwarranted removal of recipients), but has no effect because it's on the wrong element: .address-container cannot be hidden this way, only .address-row. What we want here is to hide rows if they were added by auto-CC/BCC of the previous identity AND they do not have any other pills nor user input. I'm not sure how this could escape review, QA, and tests, which is worrisome.