749690 - Add a findbar and support the zoom feature in the chat tab

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, enhancement)

Description

[Florian Quèze [:florian]]

Attachment: Patch

I was mostly interested in adding the findbar as we will want to use it for a follow up to bug 743235, but while looking at how the find commands work in other Thunderbird tabs, I noticed I could get the zoom working almost for free, and as we will want it too anyway, I put both in the same bug and patch.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 619094 [details] [diff] [review]
Patch

I just noticed something which you might want to fix in this patch. Ctrl-K doesn't focus the global search bar, even though the empty text says it does.

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -124,20 +124,93 @@ var chatTabType = {
>+  supportsCommand: function supportsCommand(aCommand, aTab) {

nit: I think we usually name these "ct_supportsCommand", just so that we can more easily see where they're coming from.

>+  doCommand: function isCommandEnabled(aCommand, aTab) {

And this function really shouldn't be named "isCommandEnabled".  ;)

>+  getBrowser: function getBrowser(aTab) {
>+    let panel = document.getElementById("conversationsDeck").selectedPanel;
>+    if (panel == document.getElementById("logDisplay")) {
>+      if (document.getElementById("logDisplayDeck").selectedPanel ==
>+          document.getElementById("logDisplayBrowserBox"))
>+        return document.getElementById("conv-log-browser");
>+    }
>+    else if (panel && panel.localName == "imconversation")
>+      return panel.browser;

As mentioned in a previous review, there should be {}s around this else clause (and the one a little further down).

Other than that I'm pretty happy.  r=me, and ui-r=me.

Later,
Blake.

Comment 2

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> Comment on attachment 619094 [details] [diff] [review]
> Patch
> 
> I just noticed something which you might want to fix in this patch. Ctrl-K
> doesn't focus the global search bar, even though the empty text says it does.

I have a patch ready for check-in addressing this specific issue in bug 749552.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch for checkin

Addresses the nits from comment 1.

Comment 4

[Florian Quèze [:florian]]

http://hg.mozilla.org/comm-central/rev/60c7b4af09cb

Comment 5

[Mark Banner (:standard8)]

Checked into aurora:

https://hg.mozilla.org/releases/comm-aurora/rev/cd0ce382db04