Closed Bug 1578463 Opened 5 months ago Closed 4 months ago

Mop up ondialogaccept in mail(news)

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

There seems to be a couple of straggling ondialogaccept occurrences still in mail/ and mailnews/ following the landings from Bug 1541789 and linked bugs.

Attached patch Mail ondialogaccept mop up (obsolete) — Splinter Review

This will need testing but hopefully a good starting point. Any changes for suite/ can be done in bug 1578467.

Attachment #9090110 - Flags: review?(jorgk)
Blocks: 1578467

As far as I can see without this patch nothing breaks (not sure about am-smime.js), just maybe some unexpected behaviour for users.
selectDialog.xul - changes already done in toolkit/ just removing spurious ondialogaccept in xul
am-smime.js - there would have been two dialogaccept event listeners for am-identity-edit.xul, the first being loaded in am-identity-edit.js

All will need testing.

Attachment #9090110 - Attachment is obsolete: true
Attachment #9090110 - Flags: review?(jorgk)
Attachment #9090118 - Flags: review?(jorgk)
Comment on attachment 9090118 [details] [diff] [review]
Mail ondialogaccept mop up v1.1

Thanks Ian. I prefer to leave this to the team who worked on this first. Is there anything that looks like it's not working now? I'd rather fix it for TB 68 before anyone complains. Not being able to edit tags (bug 1578148) was quite bad.
Attachment #9090118 - Flags: review?(jorgk)
Attachment #9090118 - Flags: review?(geoff)
Attachment #9090118 - Flags: review?(acelists)
Comment on attachment 9090118 [details] [diff] [review]
Mail ondialogaccept mop up v1.1

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

Yes I think this is correct by code inspection. Manipulating 'ondialogaccept' attributes is dead anyway. But I tested all the codepaths (except selectDialog) and the "on accept" functions seem to be called where expected.
The read-only cards and addressbooks do NOT run the "on accept" functions, as intended.
The am-smime calls smimeSave() properly, whether it is inside the account manager pane, or in the standalone dialog in am-identity-edit.xul.
Attachment #9090118 - Flags: review?(acelists) → review+

Thanks Ian for spotting these. Maybe we only searched those ondialogaccept attributes in xul files only, so these in JS got missed.

Comment on attachment 9090118 [details] [diff] [review]
Mail ondialogaccept mop up v1.1

Aceman's review is sufficient.
Attachment #9090118 - Flags: review?(geoff)
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Comment on attachment 9090118 [details] [diff] [review]
Mail ondialogaccept mop up v1.1

Since I have seen too many regressions caused by ondialogaccept issues, I wanted two reviewers here. So I had to check it myself. Looks everything should have been working even without the mop-up.

BTW, there was a "prettier issue" which I fixed.

Still pondering whether to backport this. It shouldn't be necessary.
Attachment #9090118 - Flags: review+
Attachment #9090118 - Flags: approval-comm-esr68?
Attachment #9090118 - Flags: approval-comm-beta?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9f5cedbf9218
Mop up ondialogaccept in mail(news). r=aceman,jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Attachment #9090118 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9090118 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.