Closed Bug 1519653 Opened 5 years ago Closed 5 years ago

Error this.selectedItems is undefined when opening the prefs

Categories

(Toolkit :: UI Widgets, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed

People

(Reporter: Paenglab, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.

Flags: needinfo?(arshdkhn1)
Attached patch 1519653-selectedItems.patch (obsolete) — Splinter Review

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.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(arshdkhn1)
Attachment #9036162 - Flags: review?(bgrinstead)
Attachment #9036162 - Flags: feedback?(richard.marti)
Component: Preferences → XUL Widgets
Product: Calendar → Toolkit

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 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
Attachment #9036162 - Flags: review?(bgrinstead)

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

The calendar categories list shows still this error.

Attachment #9036162 - Flags: feedback?(richard.marti)

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

Opening the prefs tab shows the error.

Attached patch 1519653-selectedItems.patch (v2) (obsolete) — Splinter Review

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).
Attachment #9036162 - Attachment is obsolete: true
Attachment #9036166 - Flags: review?(bgrinstead)
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.
Attachment #9036166 - Flags: review?(bgrinstead) → review+

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

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.

Flags: needinfo?(bgrinstead)
Keywords: regression

(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).

Flags: needinfo?(bgrinstead) → needinfo?(paolo.mozmail)

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

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

Attachment #9036166 - Flags: review?(paolo.mozmail)

(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.

Flags: needinfo?(paolo.mozmail)

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.

Attachment #9036166 - Flags: review?(paolo.mozmail) → review+

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

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

Attachment #9036618 - Flags: review?(paolo.mozmail)
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.
Attachment #9036618 - Flags: review?(paolo.mozmail) → review+
Priority: -- → P2
Attachment #9036166 - Attachment is obsolete: true
Attachment #9036618 - Attachment description: 1519653-selectedItems.patch (v3) → 1519653-selectedItems.patch (v3) - please change r=Paolo,bgrins
Keywords: checkin-needed

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41dbbcf3a61b
move initialisation of richlist properties to constructor. r=paolo

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: