1525636 - Remove ondialogextra1 and ondialogextra2 attributes from xul files

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Jonas Allmann [:jallmann]]

In order to remove the usage of new Function from dialog.xml[1], we need to remove the usage of attribute ondialogextra2[2] from all the places within our codebase.

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

[2] - https://dxr.mozilla.org/mozilla-central/search?q=ondialogextra1&redirect=false

Comment 1

[Jonas Allmann [:jallmann]]

Attachment: Bug 1525636, Replace ondialogextra1 and ondialogextra2 attributes with event handlers, r=gijs

Remove all occurences of the above mentioned attributes and replace them by event handlers.
Minor changes to consuming functions to preserve functionality.

Comment 2

[Jonas Allmann [:jallmann]]

I am not sure about the correctness of my changes for two reasons.

1. In some cases I was not sure which object to attach the eventHandler to; the window, the document, or a specific button.

2. The current implementation of _fireButtonEvent evaluates the return value of the functions stored in the ondialogextra attributes. The only way I see to reproduce that behaviour with event handlers is to use "event.preventDefault()" to cause dispatchEvent() to return false. Is that a viable solution?

https://searchfox.org/mozilla-central/source/toolkit/content/widgets/dialog.xml#435

Comment 3

[:Gijs (he/him)]

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

I am not sure about the correctness of my changes for two reasons.

1. In some cases I was not sure which object to attach the eventHandler to; the window, the document, or a specific button.

It's dispatched on the <dialog> element, so on document.documentElement. The event bubbles, so it doesn't really matter much where we attach the listener. document is fine, and you were consistent, which is the more important issue. So not worried about this part. :-)

2. The current implementation of _fireButtonEvent evaluates the return value of the functions stored in the ondialogextra attributes. The only way I see to reproduce that behaviour with event handlers is to use "event.preventDefault()" to cause dispatchEvent() to return false. Is that a viable solution?

I think that sounds great. The thing I don't really understand is... afaict, we never use the return value for the extra1/extra2 buttons, right? I see us using it for accept/cancel, but not for anything else. We pass it back to _doButtonCommand which (besides the accept/cancel cases) passes it some more, and then we pass it back to _handleButtonCommand... but I don't see it actually mattering for the extra1/extra2 case whether or not we preventDefault(). Or am I missing something?

Comment 4

[Jonas Allmann [:jallmann]]

(In reply to :Gijs (he/him) from comment #3)

I think that sounds great. The thing I don't really understand is... afaict, we never use the return value for the extra1/extra2 buttons, right? I see us using it for accept/cancel, but not for anything else. We pass it back to _doButtonCommand which (besides the accept/cancel cases) passes it some more, and then we pass it back to _handleButtonCommand... but I don't see it actually mattering for the extra1/extra2 case whether or not we preventDefault(). Or am I missing something?

Now that I look at it again, I think you are right. I missed the point that '_doButtonCommand' only handles the return value for accept and cancel buttons, so in the case of the extra button it can be ignored.

Comment 5

[Jonas Allmann [:jallmann]]

I removed the unnecessary event.preventDefault() and pushed the patch to try (tests not finished at the time of writing), which looks good so far.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0200ed7f10605fe86d4c7331ef5e23efbdc40e

I already fixed the linting errors.

Comment 7

[:Gijs (he/him)]

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

I removed the unnecessary event.preventDefault() and pushed the patch to try (tests not finished at the time of writing), which looks good so far.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0200ed7f10605fe86d4c7331ef5e23efbdc40e

I already fixed the linting errors.

Please note my follow-up comment on phabricator - https://phabricator.services.mozilla.com/D20368#inline-111267

(sorry for the spread discussion!)

Comment 8

[Jonas Allmann [:jallmann]]

Please note my follow-up comment on phabricator - https://phabricator.services.mozilla.com/D20368#inline-111267

I corrected that. (After prematurely pushing a correction that was still not fully correct.)

(sorry for the spread discussion!)

I guess that's partly my fault. I'm still figuring out where to best have these review discussions.
Please don't hesitate to point out if I can improve my use of the different tools or make things easier for you as a reviewer.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cf1169c014d
Replace ondialogextra1 and ondialogextra2 attributes with event handlers, r=Gijs

Comment 10

[:Gijs (he/him)]

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

Please note my follow-up comment on phabricator - https://phabricator.services.mozilla.com/D20368#inline-111267

I corrected that. (After prematurely pushing a correction that was still not fully correct.)

Perfect, I triggered landing based on the trypush - we don't test the "Browse..." path in the app picker, I think...

(sorry for the spread discussion!)

I guess that's partly my fault. I'm still figuring out where to best have these review discussions.

No worries!

Please don't hesitate to point out if I can improve my use of the different tools or make things easier for you as a reviewer.

For me, just keeping things on phabricator will be slightly easier, but it's not a big deal tbh. I'll see review requests and comments that come in there - if I don't respond after a day or two, feel free to ping me on the bug then.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6cf1169c014d