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

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jorgk, Assigned: bugzilla2007)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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)
Reporter

Updated

2 years ago
Duplicate of this bug: 1392053
Reporter

Updated

2 years ago
Duplicate of this bug: 1392054

Updated

2 years ago
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
Reporter

Comment 4

2 years ago
OK, let's revisit in mozilla58.
Flags: needinfo?(jorgk)
Reporter

Updated

2 years ago
Flags: needinfo?(jorgk)
Assignee

Comment 5

a year ago
(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!
Assignee

Updated

a year ago
No longer depends on: 731688
Assignee

Comment 6

a year ago
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)
Assignee

Comment 7

a year ago
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+
Assignee

Comment 9

a year ago
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

Comment 10

a year ago
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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91cf499eff0e
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.