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

Steps to reproduce:

First: Menu > Options > Layout > Vertical View
then: read an email or RSS news
and: press Ctrl+F or Menu > Find > Find in this message
Or: press Ctrl+G or Menu > Find > Find next

Actual results:

Nothing happen when pressing Ctrl+F or Ctrl+G

Expected results:

An extra bar with text field should appear below the reading message for the current email and RSS news, and it should be auto-filled with the current highlighted text from the email/RSS news if any text is selected.

(In reply to alta88 from Bug 1517818 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.

If I got it right, I've been requested to copy this comment and paste here, let me know.

I've had a look into this, and it's related to, but not caused by the destroy call in question. If you take that line away again the problem still exists. At this point I think it's a regression from bug 1411707.

What's happening is the findbar is destroyed as it's removed from the DOM (to be moved) and not reinitialised properly when inserted in the new place. I'll investigate why this worked when the findbar was an XBL binding instead of a custom element.

I removed that line in my extension, and rather than throwing on null _docShell on the destroy() in finder, there was a throw after layout change on ctrl-F with null _finder. I don't think that destroy() call belongs in updateMailPaneConfig(), everything should be totally handled in the custom element. Then again, Fx isn't instantiating <findbar> exactly in the way Tb is (ie non lazily). The fix in the extension was to try catch the destroy() and after DOM change do this:

// Reinit the findbar browser.
let findbar = this.e("FindToolbar");
findbar.browser;
findbar.browser._finder = null;
findbar.browser.finder;

What's more, it's an omission of MozXULElement base class not to automatically add an unload listener, to call a destroy, which should remove event listeners, if any. And allowing multiple identical event listeners is a regression to 10 yrs ago, back when DOM changes required a consumer to re-add all attributes etc etc. In Tb, we have not implemented a single custom element with destroy() or considered what happens when connectedCallback() runs again on a DOM reinsertion. Hence Bug 1517818. In (a small amount of) fairness though, this basic stuff belongs in the base class.

Arshad, could you please look into getting this fixed? It happens anywhere a browser and findbar are removed from the document (usually by removing an ancestor). I changed what happens when changing the layout to stop a test failing, so that's probably not the best place to start, but you can trigger the error by opening and closing the preferences tab.

The error message is:

TypeError: this._docShell is null at destroy@resource://gre/modules/Finder.jsm:63:5

(In reply to Geoff Lankow (:darktrojan) from comment #4)

Arshad, could you please look into getting this fixed? It happens anywhere a browser and findbar are removed from the document (usually by removing an ancestor). I changed what happens when changing the layout to stop a test failing, so that's probably not the best place to start, but you can trigger the error by opening and closing the preferences tab.

The error message is:

TypeError: this._docShell is null at destroy@resource://gre/modules/Finder.jsm:63:5

Hey is this issue still there? I tried to copy the steps but I can see a findbar when cntrl+f is pressed and I can also move from one result to next one using cntrl+g. Here's a video - https://youtu.be/A0rHXPuA83o

It does seem to work again, but not without the line I added in bug 1499617, which isn't ideal but I guess it can stay. I wonder what's changed that caused this bug to disappear.

(In reply to Geoff Lankow (:darktrojan) from comment #6)

It does seem to work again, but not without the line I added in bug 1499617, which isn't ideal but I guess it can stay. I wonder what's changed that caused this bug to disappear.

One similar bug was handled by Magnus. I dont remember the bug but it also had some layout specific issues.

Possibly bug 1517818.