1497093 - Content tab find bar has two find bars

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Open a content tab, e.g. by clicking on Help > Troubleshooting Information.
Press Ctrl-F to bring up the find bar. There's a second find bar next to the first.

Comment 1

[Richard Marti (:Paenglab)]

Daily from 14. sep is okay, the one from 15. sep shows the second findbar. Bug 1411707 landed in this time frame.

Comment 2

[Arshad Khan [:arshad]]

Attachment: findbar_fix.patch

Findbar custom element's content are added connectedCallback so when the content tab node is cloned, clone node doesn't have parent and the state of existing findbar custom element is that it has content of connectedState. So when the clone node is again attached to dom, the connectedCallback is fired and duplicated content is added. That's the reason why we are seeing double findbar.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9015347 [details] [diff] [review]
findbar_fix.patch

This is a good start. The problem is, it loads the findbar.css only partially. Here on Windows 10 the bar has no background colour. And when I enter something that doesn't exists in the page, the textbox doesn't turn to red.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9015347 [details] [diff] [review]
findbar_fix.patch

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

Please reflag when ready (Richard's comments addressed)

Comment 5

[Arshad Khan [:arshad]]

Attachment: findbar_fix.patch

Comment 6

[Arshad Khan [:arshad]]

Attachment: findbar_fix.patch

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9015664 [details] [diff] [review]
findbar_fix.patch

Thanks. This works now.

On Windows, on content tabs the findbar has transparent background. This isn't a issue of this bug, it happens also on TB 60. I'll file a new bug for this.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9015664 [details] [diff] [review]
findbar_fix.patch

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

::: mail/base/content/specialTabs.js
@@ +695,5 @@
>        let clone = document.getElementById("contentTab").firstChild.cloneNode(true);
>  
> +      const findbar = document.createElement("findbar");
> +      // Findbar having browserid, are only attached result listener
> +      findbar.setAttribute("browserid", "dummybrowsercontent");

The comment is not clear, grammar derailed somehow.

You're changing from dummycontentbrowser -> dummybrowsercontent. Why?

Comment 9

[Magnus Melin [:mkmelin]]

(It does work though)

Comment 10

[Arshad Khan [:arshad]]

Attachment: findbar_fix.patch

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9016583 [details] [diff] [review]
findbar_fix.patch

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

LGTM, r=mkmelin

::: mail/base/content/specialTabs.js
@@ +695,5 @@
>        let clone = document.getElementById("contentTab").firstChild.cloneNode(true);
>  
> +      const findbar = document.createElement("findbar");
> +      // Adding browserid to findbar so that browser property can be set
> +      // in findbar custom element

nit: add period at the end of the sentence

Comment 12

[Arshad Khan [:arshad]]

Attachment: findbar_fix_1.patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/283c828650e3
Remove the duplicate content of findbar. r=mkmelin