1578463 - Mop up ondialogaccept in mail(news)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Ian Neal]

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

Comment 1

[Ian Neal]

Attachment: Mail ondialogaccept mop up

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

Comment 2

[Ian Neal]

Attachment: Mail ondialogaccept mop up v1.1

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[:aceman]

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.

Comment 5

[:aceman]

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

Comment 6

[Geoff Lankow (:darktrojan)]

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

Aceman's review is sufficient.

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Pulsebot]

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

Comment 9

[Jorg K (CEST = GMT+2)]

TB 70 beta 1:
https://hg.mozilla.org/releases/comm-beta/rev/ab0554e7ca10f0496458892ad0a66b6a3aca43bb

Comment 10

[Jorg K (CEST = GMT+2)]

TB 68.1.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/ae6b32c4ae2137f290ad3b191f32406e76392779