1519653 - Error this.selectedItems is undefined when opening the prefs

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Richard Marti (:Paenglab)]

When opening the prefs, the error this.selectedItems is undefined is shown.

Full error:

this.selectedItems is undefined richlistbox.js:248
get selectedCount chrome://global/content/elements/richlistbox.js:248
_fireOnSelect chrome://global/content/elements/richlistbox.js:280
clearSelection chrome://global/content/elements/richlistbox.js:536
updateCategoryList chrome://calendar/content/preferences/categories.js:81
init chrome://calendar/content/preferences/categories.js:54
<anonymous> chrome://lightning/content/messenger-overlay-preferences.js:37
loadScript resource:///modules/Overlays.jsm:477
load resource:///modules/Overlays.jsm:195
load resource:///modules/Overlays.jsm:40
observe chrome://messenger/content/parent/ext-legacy.js:138

This shows since the 4. January 2019. So it's likely bug 1472558 which regressed this.

Comment 1

[Jorg K (CEST = GMT+2)]

Also see bug 1517040 comment #35.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1519653-selectedItems.patch

With this patch, the cloudfile test in bug 1517040 passes when switching from xbl-richlistbox back to the normal one.

Brian, does this change make sense. I don't know how it can end up uninitialised.

Comment 3

[Brian Grinstead [:bgrins]]

Since the selectedItems property is set in connectedCallback() 0, I'm assuming we are running this getter either before DOMContentLoaded has fired and we finish delayConnectedCallback(), or the element is being created from JS and this getter is being accessed before it's inserted into the DOM. Do you know which it is?

As for this patch specifically, I think a better fix would be to set this.selectedItems = new ChromeNodeList(); inside the constructor and not connectedCallback, so other methods / properties using selectedItems would be get as well. If you still see the error with that change I'd really like to know what's going on.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 9036162 [details] [diff] [review]
1519653-selectedItems.patch

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=1519653#c3

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 9036162 [details] [diff] [review]
1519653-selectedItems.patch

The calendar categories list shows still this error.

Comment 6

[Jorg K (CEST = GMT+2)]

Richard, how do you get to this error? I see no categories, and I ran into bug 1519662.

Comment 7

[Richard Marti (:Paenglab)]

Opening the prefs tab shows the error.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1519653-selectedItems.patch (v2)

OK, with this patch

• the error opening the options tab goes away
• bug 1519662 goes away
• and we can use the normal richlistbox for the cloud file stuff and the test still passes (bug 1517040).

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 9036166 [details] [diff] [review]
1519653-selectedItems.patch (v2)

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

It's not clear to me if the previous XBL behavior (where selectedItems would get reset upon construction) was important. In particular: if an element is appended, some selected items are added, then the element is removed and reinserted, the XBL constructor would fired again causing `this.selectedItems = new ChromeNodeList();` to run.

My suspicion is that this isn't an important code path, and it's not clear to me if the list _should_ be reset in that case anyway. So as long as our tests pass this seems OK to me.

Comment 11

[Jorg K (CEST = GMT+2)]

Thanks, Brian. "So as long as our tests pass this seems OK to me": I haven't run any test on M-C, to here goes. Let me know if you want more platforms. Also, we're moving only this line this.selectedItems = new ChromeNodeList();, but there's more initialisation in connectedCallback(), like this._currentIndex = null;, etc. Should that move too?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e68aaa40316d089e34566ea73113a3dcc2c5a76

Comment 12

[Jorg K (CEST = GMT+2)]

Brian, the try came out green. So unless you want to check this on my platforms or we should move more code to the constructor, this would be ready for checkin. Please let me know.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #11)

Thanks, Brian. "So as long as our tests pass this seems OK to me": I haven't run any test on M-C, to here goes. Let me know if you want more platforms. Also, we're moving only this line this.selectedItems = new ChromeNodeList();, but there's more initialisation in connectedCallback(), like this._currentIndex = null;, etc. Should that move too?

Paolo, do you have an opinion about this? See comments 3 and 10 as well. I assume all of this initialization could move to the constructor as opposed to the connectedCallback with the same reasoning: https://searchfox.org/mozilla-central/rev/7d7aca6a935f0725f35050fdaa3dd7a1b05d8d38/toolkit/content/widgets/richlistbox.js#139-147.

It's not clear to me if the calls to this.setAttribute("allowevents", "true"); and this._refreshSelection(); can/should be done in the constructor, though. I get the feeling this isn't really a case we worry about (removing and re-appending a richlistitem).

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9036166 [details] [diff] [review]
1519653-selectedItems.patch (v2)

Gentle ping, we'd like to move this forward.

Comment 15

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #13)

I assume all of this initialization could move to the constructor as opposed to the connectedCallback with the same reasoning

Hm, in fact the most correct location probably depends on the specific variable. I don't think the behavior of "richlistbox" when detached from the document has ever been a design consideration, so I'd say that either one of keeping the patch as it is now or moving all the variables to the constructor works for me.

It's not clear to me if the calls to this.setAttribute("allowevents", "true"); and this._refreshSelection(); can/should be done in the constructor, though. I get the feeling this isn't really a case we worry about (removing and re-appending a richlistitem).

I'd keep these two calls in the connectedCallback.

Comment 16

[:Paolo Amadini]

Comment on attachment 9036166 [details] [diff] [review]
1519653-selectedItems.patch (v2)

Either this or moving the initialization of the other properties here works for me.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1519653-selectedItems.patch (v3) - please change r=Paolo,bgrins

As an alternative, this moves the initialisation of all object properties to the constructor.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2635a0d89a30c943a172ec9d081d355d07de66d

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 9036618 [details] [diff] [review]
1519653-selectedItems.patch (v3) - please change r=Paolo,bgrins

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

Rubber stamping as per https://bugzilla.mozilla.org/show_bug.cgi?id=1519653#c16.

I'd slightly prefer this patch over the other, if try is green for m-c.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41dbbcf3a61b
move initialisation of richlist properties to constructor. r=paolo

Comment 20

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/41dbbcf3a61b