1541789 - Port Bug 1529231 Remove ondialogaccept and ondialogcancel attributes from xul files

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Geoff has a patch here:
https://hg.mozilla.org/try-comm-central/rev/66b16f0640176835fb05ac06411013e36bebb2fb

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1541789.patch

Comment 2

[Pulsebot]

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

Comment 3

[Jorg K (CEST = GMT+2)]

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 4

[Magnus Melin [:mkmelin]]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1541789-insert-image-fix.diff

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Jorg K (CEST = GMT+2)]

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

Thanks, I couldn't test it since my Mozmill is broken.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1541789-return-true-false-1.diff

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.

Comment 9

[Jorg K (CEST = GMT+2)]

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?

Comment 10

[Jorg K (CEST = GMT+2)]

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?

Comment 11

[:aceman]

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?

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1541789-return-true-false-2.diff

Comment 13

[Geoff Lankow (:darktrojan)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=affd70daef9c592e80e1f4ea84878c208c4c7ed5

Comment 14

[:aceman]

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

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

Thanks

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cd7996fcf42b
Follow-up: fix a missing curly brace; rs=me DONTBUILD

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Geoff Lankow (:darktrojan)]

Linting? In editor?

Comment 19

[Jorg K (CEST = GMT+2)]

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

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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)

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: 1541789-calendar-tidy-1.diff

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.

Comment 22

[Geoff Lankow (:darktrojan)]

Attachment: 1541789-calendar-tidy-2.diff

Oops, broke it.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ad2d84d80a1e
Follow-up: Tidy up dialogaccept and dialogcancel listeners in calendar/. r=philipp