Review of attachment 9121927 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much for fixing this; looks good to me. I'm not claiming to understand all the internals, but I have commented on some minor details. ::: mail/base/content/mailWidgets.js @@ -1773,5 @@ > return; > } > > - // Used to store the html:input element from where the pill was generated. > - this.originalInput; Would it be possible to retain this property (e.g. as rowInput) and hook it up with that new node-iterating function so that it updates correctly each time when called? Having the property to get the input element was useful imo, maybe it's not always about focus, addons might need this, and even for focus - as in my bug 1602431 - having the element property may sometimes make for nicer/simpler code. @@ +1907,5 @@ > /** > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { Useful function. I'd rename this to focusRowInput() - without "on" - because I think the technical term is "focus something" and focusOnRowInput() could be mistaken for a boolean (short for "focus *is* on rowInput). @@ +1908,5 @@ > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { > + if (!this.closest(".address-container")) { Can this ever happen? A pill without address-container? @@ +1961,5 @@ > break; > case "Delete": > case "Backspace": > if (!this.emailInput.value.trim() && !event.repeat) { > + this.focusOnRowInput(); focusRowInput(), here and below @@ +2391,5 @@ > */ > removePills(pill) { > for (let item of this.getAllSelectedPills()) { > + if (item == pill) { > + continue; Skip focused pill which is part of selection for deletion? Looks wrong to me. Bug 1602431 territory, unrelated to this bug.
Bug 1603166 Comment 8 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9121927 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much for fixing this; looks good to me. I'm not claiming to understand all the internals, but I have commented on some minor details. ::: mail/base/content/mailWidgets.js @@ -1773,5 @@ > return; > } > > - // Used to store the html:input element from where the pill was generated. > - this.originalInput; Would it be possible to retain this property (e.g. as rowInput) and hook it up with that new way of getting the rowInput - this.closest(".address-container").querySelector(...) - so that it updates correctly each time when called? Having the property to get the input element was useful imo, maybe it's not always about focus, addons might need this, and even for focus - as in my bug 1602431 - having the element property may sometimes make for nicer/simpler code. @@ +1907,5 @@ > /** > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { Useful function. I'd rename this to focusRowInput() - without "on" - because I think the technical term is "focus something" and focusOnRowInput() could be mistaken for a boolean (short for "focus *is* on rowInput). @@ +1908,5 @@ > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { > + if (!this.closest(".address-container")) { Can this ever happen? A pill without address-container? @@ +1961,5 @@ > break; > case "Delete": > case "Backspace": > if (!this.emailInput.value.trim() && !event.repeat) { > + this.focusOnRowInput(); focusRowInput(), here and below @@ +2391,5 @@ > */ > removePills(pill) { > for (let item of this.getAllSelectedPills()) { > + if (item == pill) { > + continue; Skip focused pill which is part of selection for deletion? Looks wrong to me. Bug 1602431 territory, unrelated to this bug.
Review of attachment 9121927 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much for fixing this; looks good to me. I'm not claiming to understand all the internals, but I have commented on some minor details. ::: mail/base/content/mailWidgets.js @@ -1773,5 @@ > return; > } > > - // Used to store the html:input element from where the pill was generated. > - this.originalInput; Would it be possible to retain this property (e.g. as rowInput) and hook it up with that new way of getting the rowInput - this.closest(".address-container").querySelector(...) - so that it updates correctly each time when called? Having the property to get the input element was useful imo, maybe it's not always about focus, addons might need this, and even for focus - as in my bug 1602431 - having the element property may sometimes make for nicer/simpler code. @@ +1907,5 @@ > /** > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { Useful function. I'd rename this to focusRowInput() - without "on" - because I think the technical term is "focus something" and focusOnRowInput() could be mistaken for a boolean (short for "focus *is* on rowInput). @@ +1908,5 @@ > + * Move the focus to the autocomplete input field of the current recipient > + * address row. > + */ > + focusOnRowInput() { > + if (!this.closest(".address-container")) { Can this ever happen? A pill without address-container? @@ +1961,5 @@ > break; > case "Delete": > case "Backspace": > if (!this.emailInput.value.trim() && !event.repeat) { > + this.focusOnRowInput(); focusRowInput(), here and below (several occurences) @@ +2391,5 @@ > */ > removePills(pill) { > for (let item of this.getAllSelectedPills()) { > + if (item == pill) { > + continue; Skip focused pill which is part of selection for deletion? Looks wrong to me. Bug 1602431 territory, unrelated to this bug.