Closed Bug 1392055 Opened 3 years ago Closed 3 years ago

Pass the full event to the callback in toolkit/content/widgets/notification.xml

Categories

(Toolkit :: Notifications and Alerts, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jorgk-bmo, Assigned: bugzilla2007)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(jhofmann)
Duplicate of this bug: 1392053
Duplicate of this bug: 1392054
Blocks: 1392056
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.
Component: XP Toolkit/Widgets: XUL → Notifications and Alerts
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(jhofmann)
Product: Core → Toolkit
Priority: -- → P3
OK, let's revisit in mozilla58.
Flags: needinfo?(jorgk)
Flags: needinfo?(jorgk)
(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!
No longer depends on: 731688
Attached patch 1392055_notification.xml.patch (obsolete) — Splinter Review
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.
Attachment #8937445 - Flags: review?(jhofmann)
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.
Attachment #8937445 - Attachment is obsolete: true
Attachment #8937445 - Flags: review?(jhofmann)
Attachment #8937446 - Flags: review?(jhofmann)
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. :)
Attachment #8937446 - Flags: review?(jhofmann) → review+
Thanks for rapid review :)

So I guess that makes this ready for check-in, doesn't it?
Keywords: checkin-needed
Summary: Pass the full event or at least event.shiftKey to the callback in toolkit/content/widgets/notification.xml → Pass the full event to the callback in toolkit/content/widgets/notification.xml
Assignee: nobody → bugzilla2007
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/91cf499eff0e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.