At https://dxr.mozilla.org/mozilla-central/rev/833f84d0d5c729054a3aa8b3f34735f56fe6436b/toolkit/content/widgets/notification.xml#491 var result = callback(this, button, aEvent.target); we call a button callback with the event target. It would be beneficial to pass the full event, or at least aEvent.shiftKey. So the patch would be a one liner adding the extra argument. Sorry about the NI SPAM, I really can't see who's in charge of this. Please point me to the right person and also adjust the component of this bug.

Let's move this to toolkit. I'm not sure who to ping here. Everyone is really busy with 57 right now. I can't imagine us turning down a patch that adds a fourth argument to the callback function, but I really only looked at it briefly.

OK, let's revisit in mozilla58.

(In reply to Jorg K (GMT+1) from comment #0) > At > https://dxr.mozilla.org/mozilla-central/rev/ > 833f84d0d5c729054a3aa8b3f34735f56fe6436b/toolkit/content/widgets/ > notification.xml#491 > var result = callback(this, button, aEvent.target); > > we call a button callback with the event target. It would be beneficial to > pass the full event, or at least aEvent.shiftKey. We definitely need the full event, adding only aEvent.shiftKey would be arbitrary and incomplete and thus perpetuate the shortcomings of the status quo. With the benefit of hindsight, the third argument was never supposed to be aEvent.target, but just aEvent, then any consumer can pick from aEvent whatever he needs. I'm not sure how much work it would be to replace the aEvent.target argument with aEvent, but it would require adjusting all consumers which sounds like much work. Adding aEvent as a fourth parameter is unlikely to break things for consumers, so that looks like the way of least resistance for now, notwithstanding later efforts to eliminate aEvent.target from the parameters list and update consumers accordingly. > So the patch would be a one liner adding the extra argument. Yes. Let's do that, please!

So here's the *ONE-line patch* as hinted by Jörg, adding aEvent as a fourth argument to the callback function. I am not in a position to check consumers of this, but I would not expect this change to break anything. Johann, kindly choose an appropriate reviewer if you're not reviewing this yourself. We should point out that it looks like a risk-free one-liner so it's not worth clogging up anyone's review queue because it'll be faster to just review it immediately. We're waiting for this to fix Thunderbird Bug 1392056, which is the last piece in the puzzle of a series of bugs to make the choice of composition mode consistent.

NEW *one-line* patch Ops, last patch somehow lost its context. > So here's the *ONE-line patch* as hinted by Jörg, adding aEvent as a fourth > argument to the callback function. > > I am not in a position to check consumers of this, but I would not expect > this change to break anything. > > Johann, kindly choose an appropriate reviewer if you're not reviewing this > yourself. > > We should point out that it looks like a risk-free one-liner so it's not > worth clogging up anyone's review queue because it'll be faster to just > review it immediately. > > We're waiting for this to fix Thunderbird Bug 1392056, which is the last > piece in the puzzle of a series of bugs to make the choice of composition > mode consistent.

Comment on attachment 8937446 [details] [diff] [review] 1392055_notification.xml.patch (v.1.1) Review of attachment 8937446 [details] [diff] [review]: ----------------------------------------------------------------- Seems good to me. :)

Thanks for rapid review :) So I guess that makes this ready for check-in, doesn't it?

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91cf499eff0e Pass the full event to the callback in notification.xml. r=jhofmann