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

ASSIGNED
Assigned to

Status

defect
--
major
ASSIGNED
3 years ago
3 years ago

People

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

Tracking

({regression, ux-consistency})

Dependency tree / graph

Thunderbird Tracking Flags

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

Details

(Whiteboard: [leave open])

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

3 years ago
+++ 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.
Assignee

Updated

3 years ago
Blocks: 1310442
Assignee

Updated

3 years ago
Blocks: 1315924

Comment 1

3 years ago
Accept my apologies for the bustage.
It would be great if the goSetLabelAccesskeyTooltiptext() could be placed at some shared place.
Assignee

Updated

3 years ago
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)
Assignee

Comment 2

3 years ago
Posted 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 3

3 years ago
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?
Assignee

Comment 4

3 years ago
Posted 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 5

3 years ago
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
Assignee

Comment 7

3 years ago
I won't be able to do anything this week.

Comment 8

3 years ago
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/?

Comment 9

3 years ago
I think not because the function calls the new strings and those are not in SM yet (at least not in 52 branch).

Updated

3 years ago
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED

Comment 10

3 years ago
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+

Updated

3 years ago
Attachment #8812506 - Attachment is obsolete: true
Attachment #8812506 - Flags: review?(iann_bugzilla)
Attachment #8812506 - Flags: review?(acelists)
Attachment #8812506 - Flags: approval-comm-aurora?
Assignee

Comment 11

3 years ago
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+
Assignee

Updated

3 years ago
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
Assignee

Comment 12

3 years ago
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?
Assignee

Updated

3 years ago
Attachment #8817916 - Attachment description: Patch as checked in (v3) → Hotfix as checked in (v3) [comment #11]

Comment 13

3 years ago
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?
Assignee

Comment 14

3 years ago
(In reply to Ian Neal from comment #13)
> Flags: approval-mozilla-aurora? → approval-comm-aurora?

Sorry, always screwing that up...  :-\

Comment 15

3 years ago
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+
Assignee

Comment 17

3 years ago
(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.

Comment 18

3 years ago
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?
Assignee

Comment 19

3 years ago
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.

Comment 21

3 years ago
Marking this fixed for now so the Aurora query doesn't pick this up.
You need to log in before you can comment on or make changes to this bug.