Port Bug 1529231 Remove ondialogaccept and ondialogcancel attributes from xul files

RESOLVED FIXED in Thunderbird 68.0

Status

task
RESOLVED FIXED
2 months ago
14 days ago

People

(Reporter: jorgk, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Comment 1

2 months ago
Posted patch 1541789.patchSplinter Review
Attachment #9055733 - Flags: review?(philipp)
Attachment #9055733 - Flags: review?(mkmelin+mozilla)

Comment 2

2 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ea892780482c
Port bug 1529231: Replace ondialogaccept and ondialogcancel attributes with addEventListener. rs=bustage-fix,jorgk

Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Reporter

Updated

2 months ago
Target Milestone: --- → Thunderbird 68.0
Reporter

Comment 3

2 months ago

Almost there:
mozmake mozmill-one SOLO_TEST=composition/test-multipart-related.js
fails now. The test inserts and image using imageDlg, but that doesn't appear to work, the dialogue is not accepted and no image is inserted. Recent change:
https://hg.mozilla.org/comm-central/rev/ea892780482c5941b4df1b179cd6b92c681d8757#l54.9

Test does: dialog.window.document.getElementById("imageDlg").acceptDialog();

Comment on attachment 9055733 [details] [diff] [review]
1541789.patch

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

rs=mkmelin but probably there's something more to fix.
Attachment #9055733 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 5

2 months ago

Fix for the failing mozmill test. The event.preventDefault bit isn't actually required to fix the test but still needs changing. There may be other instances.

Assignee: nobody → geoff
Attachment #9055755 - Flags: review?(jorgk)

Comment 6

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/13ef0f807c90
Follow-up: have dialog accept handlers run in the right order when inserting an image. r=jorgk DONTBUILD
Reporter

Comment 7

2 months ago
Comment on attachment 9055755 [details] [diff] [review]
1541789-insert-image-fix.diff

Thanks, I couldn't test it since my Mozmill is broken.
Attachment #9055755 - Flags: review?(jorgk) → review+
Assignee

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 8

2 months ago

ondialogaccept and ondialogcancel could return false to prevent the action. Now they can't, so it needs to be done with event.preventDefault.

Please check I haven't made logic errors, especially by forgetting to return where I should.

Attachment #9055769 - Flags: review?(acelists)
Reporter

Comment 9

2 months ago
Comment on attachment 9055769 [details] [diff] [review]
1541789-return-true-false-1.diff

Perhaps Magnus can get to this quicker. And the Calendar part needs a Calendar peer, or not?
Attachment #9055769 - Flags: review?(philipp)
Attachment #9055769 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 10

2 months ago
Comment on attachment 9055769 [details] [diff] [review]
1541789-return-true-false-1.diff

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

I'd say it's mostly OK with one nit and two questions.

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +261,1 @@
>      }

That entire block can go.

