Closed Bug 1497093 Opened Last year Closed 11 months ago

Content tab find bar has two find bars

Categories

(Thunderbird :: Toolbars and Tabs, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: darktrojan, Assigned: arshad)

Details

Attachments

(1 file, 4 obsolete files)

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.
Daily from 14. sep is okay, the one from 15. sep shows the second findbar. Bug 1411707 landed in this time frame.
Attached patch findbar_fix.patch (obsolete) — Splinter Review
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.
Assignee: nobody → arshdkhn1
Attachment #9015347 - Flags: review?(richard.marti)
Attachment #9015347 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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.
Attachment #9015347 - Flags: review?(richard.marti)
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)
Attachment #9015347 - Flags: review?(mkmelin+mozilla)
Attached patch findbar_fix.patch (obsolete) — Splinter Review
Attachment #9015347 - Attachment is obsolete: true
Attachment #9015637 - Flags: review?(richard.marti)
Attachment #9015637 - Flags: review?(mkmelin+mozilla)
Attached patch findbar_fix.patch (obsolete) — Splinter Review
Attachment #9015637 - Attachment is obsolete: true
Attachment #9015637 - Flags: review?(richard.marti)
Attachment #9015637 - Flags: review?(mkmelin+mozilla)
Attachment #9015664 - Flags: review?(richard.marti)
Attachment #9015664 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9015664 - Flags: review?(richard.marti) → review+
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?
(It does work though)
Attached patch findbar_fix.patch (obsolete) — Splinter Review
Attachment #9015664 - Attachment is obsolete: true
Attachment #9015664 - Flags: review?(mkmelin+mozilla)
Attachment #9016583 - Flags: review?(mkmelin+mozilla)
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
Attachment #9016583 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9016583 - Attachment is obsolete: true
Attachment #9016610 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/283c828650e3
Remove the duplicate content of findbar. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.