Something wrong here: goSetLabelAccesskeyTooltiptext("cmd_properties-button", ... ): getElementById("cmd_properties-button") failed!

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Address Book
--
minor
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

({regression})

Trunk
Thunderbird 53.0
regression

Thunderbird Tracking Flags

(thunderbird51 unaffected, thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

STR:

Open a compose window.
Summon the contacts side bar.
Click onto a contact.

In the debug window see:
Something wrong here: goSetLabelAccesskeyTooltiptext("cmd_properties-button", ...): getElementById("cmd_properties-button") failed!--DOCSHELL 0000000009011800 =

Not nice. Not even a newline after the message.

This was introduced here:
https://hg.mozilla.org/comm-central/rev/71581aa20801#l1.124

Can you please
a) add a newline to the dumps
b) Not print a message if you detect an expected case.
(Assignee)

Comment 1

5 months ago
Created attachment 8814624 [details] [diff] [review]
1320507-addressing-dump.patch (v1)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8814624 - Flags: review?(acelists)

Comment 2

5 months ago
Comment on attachment 8814624 [details] [diff] [review]
1320507-addressing-dump.patch (v1)

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

I asked about this in bug 1310442 comment 23, whether a missing element could be sometimes expected. But I do not remember how it ended.

::: mail/components/addrbook/content/abCommon.js
@@ +906,5 @@
>        }
>      } else if (aTooltipTextAttribute === "") {
>        node.removeAttribute("tooltiptext");
>      }
> +  } else if (aID != "cmd_properties-button") {

We may use this function for many other cases and elements so we can't whitelist all the exceptions. I think we should just be silent when the element is not there. That is what the current m-c functions do too, e.g. https://dxr.mozilla.org/comm-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozilla/toolkit/content/globalOverlay.js#131

I now wonder if the other dumps are useful too. The function may remove the attribute it some cases so not finding it later is not an error, we can always set it.

Yes, I suggested to add them in my comment there:( What do you think?
Attachment #8814624 - Flags: review?(acelists)
(Assignee)

Comment 3

5 months ago
(In reply to :aceman from comment #2)
> Yes, I suggested to add them in my comment there:( What do you think?
Hmm, I don't understand the comment. "Them" and "there" referring to what?

I think that you're the best candidate to fix it, r?jorgk ;-)
Assignee: jorgk → acelists
(Assignee)

Updated

5 months ago
Attachment #8814624 - Attachment is obsolete: true

Comment 4

5 months ago
(In reply to Jorg K (GMT+1) from comment #3)
> (In reply to :aceman from comment #2)
> > Yes, I suggested to add them in my comment there:( What do you think?
> Hmm, I don't understand the comment. "Them" and "there" referring to what?

Them = the dump statements in case the attributes is not found.
There = the comment 23 in bug 1310442 .

> I think that you're the best candidate to fix it, r?jorgk ;-)

No, I need a second opinion as I propose to remove the checks and dumps I have myself requested to add.
(Assignee)

Comment 5

5 months ago
I really didn't want to get involved (like in the 'oncommand' popup issue), so here I am being asked for advice.

Let's try a minimal version of advice here:
I don't want to see a dump saying "something wrong here" is there is nothing wrong. So if you're saying that there can be situations where the attribute is not found, then there should be no dump.

Comment 6

5 months ago
Ah, I take the proposal back. We look for one attribute e.g. named "aLabelAttribute", then get its value and set it to the real attribute, e.g. "label". I was thinking why we dump if "label" was not there, when we can just set it. But the check actually checks "aLabelAttribute" for existence. And that is bad if it isn't found, we can't copy the value and that is unexpected.

So please refresh your patch, just remove the special id checking and do not dump if node was not found.
(Assignee)

Comment 7

5 months ago
Created attachment 8814628 [details] [diff] [review]
1320507-addressing-dump.patch (v2)
Assignee: acelists → jorgk
Attachment #8814628 - Flags: review?(acelists)

Comment 8

5 months ago
Comment on attachment 8814628 [details] [diff] [review]
1320507-addressing-dump.patch (v2)

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

Yes, thanks.
Attachment #8814628 - Flags: review?(acelists) → review+
(Assignee)

Comment 9

5 months ago
Created attachment 8814633 [details] [diff] [review]
1320507-addressing-dump.patch (v3)

How about something a little more elegant ;-)
No need to unroll a loop of three.
Attachment #8814628 - Attachment is obsolete: true
Attachment #8814633 - Flags: review?(acelists)
(Assignee)

Updated

5 months ago
Attachment #8814633 - Attachment description: 1320507-addressing-dump.patch → 1320507-addressing-dump.patch (v3)

Comment 10

5 months ago
Comment on attachment 8814633 [details] [diff] [review]
1320507-addressing-dump.patch (v3)

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

Nice.
Attachment #8814633 - Flags: review?(acelists) → review+
(Assignee)

Comment 11

5 months ago
C-C (TB 53): https://hg.mozilla.org/comm-central/rev/398acd8847d2c329993a97368ecf1ee9eee2d110
C-A (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/e1f2f159d96f57e9f21e0599b3f33f32408cb73d
(uplifted so it joins the original change)
Severity: enhancement → minor
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-thunderbird51: --- → unaffected
status-thunderbird52: --- → fixed
status-thunderbird53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(In reply to Jorg K (GMT+1) from comment #9)
> Created attachment 8814633 [details] [diff] [review]
> 1320507-addressing-dump.patch (v3)
> 
> How about something a little more elegant ;-)
> No need to unroll a loop of three.

Awesome! Great teamwork.

FTR, my original dump on missing element had a question mark: "Something wrong here?", to hint that it may or may not be wrong when getElementById does not succeed. However, Aceman rationalized that subtle difference away... :)
You need to log in before you can comment on or make changes to this bug.