1115183 - "Node was not found" error when going into appmenu->preferences->layout

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[:aceman]

Appmenu->preferences->layout. As soon as the final menupopup appears (with the "classic view" etc items) the error is shown in the error console:

Error: NotFoundError: Node was not found
Source File: chrome://messenger/content/mailCore.js
Line: 306

Comment 1

[Retro]

Attachment: Bug_1115183.patch

Hey aceman, I may have the solution for this. 

onViewToolbarsPopupShowing was being called when you hover over preferences but also when you hover over layout. The effect is that the function is called twice with the exact same arguments but the second time it's called insertBefore can't find the node provided it then throws an error and bails out early from the function.

Adding an if check to make sure the function is only called once seems to clear up the error and does not mess up any of the menu's as far as I can tell.

Comment 2

[:aceman]

Comment on attachment 8713950 [details] [diff] [review]
Bug_1115183.patch

Review of attachment 8713950 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it looks like your analysis is correct. The same function with same arguments (probably from the the same onpoupshowing attribute) is called when Preferences is opened and also when Layout is opened. That is kinda strange. But is seems the Seamonkey guys also found that out in similar situation. See
https://bugzilla.mozilla.org/show_bug.cgi?id=464653#c51 . Please fix it is in the same way as them, put the check inside onViewToolbarsPopupShowing().

::: mail/base/content/mailWindowOverlay.xul
@@ -1313,5 @@
>                     label="&preferencesCmdUnix.label;"
>  #endif
>                     oncommand="openOptionsDialog();">
>            <menupopup id="appmenu_customizeMenu"
> -                     onpopupshowing="onViewToolbarsPopupShowing(event, 'mail-toolbox');">

The patch does not apply. This line you remove is different in the tree:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1317
You probably have made several patches on this spot and this one is not a merge of them, just the last one.

Comment 3

[alta88]

This is a problem everywhere, but it usually doesn't cause a throw and so isn't visible (Bug 867393).  There, I thought the same solution as here was ok, but in fact that's hacky. Plus, the style now is to add event listeners to an element rather than on* handlers as xul attributes. Since then I had to fix this in the MoreLayouts extension as it touched the Layout menuitem; the better solution is to call event.stopImmediatePropagation() immediately in the onpopupshowing handler function, imo.

Comment 4

[Retro]

(In reply to alta88 from comment #3)
> the better solution is to call event.stopImmediatePropagation() immediately
> in the onpopupshowing handler function, imo.

alta88, yeah as far as bugs go this one is fairly benign. I was trying to implement the fix the way you mentioned. Immediately as I enter onViewToolbarsPopupShowing(aEvent, toolboxIds, aInsertPoint) I try calling aEvent.stopImmediatePropagation(); but that does not seem to do the trick. The function is still called when you click the submenu "Layout" 


However implementing the fix style that :aceman recommended does seem to do the trick.

Comment 5

[Retro]

Attachment: Bug_1115183_1.patch

By the way here's the correction, I mirrored the method used in https://bugzilla.mozilla.org/show_bug.cgi?id=464653#c51 and it does fix this error without having to add an if check to the mailWindowOverlay.xul file.

Comment 6

[:aceman]

Comment on attachment 8714057 [details] [diff] [review]
Bug_1115183_1.patch

Review of attachment 8714057 [details] [diff] [review]:
-----------------------------------------------------------------

Somehow the patch does not apply again. Please update your c-c tree (but not m-c as it is broken now).

::: mail/base/content/mailCore.js
@@ +255,5 @@
>    if (!Array.isArray(toolboxIds))
>      toolboxIds = [toolboxIds];
>  
>    let popup = aEvent.target;
> +  //Return early in the event that a submenu triggers this function.

// Return early in case a submenu triggered this function via bubbling of events.

@@ +258,5 @@
>    let popup = aEvent.target;
> +  //Return early in the event that a submenu triggers this function.
> +  if (popup != aEvent.currentTarget) {
> +    return;
> +  }

The style is to not use braces for one line if blocks in JS.

Also please move this check (together with popup = event.target) to the top of the function before the toolboxIds check.

Comment 7

[Retro]

Attachment: Bug_1115183_2.patch

Hm, just can't seem to catch a break with these patches messing up. I had the same problem earlier with a different bug I was working on. How about now? Does this on work?

Comment 8

[alta88]

(In reply to Retro from comment #4)
> (In reply to alta88 from comment #3)
> > the better solution is to call event.stopImmediatePropagation() immediately
> > in the onpopupshowing handler function, imo.
> 
> alta88, yeah as far as bugs go this one is fairly benign. I was trying to
> implement the fix the way you mentioned. Immediately as I enter
> onViewToolbarsPopupShowing(aEvent, toolboxIds, aInsertPoint) I try calling
> aEvent.stopImmediatePropagation(); but that does not seem to do the trick.
> The function is still called when you click the submenu "Layout" 
> 
> 
> However implementing the fix style that :aceman recommended does seem to do
> the trick.

No, stopImmediatePropagation() goes in InitViewLayoutStyleMenu(), to prevent the event bubbling to the ancestor menupopup's handler onViewToolbarsPopupShowing(), or any other ancestor.  So the patch method is inferior to catching the problem at the root.

Comment 9

[Retro]

Attachment: Bug_1151833.patch

My apologizes, figured alta88 meant I had to put that function call inside onViewToolbarsPopupShowing. Here's the fix that takes care of it inside InitViewLayoutStyleMenu in mailWindowOverlay.js.

Comment 10

[alta88]

@Retro - no worries, your work on this is appreciated.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8714183 [details] [diff] [review]
Bug_1151833.patch

Review of attachment 8714183 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good (but doesn't apply). I'll attach an unbitrotted version.

::: mail/base/content/mailWindowOverlay.js
@@ +274,5 @@
>      appmenuCharset.setAttribute("disabled", !msgWindow.mailCharacterSet);
>  }
>  
>  function InitViewLayoutStyleMenu(event)
> +{ 

nit: trailing space

@@ +275,5 @@
>  }
>  
>  function InitViewLayoutStyleMenu(event)
> +{ 
> +  //Prevent submenus from unnecessarily triggering onViewToolbarsPopupShowing via bubbling of events.

nit: we cap lines @ 80 ch

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: Bug_1151833.patch

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/6fd0470fb29c33dbdc0f1f1305b392e0b625bf3a
Bug 1115183 - Prevent submenus from unnecessarily triggering onViewToolbarsPopupShowing via bubbling of events. r=mkmelin