[de-XBL] Ensure custom elements clean up on DOM reinsertions (example: RSS mail-urlfield URLs opens twice due to stacked onclick event listener on layout change)
Categories
(Thunderbird :: Message Reader UI, defect)
Tracking
(thunderbird67 fixed, thunderbird68 fixed)
People
(Reporter: fernicar, Assigned: mkmelin)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
|
550.83 KB,
image/png
|
Details | |
|
8.38 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
Thunderbird v65.0b1 build 20181222205702.png
After updating MoreLayouts for 64, the problem is even more manifest: the tree-col image element is duplicated/reinserted on each layout change.
Anway, something like this needs to be implemented for custom elements generically to handle DOM reinsertions, otherwise there will be future pain. The code below fixes the bug here.
class MozMailUrlfield extends MozMailHeaderfield {
- constructor() {
- super();
- this.destroy = this.destroy.bind(this);
- }
connectedCallback() {
super.connectedCallback();
this.setAttribute("context", "copyUrlPopup");
this.classList.add("text-link", "headerValueUrl");
- this.addEventListener("click", (event) => {
-
if (event.button != 2) { -
openUILink(encodeURI(event.target.textContent), event); -
} - });
- this.addEventListener("click", this.onClickListener);
- this.addEventListener("unload", this.destroy);
- this._initialized = true;
- }
- onClickListener(event) {
- if (event.button != 2) {
-
openUILink(encodeURI(event.target.textContent), event); - }
- }
- destroy() {
- if (this._initialized) {
-
this._initialized = false; -
this.removeEventListener("click", this.onClickListener); -
this.removeEventListener("unload", this.destroy); - }
}
}
This will fix the duplicated treecol images. We need to review custom elements and keep DOM changes in mind...
class MozTreecolImage extends customElements.get("treecol") {
static get observedAttributes() {
return ["src"];
}
connectedCallback() {
+ if (this._initialized)
+ return;
+
this.image = document.createElement("image");
this.image.classList.add("treecol-icon");
this.appendChild(this.image);
this._updateAttributes();
+ this._initialized = true;
}
attributeChangedCallback() {
this._updateAttributes();
}
_updateAttributes() {
if (!this.isConnected || !this.image) {
(In reply to fernicar from comment #4)
(In reply to alta88 from comment #3)
that's pretty key (nice video too). this is a regression from Bug 1489748,
an onlick event listener is getting stacked on each layout change.I forgot to show/mention Ctrl+F and Ctrl+G in the video, it stop working as
soon as the layout is changed.
This is a regression from Bug 1499617, for some reason a .destroy() method was added to the Findbar, without realizing that on some layout changes the findbar.destroy() already runs in the custom element, so messagepane browser docShell is already destroyed, causing a throw. Further, the findbar needs to be reset after the layout change.
Please file a separate bug for this and reference this comment/regressing bug.
|
Reporter | |
Comment 10•6 years ago
|
||
(In reply to alta88 from comment #9)
This is a regression from Bug 1499617, for some reason a .destroy() method was added to the Findbar, without realizing that on some layout changes the findbar.destroy() already runs in the custom element, so messagepane browser docShell is already destroyed, causing a throw. Further, the findbar needs to be reset after the layout change.
Please file a separate bug for this and reference this comment/regressing bug.
Yes, here is the new link: Bug 1519416 comment #1
|
Assignee | |
Comment 11•6 years ago
|
||
|
|
Assignee | |
Comment 12•6 years ago
|
||
(In reply to alta88 from comment #6)
AFAIK, removing internal event listeners is not needed. They will go away automatically when the object goes away.
|
||
Comment 13•6 years ago
|
||
|
|
Assignee | |
Comment 14•6 years ago
|
||
The eventDialog.js try failure should be unrealated. The test works fine locally too with
make mozmill-one -C comm/calendar/test/mozmill SOLO_TEST=eventDialog/testAlarmDialog.js
|
|
Assignee | |
Updated•6 years ago
|
|
||
Comment 15•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/044047050f11
add some guards against adding children and event listeners more than once since connectedCallback() can run many times. r=arshad
|
||
Comment 16•6 years ago
|
||
It says "regression", does this need to go into the beta?
|
|
Assignee | |
Comment 17•6 years ago
|
||
Wouldn't say it's a super visible problem, but it might as well go into beta.
|
|
||
Updated•6 years ago
|
|
|
||
Comment 18•6 years ago
|
||
Description
•