Remove ondialogextra1 and ondialogextra2 attributes from xul files

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: jallmann, Assigned: jallmann)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment)

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

Summary: Remove ondialogextra2 attribute from xul files → Remove ondialogextra1 and ondialogextra2 attributes from xul files

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

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

Flags: needinfo?(gijskruitbosch+bugs)

(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. :-)

  1. 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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.

Flags: needinfo?(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.

Duplicate of this bug: 1521505

(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!)

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.

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6cf1169c014d
Replace ondialogextra1 and ondialogextra2 attributes with event handlers, r=Gijs

(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.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
No longer depends on: 1535080
Regressions: 1535080
You need to log in before you can comment on or make changes to this bug.