::: editor/ui/dialogs/content/EdLinkChecker.js
@@ +191,5 @@
>  /*
>    LinkCheckTimeOut();
>  */
> +  if (!onCancel()) {
> +    event.preventDefault();

Which onCancel() is being called here? And does it still return true/false? We're just in the process of removing that, right?

::: mailnews/base/prefs/content/AccountManager.js
@@ +294,5 @@
>  {
>    // If onCancel() present in current page frame, call it.
> +  if ("onCancel" in top.frames["contentFrame"]) {
> +    if (!top.frames["contentFrame"].onCancel()) {
> +      event.preventDefault();

Which onCancel() is called and does it still return true/false?
Attachment #9055769 - Flags: review+

Comment 11

2 months ago
Comment on attachment 9055769 [details] [diff] [review]
1541789-return-true-false-1.diff

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

Thanks, I looked at the non-calendar parts.

::: editor/ui/dialogs/content/EdColorPicker.js
@@ +258,3 @@
>  {
>    if (!ValidateData())
> +    event.preventDefault();

Return here.

::: editor/ui/dialogs/content/EdColorProps.js
@@ +429,5 @@
>        });
>        File.createFromNsIFile(nsFile).then(file => {
>          reader.readAsDataURL(file);
>        });
> +      event.preventDefault(); // Don't close just yet...

Return here.

::: editor/ui/dialogs/content/EdDialogCommon.js
@@ -560,5 @@
>  function onCancel()
>  {
>    SaveWindowLocation();
> -  // Close dialog by returning true
> -  return true;

Yeah, this needs to keep returning true if we want to test onCancel() as boolean elsewhere.

::: editor/ui/dialogs/content/EdFormProps.js
@@ +111,5 @@
>    {
>      Services.prompt.alert(window, GetString("Alert"), GetString("NoFormAction"));
>      gForm.Action.focus();
>      formActionWarning = false;
> +    event.preventDefault();

Return here.

::: editor/ui/dialogs/content/EdInsSrc.js
@@ +124,4 @@
>  {
>    let html = gDialog.srcInput.value;
>    if (!html)
> +    event.preventDefault();

Probably return here.

::: editor/ui/dialogs/content/EdLinkChecker.js
@@ +191,5 @@
>  /*
>    LinkCheckTimeOut();
>  */
> +  if (!onCancel()) {
> +    event.preventDefault();

Looks like the one in EdDialogCommon.js#560 .

::: editor/ui/dialogs/content/EdTableProps.js
@@ +1273,5 @@
>    var retVal = Apply();
>    if (retVal)
>      SaveWindowLocation();
>  
> +  if (!retVal) {

maybe just 'else' here?

::: mail/base/content/safeMode.js
@@ +47,5 @@
>        deleteLocalstore();
>      if (document.getElementById("disableAddons").checked) {
>        disableAddons();
>        // disableAddons will asynchronously restart the application
> +      event.preventDefault();

return here, just to be safe?
Attachment #9055769 - Flags: feedback+
Assignee

Comment 12

2 months ago
Attachment #9055769 - Attachment is obsolete: true
Attachment #9055769 - Flags: review?(philipp)
Attachment #9055769 - Flags: review?(mkmelin+mozilla)
Attachment #9055769 - Flags: review?(acelists)
Attachment #9056042 - Flags: review?(philipp)
Attachment #9056042 - Flags: review?(acelists)

Comment 14

2 months ago
Comment on attachment 9056042 [details] [diff] [review]
1541789-return-true-false-2.diff

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

Thanks
Attachment #9056042 - Flags: review?(acelists) → review+

Comment 15

2 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/76cba5d0233a
Fix cases where ondialog* attribute returned true or false. r=aceman(non-calendar), rs=bustage-fix,jorgk(calendar)

Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED

Comment 16

2 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/cd7996fcf42b
Follow-up: fix a missing curly brace; rs=me DONTBUILD
Reporter

Comment 17

2 months ago

Hmm, and the linter doesn't pick up such things?

Assignee

Comment 18

2 months ago

Linting? In editor?

Reporter

Comment 19

2 months ago

Editor, sigh, I should have looked before asking. Sorry.

Comment on attachment 9055733 [details] [diff] [review]
1541789.patch

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

r+ with this change:

::: calendar/base/content/dialogs/chooseCalendarDialog.js
@@ +8,5 @@
>  
>  var { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm");
>  
> +document.addEventListener("dialogaccept", doOK);
> +document.addEventListener("dialogextra1", doExtra1);

Putting the addEventListener at the top while the function is at the bottom is a bit strange, can we move these listeners to the bottom of the file? (here and in other places)
Attachment #9055733 - Flags: review?(philipp) → review+
Attachment #9056042 - Flags: review?(philipp) → review+
Assignee

Comment 21

a month ago
Posted patch 1541789-calendar-tidy-1.diff (obsolete) — Splinter Review

I've attached the functions directly to the addEventListener calls, it saves us from having a bunch of single-use functions lying around. I also managed to fix the closing of event dialogs and event tabs, which I broke earlier by not understanding the two ways it was called.

Attachment #9058126 - Flags: review?(philipp)
Assignee

Comment 22

a month ago

Oops, broke it.

Attachment #9058126 - Attachment is obsolete: true
Attachment #9058126 - Flags: review?(philipp)
Attachment #9058136 - Flags: review?(philipp)
Attachment #9058136 - Flags: review?(philipp) → review+

Comment 23

a month ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ad2d84d80a1e
Follow-up: Tidy up dialogaccept and dialogcancel listeners in calendar/. r=philipp
Regressions: 1544932

Updated

a month ago
Type: defect → task

Updated

28 days ago
Regressions: 1547581
Regressions: 1550865
You need to log in before you can comment on or make changes to this bug.