Closed Bug 1476803 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: XUL Replacements, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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?
Attached patch ToolbarLoadingFailure-V1.diff (obsolete) — Splinter Review
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.
Attachment #8993268 - Flags: feedback?(geoff)
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
Flags: needinfo?(jorgk)
Blocks: 1476259
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 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.
Attachment #8993268 - Flags: feedback?(geoff) → feedback+
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.
Flags: needinfo?(jorgk)
Attachment #8993268 - Flags: review?(philipp)
Attachment #8993268 - Flags: review?(jorgk)
Comment on attachment 8993268 [details] [diff] [review]
ToolbarLoadingFailure-V1.diff

On offence intended, but six eyes will be enough for this :-)
Attachment #8993268 - Flags: review?(jorgk)
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?
Attachment #8993268 - Flags: review?(philipp) → review+
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.
Assignee: nobody → makemyday
Attachment #8993268 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8994056 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d3c51d57e07a
Avoid passing items to toolbar.xml if required attributes are not defined. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Rebased for you as a service ;-)
Target Milestone: --- → Thunderbird 63.0
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.
Attachment #8994056 - Flags: approval-comm-beta+
Attachment #8994056 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.