Closed Bug 1517818 Opened 8 months ago Closed 5 months ago

[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)

defect
Not set

Tracking

(thunderbird67 fixed, thunderbird68 fixed)

RESOLVED FIXED
Thunderbird 68.0
Tracking Status
thunderbird67 --- fixed
thunderbird68 --- fixed

People

(Reporter: fernicar, Assigned: mkmelin)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

One (1) click on the source URL (a link provided by Thunderbird graphic user interface) of any RSS news  


Actual results:

opens twice (2 identical tabs) in Firefox if Firefox is open, or random crash Firefox there was no instance of Firefox running.

Thunderbird v65.0b1 build 20181222205702, Windows 10, Firefox 64 x64bits
Ctrl+F and Ctrl+G stopped working, all bugs appeared at the same time 3 updates ago.


Expected results:

open only one tab in Firefox.
I can't reproduce this on win7 and clean Fx66 and Tb66, it works as expected, along with ctrl-f and g. Make sure no extension is a variable.
Status: UNCONFIRMED → RESOLVED
Closed: 8 months ago
Resolution: --- → WORKSFORME
(In reply to alta88 from comment #1)
> I can't reproduce this on win7 and clean Fx66 and Tb66, it works as
> expected, along with ctrl-f and g. Make sure no extension is a variable.

Thanks for your testing, I found out you need is to change the layout:
Menu > Options > Layout > Vertical View
Tested in a clean installed VirtualBox Win7 x86, and Thunderbird Beta Version 65.0b1 and RSS feed:
http://blog.mozilla.org/

Tested restarting in safe mode (no extensions available), same bug.
I can't test in Thunderbird 66, where did you get it?. The current download Thunderbird Beta button link is:
https://download.mozilla.org/?product=thunderbird-65.0b1-SSL&os=win&lang=en-US

Here is the video recording of the test in windows 7 32bis and internet explorer.
https://youtu.be/POT2jLd_Jb0
(In reply to fernicar from comment #2)
> (In reply to alta88 from comment #1)
> > I can't reproduce this on win7 and clean Fx66 and Tb66, it works as
> > expected, along with ctrl-f and g. Make sure no extension is a variable.
> 
> Thanks for your testing, I found out you need is to change the layout:
> Menu > Options > Layout > Vertical View

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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(mkmelin+mozilla)
Resolution: WORKSFORME → ---
(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.
there are a lot of issues after a layout change without a restart. they have all been fixed (not this new event listener stacking one) in the MoreLayouts extension, but I have not yet updated it for Tb 64+.

https://bitbucket.org/alta8888/morelayouts
Blocks: 1489748
Keywords: regression
Summary: RSS source URLs from Thunderbird GUI opens twice → [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)

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);
    
  • }
    }
    }
Flags: needinfo?(arshdkhn1)
Attached file code (obsolete) —

ouch, since when is markdown a thing in bugzilla.

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.

(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

Attached patch bug1517818_dom_reinsert.patch (obsolete) — Splinter Review
Assignee: nobody → mkmelin+mozilla
Attachment #9034799 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(arshdkhn1)
Attachment #9053588 - Flags: review?(arshdkhn1)

(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 on attachment 9053588 [details] [diff] [review]
bug1517818_dom_reinsert.patch

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

::: mail/base/content/mailWidgets.js
@@ +38,5 @@
>          openUILink(encodeURI(event.target.textContent), event);
>        }
>      });
>    }
> +  connectedCallback() {

need empty line before connectedCB
Attachment #9053588 - Flags: review?(arshdkhn1) → review+

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

Attachment #9053588 - Attachment is obsolete: true
Attachment #9054178 - Flags: review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 8 months ago5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

It says "regression", does this need to go into the beta?

Flags: needinfo?(mkmelin+mozilla)
Target Milestone: --- → Thunderbird 68.0

Wouldn't say it's a super visible problem, but it might as well go into beta.

Flags: needinfo?(mkmelin+mozilla)
Attachment #9054178 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.