Closed Bug 1541789 Opened 2 years ago Closed 2 years ago

Port Bug 1529231 Remove ondialogaccept and ondialogcancel attributes from xul files

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached patch 1541789.patchSplinter Review
Attachment #9055733 - Flags: review?(philipp)
Attachment #9055733 - Flags: review?(mkmelin+mozilla)

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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

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+

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)
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
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+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 1541789-return-true-false-1.diff (obsolete) — Splinter Review

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)
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)
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 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+
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 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+

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
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/cd7996fcf42b
Follow-up: fix a missing curly brace; rs=me DONTBUILD

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

Linting? In editor?

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+
Attached 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)

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+
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
Type: defect → task
Regressions: 1547581
Regressions: 1550865
Regressions: 1563694
Regressions: 1577989
Regressions: 1578148
Blocks: 1578463
You need to log in before you can comment on or make changes to this bug.