Review of attachment 9121409 [details] [diff] [review]: ----------------------------------------------------------------- From a UI point of view this works as expected, thanks for taking care of this. I left a little comment for a portion of code, but of course let's wait for Magnus' review. Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header `# User Thomas Düllmann <bugzilla2007@duellmann24.net>` ::: mail/base/content/mailWidgets.js @@ +2057,5 @@ > + } > + } > + } > + return null; > + } This is a bit verbose and can be simplified by removing the `else` statements. ``` if (siblingsType == "next") { // Check for next siblings. let element = this.nextElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.nextElementSibling; } } // Check for previous siblings. let element = this.previousElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.previousElementSibling; } return null; ```
Bug 1602431 Comment 14 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 9121409 [details] [diff] [review]: ----------------------------------------------------------------- From a UI point of view this works as expected, thanks for taking care of this. I left a little comment for a portion of code, but of course let's wait for Magnus' review. Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header `# User Thomas Düllmann <bugzilla2007[spam]>` ::: mail/base/content/mailWidgets.js @@ +2057,5 @@ > + } > + } > + } > + return null; > + } This is a bit verbose and can be simplified by removing the `else` statements. ``` if (siblingsType == "next") { // Check for next siblings. let element = this.nextElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.nextElementSibling; } } // Check for previous siblings. let element = this.previousElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.previousElementSibling; } return null; ```
Review of attachment 9121409 [details] [diff] [review]: ----------------------------------------------------------------- From a UI point of view this works as expected, thanks for taking care of this. I left a little comment for a portion of code, but of course let's wait for Magnus' review. Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header `# User Thomas Düllmann <bugzilla2007**[snip]**>` ::: mail/base/content/mailWidgets.js @@ +2057,5 @@ > + } > + } > + } > + return null; > + } This is a bit verbose and can be simplified by removing the `else` statements. ``` if (siblingsType == "next") { // Check for next siblings. let element = this.nextElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.nextElementSibling; } } // Check for previous siblings. let element = this.previousElementSibling; while (element) { if (!element.hasAttribute("selected")) { return element } element = element.previousElementSibling; } return null; ```