Closed Bug 1569399 Opened 5 years ago Closed 5 years ago

entity openConversationButton.tooltip used twice, causes wrong tooltip for conversation button in main toolbar to be shown

Categories

(Thunderbird :: Toolbars and Tabs, defect)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6868+ fixed, thunderbird69 fixed, thunderbird70 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0
Tracking Flags:
Tracking Status
thunderbird_esr68 68+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: aryx, Assigned: khushil324)

Details

Attachments

(2 files)

Bug-1569399_openConversationButton-tooltip-edited.patch
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1569399-beta-esr68.patch
(deleted), patch
khushil324
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

Daily 70.0a1 20190726083520 and Thunderbird 68.0b5 on Windows 8.1

The entity openConversationButton.tooltip is used twice (for chat conversations and threaded mail conversations) which causes the wrong tooltip to be shown for the conversation button in the main toolbar.

Steps to reproduce:

  1. Customize main toolbar.
  2. Add 'Conversation' button.
  3. End customization.
  4. Move mouse over button.

Actual result:
Tooltip "Start a conversation" (mail/locales/en-US/chrome/messenger/chat.dtd)

Expected result:
Tooltip "Show conversation of selected message" (mail/locales/en-US/chrome/messenger/messenger.dtd)

Occurrences in code: https://dxr.mozilla.org/comm-release/search?q=openConversationButton&redirect=false

Since when do we use this twice? Is this working in TB 68? Anyway, too late to fix it there.

Khushil has done quite some work in chat, or else Richard may have some spare time.

Flags: needinfo?(richard.marti)
Flags: needinfo?(khushil324)
Assignee: nobody → khushil324
Flags: needinfo?(khushil324)

Should I create the patch for TB68 or Daily or both?

If you use a new entity, only for Daily. If you correct the loading order/scopes (= no new entity necessary), then for both, please. Thank you.

Flags: needinfo?(richard.marti)
Comment on attachment 9081224 [details] [diff] [review] Bug-1569399_openConversationButton-tooltip-edited.patch Looks good to me. Sadly not backportable since it's a string change, that is, you're creating a new string openMsgConversationButton.
Attachment #9081224 - Flags: review?(jorgk) → review+

Coming back to Aryx' suggestion:
https://dxr.mozilla.org/comm-release/search?q=openConversationButton&redirect=false

So openConversationButton is used in mail/locales/en-US/chrome/messenger/chat.dtd and mail/locales/en-US/chrome/messenger/messenger.dtd.

Khushil, can you try a version for TB 69 and TB 68 where you move chat.dtd after messenger.dtd here:
https://searchfox.org/comm-central/rev/84706b631aafd18e54074588efb71ddf91293ee6/mail/base/content/messenger.xul#30-34

Of course we can land the proper fix on trunk, but we should consider fixing the branches as well. Or do the reshuffling on all branches and additionally fix the string on trunk.

I'll wait for your reply before landing this.

Flags: needinfo?(khushil324)

Actually, I have a patch, I'll let you review it if it works.

Flags: needinfo?(khushil324)
Attached patch 1569399-beta-esr68.patch (deleted) — — Splinter Review

This fixes the Conversation button after being placed onto a toolbar. I can't see where the "start a conversation" button is used. Anyone knows?

Attachment #9081356 - Flags: review?(khushil324)
Comment on attachment 9081356 [details] [diff] [review] 1569399-beta-esr68.patch Review of attachment 9081356 [details] [diff] [review]: ----------------------------------------------------------------- It works for me. r=khushil.
Attachment #9081356 - Flags: review?(khushil324) → review+
Comment on attachment 9081356 [details] [diff] [review] 1569399-beta-esr68.patch This needs uplift to fix the incorrect tooltip.
Attachment #9081356 - Flags: approval-comm-esr68?
Attachment #9081356 - Flags: approval-comm-beta?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eaa12f8b4d9e
Move chat.dtd after messenger.dtd in messenger.xul to work around duplicate string issue. r=khushil
https://hg.mozilla.org/comm-central/rev/050d0ef127f2
openConversationButton.tooltip changed to openMsgConversationButton.tooltip for messenger.dtd to fix duplication. r=jorgk

Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Attachment #9081356 - Flags: approval-comm-esr68?
Attachment #9081356 - Flags: approval-comm-esr68+
Attachment #9081356 - Flags: approval-comm-beta?
Attachment #9081356 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: