Closed Bug 1318852 Opened 4 years ago Closed 18 days ago

Make labels of cmd_properties action-oriented and context-sensitive, e.g. "Edit Contact", "Edit List", etc. (port Thunderbird bug 1310442)

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
major

Tracking

(thunderbird_esr78 unaffected, thunderbird52 fixed, seamonkey2.48 unaffected, seamonkey2.49esr affected, seamonkey2.50 affected)

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird52 --- fixed
seamonkey2.48 --- unaffected
seamonkey2.49esr --- affected
seamonkey2.50 --- affected

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

Details

(Keywords: regression, ux-consistency, Whiteboard: [leave open])

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1310442 +++

Changes/additions from mail/components/addrbook/content need to be ported to suite/ given https://hg.mozilla.org/comm-central/diff/71581aa20801/mailnews/addrbook/content/abResultsPane.js

(Quoting Frank-Rainer Grahl from bug 1315924 comment #2)
> Hmm it looks like it was updated probably by TB without porting all changes
> / strings to SeaMonkey. Maybe this is the problem. rsx11m could you check it?
> 
> Timestamp: 11/19/2016 12:30:54 PM
> Error: ReferenceError: goSetLabelAccesskeyTooltiptext is not defined
> Source File: chrome://messenger/content/addressbook/abResultsPane.js
> Line: 444
> 
> Timestamp: 11/19/2016 12:30:54 PM
> Error: An error occurred updating the cmd_properties command: [Exception...
> "[JavaScript Error: "goSetLabelAccesskeyTooltiptext is not defined" {file:
> "chrome://messenger/content/addressbook/abResultsPane.js" line:
> 444}]'[JavaScript Error: "goSetLabelAccesskeyTooltiptext is not defined"
> {file: "chrome://messenger/content/addressbook/abResultsPane.js" line:
> 444}]' when calling method: [nsIController::isCommandEnabled]"  nsresult:
> "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS
> frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line
> 84"  data: yes]
> Source File: chrome://global/content/globalOverlay.js
> Line: 89

(Quoting rsx11m from bug 1315924 comment #3)
> Bug 1310442 - Make labels of cmd_properties action-oriented and
> context-sensitive, e.g. "Edit Contact", "Edit List", etc. (consistent with
> improved dialogue titles from bug 1312262).
> 
> https://hg.mozilla.org/comm-central/rev/71581aa20801 only pushed strings for
> mail/ but not suite/.
> Confirming per code review.
Blocks: 1310442
Blocks: 1315924
Accept my apologies for the bustage.
It would be great if the goSetLabelAccesskeyTooltiptext() could be placed at some shared place.
Summary: Make labels of cmd_properties action-oriented and context-sensitive, e.g. "Edit Contact", "Edit List", etc. (port Thunderbird bug 1310442)) → Make labels of cmd_properties action-oriented and context-sensitive, e.g. "Edit Contact", "Edit List", etc. (port Thunderbird bug 1310442)
Attached patch Hotfix for suite (obsolete) — Splinter Review
Since this was already merged into comm-aurora and we won't be able to port the string changes there, this patch simply disables the change for suite/ until a proper solution is found here.
Attachment #8812506 - Flags: review?(iann_bugzilla)
Attachment #8812506 - Flags: review?(acelists)
Attachment #8812506 - Flags: approval-comm-aurora?
Comment on attachment 8812506 [details] [diff] [review]
Hotfix for suite

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

Could you just check for the existence of the function (typeof goSetLabelAccesskeyTooltiptext == "function") to avoid the preprocessing?
Attached patch Alternative patch (obsolete) — Splinter Review
Like this? I didn't test this version...
Attachment #8812509 - Flags: review?(iann_bugzilla)
Attachment #8812509 - Flags: review?(acelists)
Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

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

Thanks, works for me in TB. Needs testing in SM.

::: mailnews/addrbook/content/abResultsPane.js
@@ +414,5 @@
>            }
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")

Missing { at the end.

@@ +415,5 @@
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")
> +          let labelAttr = "valueGeneric";

I assume this is a temporary measure and you will fix this shortly for SM in the proper way, so please do not reindent the whole block unnecessarily (you would then reindent it back later).
Attachment #8812509 - Flags: review?(acelists) → review+
>> Since this was already merged into comm-aurora and we won't be able to port the string changes there,

Its still early in the c-a cycle and 52 is the next ESR so maybe we could get away with string changes this time only?

FRG
I won't be able to do anything this week.
Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

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

::: mailnews/addrbook/content/abResultsPane.js
@@ +414,5 @@
>            }
>          }
>          return (enabled && (numSelected > 0));
>        case "cmd_properties":
> +        if (typeof goSetLabelAccesskeyTooltiptext == "function")

Can't you just move the function from mail/ to mailnews/?
I think not because the function calls the new strings and those are not in SM yet (at least not in 52 branch).
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Comment on attachment 8812509 [details] [diff] [review]
Alternative patch

[Triage Comment]
r/a=me
Attachment #8812509 - Flags: review?(iann_bugzilla)
Attachment #8812509 - Flags: review+
Attachment #8812509 - Flags: approval-comm-aurora+
Attachment #8812506 - Attachment is obsolete: true
Attachment #8812506 - Flags: review?(iann_bugzilla)
Attachment #8812506 - Flags: review?(acelists)
Attachment #8812506 - Flags: approval-comm-aurora?
Thanks for the reviews, comment #5 addressed (no indentation).
Pushed as https://hg.mozilla.org/comm-central/rev/0b822264879a
Attachment #8812509 - Attachment is obsolete: true
Attachment #8817916 - Flags: review+
Component: MailNews: Address Book & Contacts → Address Book
Flags: approval-comm-aurora+
Product: SeaMonkey → MailNews Core
Whiteboard: [leave open]
Target Milestone: --- → Thunderbird 53.0
Version: SeaMonkey 2.49 Branch → 52
Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

I've moved this to MailNews Core given that the final solution will involve moving that function on trunk. I won't get to this before January.

For SeaMonkey 2.49, we'll need this on aurora where it's essentially NPOTB for Thunderbird 52, but Joerg probably needs to re-approve (and will check in).

Approval Request Comment
[Feature/Bug causing the regression]: bug 1310442
[User impact if declined]: Bustage for SeaMonkey
Attachment #8817916 - Flags: approval-mozilla-aurora?
Attachment #8817916 - Attachment description: Patch as checked in (v3) → Hotfix as checked in (v3) [comment #11]
Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Think you meant this flag.
Attachment #8817916 - Flags: approval-mozilla-aurora? → approval-comm-aurora?
(In reply to Ian Neal from comment #13)
> Flags: approval-mozilla-aurora? → approval-comm-aurora?

Sorry, always screwing that up...  :-\
Comment on attachment 8817916 [details] [diff] [review]
Hotfix as checked in (v3) [comment #11]

I'm really not so happy with this solution. You should have added a comment to say that this is a temporary measure (without proper indentation) and pointed to a bug number.

I will take the liberty to land a comment with rs=documentation-only.
Attachment #8817916 - Flags: approval-comm-aurora? → approval-comm-aurora+
(In reply to Jorg K (GMT+1) from comment #15)
> I'm really not so happy with this solution. You should have added a comment
> to say that this is a temporary measure (without proper indentation) and
> pointed to a bug number.

Makes perfectly sense, apologies for that omission.
I assume that you'll land this on comm-aurora.
Yep. I landed on Aurora yesterday, so unless you're in a hurry, it will wait a few days. How/where do you build SM, BTW?
No rush, next weekend will be fine. Building SM works the same way as TB, just change the target application from "mail" to "suite" (recently however trunk was frequently busted due to changes on mozilla-central). I usually build on 64-bit Linux, but use Windows for UI-relevant changes too.
Marking this fixed for now so the Aurora query doesn't pick this up.

STM we should close this and make a new bug?

Flags: needinfo?(geoff)

This code disappeared a long time ago. I don't see anything left to do.

Status: ASSIGNED → RESOLVED
Closed: 18 days ago
Flags: needinfo?(geoff)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.