Opened 8 years ago
Closed 8 years ago
#16709 closed defect (wontfix)
The on() method is not properly working on dojox mobile editable widget ( e.g. : List and IconContainer and maybe some others)
Reported by: | Clement Mathieu | Owned by: | Adrian Vasiliu |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9.1 |
Component: | DojoX Mobile | Version: | 1.8.3 |
Keywords: | Cc: | Bill | |
Blocked By: | Blocking: |
Description
How to reproduce :
_ use a dojox/mobile/IconContainer with some icons and with the property "editable=true" like in dojox/mobile/tests/test_IconContainer-editable.html
_ Then try to replace the connect.connect(ic, "onDeleteItem", null, function(){}) by ic.on("deleteItem", function(){})
_ delete an item, the callback is not executed.
Cause
The ic.on call does not work properly because it tries to find a function onDeleteItem in the protoype of ic. But since this function is added to ic with a safeMixin, the function is added directly to the object not to its prototype.
Attachments (1)
Change History (10)
Changed 8 years ago by
Attachment: | ticket16709.patch added |
---|
comment:2 Changed 8 years ago by
sbrunot, could you please add a test? (manual or, better, automatic) Update: as a matter of fact, a manual test case is already included, sorry. (This modifies an existing test to use widget.on() instead of connect.connect(), but since there are many other IconContainer tests that still use connect.connect() both use-cases are covered.)
comment:3 Changed 8 years ago by
Milestone: | tbd → 1.9.1 |
---|---|
Owner: | changed from Eric Durocher to Adrian Vasiliu |
Status: | new → assigned |
comment:4 follow-up: 6 Changed 8 years ago by
Cc: | Bill added |
---|
See also the discussion in the duplicate #17161.
We now have to make our minds whether we go for the submitted ticket16709.patch, waiting to get rid of it in 2.0, as said in #17161. The concern here is the use of undocumented/private dijit APIs, and the "hacky"/fragile aspect of it. Bill, if you can take a look at it, your advice would help.
comment:6 Changed 8 years ago by
Replying to Adrian:
We now have to make our minds whether we go for the submitted ticket16709.patch, waiting to get rid of it in 2.0, as said in #17161. The concern here is the use of undocumented/private dijit APIs, and the "hacky"/fragile aspect of it. Bill, if you can take a look at it, your advice would help.
I looked at the patch. It scares me a little because it modifies this.constructor._onMap, affecting all instances of IconContainer or RoundRectList, regardless of whether or not they mix in editableMixinClass. But I guess it will work.
The pre-patch code also scares me, as I worry that declare.safeMixin() is not a valid way to add methods to a class instance, especially methods that call this.inherited(). The safe way is to do what data-dojo-mixins does: create a new subclass.
So, as I said in #17161, my instinct is to close this as wontfix and mark the editable and editableMixinClass properties as deprecated. But if you feel strongly about getting editableMixinClass to work again, you can check in the patch. It seems fragile, but I think it will work.
comment:7 Changed 8 years ago by
For the very same reasons as bill ones, I think it's safer not to fix it, and rely on the data-dojo-mixins workaround, which solves the issue also in this case:
<ul id="iconContainer" data-dojo-type="dojox.mobile.IconContainer" data-dojo-mixins="dojox/mobile/_EditableIconMixin" data-dojo-props="editable:true">
(note that 'editable' still need to be set to true, but the dynamic mixin will not be done).
comment:8 Changed 8 years ago by
Ok, thanks bill and pruzand for your input. Since we all think the same, and there is a workaround, and Dojo Mobile 2.0 will anyway solve it in a better way, I'm closing as "wontfix".
comment:9 Changed 8 years ago by
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
update the _onMap of a widget constructor after dynamically adding an editable mixin (IBM CCLA).