Remove ondialogaccept and ondialogcancel attributes from xul files

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 months ago
18 days ago

People

(Reporter: jallmann, Assigned: jallmann)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 months ago

+++ This bug was initially created as a clone of Bug #1525636 +++

In order to remove the usage of new Function from dialog.xml[1], all occurences of the ondialogaccept[2] and ondialogcancel[3] attributes need to be removed from the codebase and replaced with event handlers.

[1] - https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/content/widgets/dialog.xml#418

[2] - https://searchfox.org/mozilla-central/search?q=ondialogaccept&path=

[3] - https://searchfox.org/mozilla-central/search?q=ondialogcancel&path=

(Assignee)

Comment 1

2 months ago

Removed all occurences of ondialogaccept.
Removed all occurences of ondialogcancel.
Replaced all removed attributes with event handlers.

(Assignee)

Comment 2

2 months ago

I'm having difficulties with the following files:
I want to replace the two attributes ondialogaccept="return dialog.onOK()" and ondialogcancel="return dialog.onCancel()" in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#30

As far as I understand, the responsible script is HelperAppDlg.jsm and I'm trying to add the eventHandlers somewhere in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/HelperAppDlg.jsm#411

I don't know how to correctly reference the functions onOK() and onCancel() in the definition of the event handlers.
Things I've tried include the following:

this.mDialog.document.addEventListener("dialogaccept", function() { dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { nsUnknownContentTypeDialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.mDialog.dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", this.onOK);
this.mDialog.document.addEventListener("dialogaccept", onOK);

None of these appear to work. Either the function can not be referenced at all or subsequent references inside onOK() are broken. I'm out of ideas how this could work and would be very thankful for advice.

Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

2 months ago

Sorry for the delay in response.

(In reply to Jonas Allmann [:jallmann] from comment #2)

I'm having difficulties with the following files:
I want to replace the two attributes ondialogaccept="return dialog.onOK()" and ondialogcancel="return dialog.onCancel()" in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#30

As far as I understand, the responsible script is HelperAppDlg.jsm and I'm trying to add the eventHandlers somewhere in

https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/HelperAppDlg.jsm#411

Yep.

I don't know how to correctly reference the functions onOK() and onCancel() in the definition of the event handlers.
Things I've tried include the following:

this.mDialog.document.addEventListener("dialogaccept", function() { dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { nsUnknownContentTypeDialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", function() { this.mDialog.dialog.onOK(); });
this.mDialog.document.addEventListener("dialogaccept", this.onOK);
this.mDialog.document.addEventListener("dialogaccept", onOK);

None of these appear to work. Either the function can not be referenced at all or subsequent references inside onOK() are broken. I'm out of ideas how this could work and would be very thankful for advice.

So the reason these don't work is:

  • the first two: they're being run in the .jsm scope, and there is no global "dialog" variable. "nsUnknownContentTypeDialog" is a function (used as a constructor) and it doesn't have an "onOK" property; only the instances constructed using that function would do. We need to reference the instance we're using for this dialog.
  • the ones with a function() closure that use 'this' - the event handler won't get invoked with the same this as the method that's adding the event listeners (ie the place where the this.mDialog.document.addEventListener call is. If you want to preserve the same this, you could use an arrow function. However, passing a function instance inline like that would make it impossible to remove the listener (because we're not keeping a reference to the (arrow) function we're passing, and so there's nothing we can pass to removeEventListener that will remove the listener again).
  • passing this.onOK will work but have a different this when called. Again, an arrow function could work, or you could pass this.onOK.bind(this). In some cases, we re-bind methods on objects in the constructor so that this type of passing of function references works, and we can then also remove the listener again using that reference. See e.g. https://searchfox.org/mozilla-central/source/browser/base/content/browser-pageActions.js#51 for an example.
  • passing onOK literally doesn't work because there's no onOK variable in the global scope in the jsm.

Do we ever remove the listeners? We might need to do so to avoid leaks...

I'd suggest something like this:

this.mDialog.document.addEventListener("dialogaccept", this);
this.mDialog.document.addEventListener("dialogcancel", this);

and then define a handleEvent method on nsUnknownContentTypeDialog.prototype. This will correctly keep this as the same nsUnknownContentTypeDialog instance, and so you can call this.onOK() and this.onCancel() or whatever from that method, based on the event type (handleEvent will be passed 1 argument, which is the event).

You can then factor out the final few lines of onOK and onCancel, which are the same, into an onUnload method, and there also remove the listeners again (removeEventListener("dialogaccept", this) etc.).

Hopefully that helps!

Flags: needinfo?(gijskruitbosch+bugs)

Updated

a month ago
See Also: → 1536415

Comment 4

27 days ago

This is approved and ready to land but can't because it conflicts with the fix from bug 1535080 - can you rebase this patch on top of that one, and then resubmit to phabricator? (using moz-phab and hg, moz-phab submit . will submit only the current commit, which should help)

Flags: needinfo?(jallmann)
(Assignee)

Updated

27 days ago
Status: NEW → ASSIGNED
Flags: needinfo?(jallmann)
(Assignee)

Updated

27 days ago
Keywords: checkin-needed

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

Keywords: checkin-needed
Flags: needinfo?(jallmann)

Comment 7

27 days ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/8691a4583cae
Remove all occurences of ondialogaccept and ondialogcancel, r=Gijs

Comment 8

26 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
(Assignee)

Updated

20 days ago
Flags: needinfo?(jallmann)
Attachment #9053646 - Attachment is obsolete: true

Comment 9

18 days ago

It would be nice if you could CC someone from the Thunderbird team on bugs like this one.

Updated

18 days ago
Depends on: 1541789

Comment 10

18 days ago

(In reply to Jorg K (GMT+2) from comment #9)

It would be nice if you could CC someone from the Thunderbird team on bugs like this one.

Sorry this took you by surprise; It looks like there's people CC'd on the metabug, and so I assume there's not much point CC'ing you on all the other (mostly closed) items in its dep tree at this point...

Comment 11

18 days ago

What's the meta-bug? The eval stuff in bug 1473549? I thought this is part of the de-XUL effort, no?

Comment 12

18 days ago

(In reply to Jorg K (GMT+2) from comment #11)

What's the meta-bug? The eval stuff in bug 1473549?

Yes.

I thought this is part of the de-XUL effort, no?

No.

You need to log in before you can comment on or make changes to this bug.