1476803 - Overlay loader tries to set a toolbar item with passing an id

Status: RESOLVED FIXED

(MailNews Core :: XUL Replacements, defect)

Description

[[:MakeMyDay]]

When opening an existing event or creating in window mode in Lightning, the error console shows:

> Empty string passed to getElementById(). toolbar.xml:324:27

In the debugger you can see, that after adding the expected (and thereafter visible) menu items an additional item is tried to be added by passing in an empty id.

However, the currentSet in the xulstore.json has no empty items in the list:

>"chrome://calendar/content/calendar-event-dialog.xul":{
>  "calendar-event-dialog":{
>    "screenX":"407",
>    "screenY":"111",
>    "width":"684",
>    "height":"516"
>  },
>  "calendar-task-dialog":{
>    "id":"calendar-task-dialog"
>  },
>  "event-toolbox":{
>    "mode":"full"
>  },
>  "event-toolbar":{
>    "currentset":"button-saveandclose,button-attendees,button-privacy,button-url,button-delete,button-timezones"
>  }
>}

Resetting to defaultSet doesn't change the behaviour.

call stack for all the loaded items:
_getToolbarItem toolbar.xml:303
set_currentSet toolbar.xml:267
_init toolbar.xml:175
listener toolbar.xml:138

call stack for the empty id:
_getToolbarItem toolbar.xml:303
set_currentSet toolbar.xml:267
load Overlays.jsm:173
load Overlays.jsm:44
observe ext-legacy.js:100

Since we have two toolbars (one for the menu [1] and one for the icons [2] in calendar-event-dialog.xul, the menu toolbar gets maybe pushed at [3] too, for which no defaultset can be resolved at [4]?


[1] https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.xul#269
[2] https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.xul#566
[3] https://searchfox.org/comm-central/source/common/src/Overlays.jsm#283
[4] https://searchfox.org/comm-central/source/common/src/Overlays.jsm#168

Comment 1

[Geoff Lankow (:darktrojan)]

Hmm, I'm not seeing this, although I think I have seen it before. Could you add a debug line to Overlays.jsm and find out exactly what it's trying to set to where?

Comment 2

[[:MakeMyDay]]

Attachment: ToolbarLoadingFailure-V1.diff

It is as I expected the menubar, which neither has an id nor a default set. The attached patch deals with the issue by preventing to set currentSet if no attribut |defaultset| exists. I also wrapper to chekc for existance of the |id| attribute to save the xulstore lookup if not present.

I saw no side effects with this patch applied and the error message vanished.

Comment 3

[[:MakeMyDay]]

And a technical requestion: there aren't yet defined owners/peers for the new submodule XUL Replacements at [1] (module assignments to all products seem to be a little outdated these days) - I assume reviews would be send to the module owner/peers until that is fixed?

[1] https://wiki.mozilla.org/Modules/MailNews_Core

Comment 4

[Jorg K (CEST = GMT+2)]

That's always been a problem in Mailnews. Send reviews to Aceman, Magnus, Philipp, Geoff or myself. The person with the best skill set can do it.

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 8993268 [details] [diff] [review]
ToolbarLoadingFailure-V1.diff

Looks good. I was going to change the code to just ignore toolbars with type="menubar", and I think I still might, but I guess there could be other toolbars with no defaultset.

Comment 6

[[:MakeMyDay]]

Comment on attachment 8993268 [details] [diff] [review]
ToolbarLoadingFailure-V1.diff

Ok, then let's go with that unless Geoff likes to add the forementioned check as well.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993268 [details] [diff] [review]
ToolbarLoadingFailure-V1.diff

On offence intended, but six eyes will be enough for this :-)

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8993268 [details] [diff] [review]
ToolbarLoadingFailure-V1.diff

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

r+wc

::: common/src/Overlays.jsm
@@ +170,5 @@
> +        let currentset = xulStoreService.getValue(this.location, bar.id, "currentset");
> +        if (currentset) {
> +          bar.currentSet = currentset;
> +        } else if (bar.getAttribute("defaultset")) {
> +          bar.currentSet = bar.getAttribute("defaultset");

I think we would still need to set the currentSet = defaultset in even if the bar has no id, right?

Comment 9

[[:MakeMyDay]]

Attachment: ToolbarLoadingFailure-V2.diff

Thanks. I removed the check for the bar.id - xulStoreService would return an empty string in case it runs into a problem with getting the value, so this should be ok.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d3c51d57e07a
Avoid passing items to toolbar.xml if required attributes are not defined. r=philipp

Comment 11

[Jorg K (CEST = GMT+2)]

Rebased for you as a service ;-)

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8994056 [details] [diff] [review]
ToolbarLoadingFailure-V2.diff

I guess that will need to be uplifted with the rest of the overlay things although it's not clear whether we'll ship a TB 62 with a non-functional calendar.