982752 - Error: childNodes.lastChild is undefined Source file: chrome://editor/content/editor.js Line: 2756

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Philip Chee]

Thu Mar 13 2014 01:24:13
Error: An error occurred updating the cmd_updateStructToolbar
command: [Exception... "[JavaScript Error: "childNodes.lastChild is undefined"
{file: "chrome://editor/content/editor.js" line: 2756}]
'[JavaScript Error: "childNodes.lastChild is undefined" {file: "chrome://editor/content/editor.js" line: 2756}]' when calling method: [nsIControllerCommand::isCommandEnabled]"
nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"
location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 78"  data: yes]
Source file: chrome://global/content/globalOverlay.js
Line: 84
 ----------
Thu Mar 13 2014 01:24:13
Error: childNodes.lastChild is undefined
Source file: chrome://editor/content/editor.js
Line: 2756

http://hg.mozilla.org/comm-central/diff/875250f407aa/editor/ui/composer/content/editor.js#l1.55

>    while (childNodes.length > 1) {
>      // Remove back to front so there's less moving about.
> -    toolbar.removeChild(childNodes.item(childNodes.length - 2));
> +    childNodes.lastChild.previousSibling.remove();
childNodes is a nodeList, hence it does not have a property called lastChild. The following works for me:

  var toolbar = document.getElementById("structToolbar");
  var childNodes = toolbar.childNodes;
  while (childNodes.length > 1) {
    childNodes.item(childNodes.length -2 ).remove();

Comment 1

[Philip Chee]

>   var toolbar = document.getElementById("structToolbar");
>   var childNodes = toolbar.childNodes;
>   while (childNodes.length > 1) {
>     childNodes.item(childNodes.length -2 ).remove();

This is a live nodeList, so every time you remove a node the nodeList has to be recomputed. Instead, you could use querySelectorAll which returns a non-live nodeList, bonus point for excluding the last child which is a label.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v1

Comment 3

[Ian Neal]

Comment on attachment 8390571 [details] [diff] [review]
patch, v1

As Philip suggested, please use querySelectorAll.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v2

Comment 5

[Ian Neal]

Comment on attachment 8391940 [details] [diff] [review]
patch, v2

>+++ b/editor/ui/composer/content/editor.js
>@@ -2742,22 +2742,20 @@ function UpdateStructToolbar()
>       oneElementSelected == gLastFocusNodeWasSelected)
>     return;
> 
>   gLastFocusNode = element;
>   gLastFocusNodeWasSelected = mixed.oneElementSelected;
> 
>   var toolbar = document.getElementById("structToolbar");
>   if (!toolbar) return;
>+  // We need to leave the <label> to flex the buttons to the left.
>+  let toolbarButtonsObsolete = toolbar.querySelectorAll("toolbarbutton");
>+  for (let i = toolbarButtonsObsolete.length - 1; i >= 0; i--) {
>+    toolbarButtonsObsolete.item(i).remove();
As it is now a static list, the nodes do not need to be removed in any particular order, so you could do something like:
for (let node of toolbarButtonsObsolete)
  node.remove()

You might even be able to remove toolbarButtonsObsolete completely and just use toolbar.querySelectorAll("toolbarbutton") in the for statement.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v3

Comment 7

[Ian Neal]

Comment on attachment 8392403 [details] [diff] [review]
patch, v3

r=me
The { and } are not needed though.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v4, r=IanN

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c7ddf29222cd