Opened 7 years ago

Closed 7 years ago

#17002 closed defect (fixed)

Regression with widgets in template having attach events

Reported by: Brian Arnold Owned by: Brian Arnold
Priority: blocker Milestone: 1.9
Component: Dijit Version: 1.9.0b2
Keywords: Cc: cjolif, Ed Chatelain
Blocked By: Blocking:

Description

I'm providing both a demonstration and a one-line patch on this ticket, which shows the issue we've recently encounter with templated widgets that also have widgets in their template.

If a templated widget contains a widget, and tries to use an attach event to bind to that event, if that event is one that comes in from dijit/_Widget's DOM-like events, the event does not bind correctly.

After doing some digging, it looks like it's how back-compat was implemented in dijit/_WidgetsInTemplateMixin. In _beforeFillContent, it follows this logic when dealing with attach events:

        // callback to do data-dojo-attach-event to a widget
        if(type in widget){
                // back-compat, remove for 2.0
                return aspect.after(widget, type, callback, true);
        }else{
                // 1.x may never hit this branch, but it's the default for 2.0
                return widget.on(type, callback, true);

In the event of using something like this:

<div data-dojo-type="dijit/form/TextBox" data-dojo-attach-event="onClick: doSomething"></div>

It'll fall into that first branch and use aspect.after. However, aspect.after isn't a valid replacement for the older connect method from dojo/_base/connect. Semantically they're similar, but dijit/_Widget is adding an aspect around connect.connect and kernel.connect that vivifies the event upon binding, and aspect.after is left clean, so the event is bound but it's never actually fired.

In the demo I'm uploading, you'll notice that I'm using both a Button and a TextBox?. The Button fires its onClick, but that's because dijit/form/_ButtonMixin explicitly defines an onClick method, rather than leaning on the automagic one from dijit/_Widget. The TextBox?, on the other hand, doesn't ever actually fire its onClick since it wasn't brought to life.

The fix is a simple one. It's a one-line change that seems to be working well, and is valid since it's a back-compat adjustment anyways:

        // callback to do data-dojo-attach-event to a widget
        if(type in widget){
                // back-compat, remove for 2.0
                return widget.connect(widget, type, callback);
        }else{
                // 1.x may never hit this branch, but it's the default for 2.0
                return widget.on(type, callback, true);

By replacing the aspect with widget.connect, the event comes to life as it used to in 1.8 and prior.

The attached demonstration of the bug is a simple bugdemo package, to be put as a sibling of dojo etc. Works well out of the box with 1.8, completely broken against current trunk. With this one-line adjustment applied locally, everything works well again.

I'm also attaching a patch file that cleanly applies against trunk right now to make the change.

Normally I wouldn't feel bad about making a one-line change, but given that it's in the heart of a fairly important Dijit, I figured I'd pass it through a ticket for your review, Bill. Thoughts?

Attachments (2)

bugdemo.zip (1.6 KB) - added by Brian Arnold 7 years ago.
Demonstration of widgets in template with attach events not working well against Dojo trunk
_WITM-backcompat.patch (595 bytes) - added by Brian Arnold 7 years ago.
A one-line patch for _WidgetsInTemplateMixin to restore behavior of widgets with attach events

Download all attachments as: .zip

Change History (6)

Changed 7 years ago by Brian Arnold

Attachment: bugdemo.zip added

Demonstration of widgets in template with attach events not working well against Dojo trunk

Changed 7 years ago by Brian Arnold

Attachment: _WITM-backcompat.patch added

A one-line patch for _WidgetsInTemplateMixin to restore behavior of widgets with attach events

comment:1 Changed 7 years ago by bill

Owner: changed from bill to Brian Arnold
Status: newassigned

Thanks, that all makes sense. Feel free to check in if you can add a samll test into _WidgetsInTemplateMixin.html. I guess you would need to on.emit() an event to tst that the hookup really worked.

comment:2 Changed 7 years ago by cjolif

Cc: cjolif added

comment:3 Changed 7 years ago by Ed Chatelain

Cc: Ed Chatelain added

comment:4 Changed 7 years ago by Brian Arnold

Resolution: fixed
Status: assignedclosed

In [31268]:

Changing how back-compat is handle for attach events, fixes #17002

Note: See TracTickets for help on using tickets.