Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16859 closed defect (fixed)

_WidgetBase forces custom event binding names to be all lowercase

Reported by: Alpha Owned by: bill
Priority: undecided Milestone: 2.0
Component: Dijit Version: 1.8.3
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

_WidgetBase.on calls dojo/on to add the event listener without modifications to the "type" parameter (event name passed). _WidgetBase.emit calls dojo/on to emit the event, using type.toLowerCase()

In such a way, calling widget.on("myEvent", function() {}) will never be triggered, even if the widget executes: this.emit("myEvent").

Workaround is to use all lowercase, but it can get cumbersome. ("buttononeclicked" vs. "buttonOneClicked")

Change History (18)

comment:1 Changed 6 years ago by Alpha

(Can I please by added to the cc: of this ticket? I cannot add myself when commenting on it, cannot do it when adding it, and cannot modify the ticket. Thanks.)

comment:2 Changed 6 years ago by bill

Milestone: tbd2.0

Don't worry, you'll get mail automatically for tickets you file.

IIRC the behavior is because _WidgetBase.emit() is used both to emit an event and to call legacy notification methods, ex: onClick(). So this limitation is probably better addressed for 2.0 than 1.9, unless you have a non-backwards-compatibility-breaking patch?

comment:3 Changed 6 years ago by Alpha

Thanks bill.

My idea was to just simplify all strings to be lowercase so that there's a standard ground for event handling, and developers can choose whatever naming scheme they want. They could even be inconsistent and emit("onclick") and catch("onClick"). I think this approach makes more sense than having emit("onClick") not be caught by on("onClick").

I do not know if this breaks any previous functionality (which is more important) but I'll try to provide a patch and make sure that all tests are consistent and working.

Will get back to you soon.

comment:4 Changed 6 years ago by bill

BTW, the arguments to emit() and on() shouldn't contain the substring "on"... for example for attaching to click on DOMNodes we say on(node, "click", ...).

I thought the current code was already simplifying all strings to be lowercase, via the line:

on.emit(this.domNode, type.toLowerCase(), eventObj);

comment:5 Changed 6 years ago by Alpha

You're right on the "on" comment, I chose a poor example.

The current code is simplifying the calls to emit, but not the calls to addEventListener, which creates the inconsistency.

comment:6 Changed 6 years ago by Alpha

Hi, I've been having several problems running the whole tests for _WidgetBase.

Still, I went ahead and tried out running the same set of tests with the following modification:

In the on function, replace the following statement:

return this.own(on(this.domNode, type, func))[0];

With the following one:

return this.own(on(this.domNode, type.toLowerCase(), func))[0];

As you see, the difference is quite trivial. However, I would like to have it verified through the tests, but I wasn't able to.

I also was able to verify that this inconsistency still exists in the last nightly build.

comment:7 Changed 6 years ago by bill

My worry is that not all DOM event names are lowercase, like DOMMouseScroll. I'm looking at http://en.wikipedia.org/wiki/DOM_events.

comment:8 Changed 6 years ago by Alpha

Thanks Bill, that makes sense. I realize I can keep suggesting patches here and there but that is certainly not a good or a sane approach.

I think the inconsistency needs to be tackled at the root:

  • make dojo/on be consistent on how it handles emit/on calls (it currently is: using the same casing)
  • make _WidgetBase be transparent on it and leave the event handling and naming to dojo/on. Right now it is stepping on the way and this shared responsibility will end on a consistent approach breaking either one or the other.

The quick patches I can think of is having _WidgetBase emit both the lowercase version of the event and the untouched version of the event (if it's not all lowercase already). It sucks to throw an event twice, but if they are going to be caught only by the same casing rule, then they shouldn't be caught twice.

(Please take my comments as a naive suggestion. I am not that experienced into dojo's history or legacy-support, but I do think that this test case reveals that "something does not quite feel right" and want to give my best as to help improve it. Thanks for bearing with me!)

comment:9 Changed 6 years ago by bill

I'd be scared to emit both events because I'm afraid that on some browsers that would lead to double notifications. You could easily patch your code to do that though. As I said above I'd rather just wait until 2.0 and then fix this correctly.

comment:10 Changed 6 years ago by bill

#17276 is a duplicate of this ticket.

comment:11 Changed 6 years ago by cjolif

Cc: cjolif added

comment:12 Changed 6 years ago by cjolif

I think we should at least be consistent, and so have _WidgetBase.on also lower-casing?

comment:13 Changed 6 years ago by bill

As I alluded to earlier that would break existing functionality for (admittedly unusual) cases like _WidgetBase.on("DOMMouseScroll", ...).

comment:14 Changed 6 years ago by cjolif

oh right, sorry for having missed that one. Maybe you can treat standard DOM events in a specific manner ;)

comment:15 Changed 6 years ago by bill

I agree that we should be consistent, and we will be consistent in 2.0, by making everything case sensitive. I just don't see an urgency to fix this behavior before then.

comment:16 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

comment:17 Changed 6 years ago by Mark DeMichele

I have a stupid suggestion for fixing this in <2.0

Why not just change the portion of code that calls addEventListener to lowercase the name. That way it will be consistent with the .emit. The event will be case insensitive, but at least we can make are event names look nice. Just a thought.

comment:18 Changed 6 years ago by bill

As I alluded to earlier that would break existing functionality for (admittedly unusual) cases like _WidgetBase.on("DOMMouseScroll", ...).

Note: See TracTickets for help on using tickets.