Opened 9 years ago
Closed 9 years ago
#16288 closed defect (fixed)
dojox/form/manager update connect to on
| Reported by: | dylan | Owned by: | dylan |
|---|---|---|---|
| Priority: | high | Milestone: | 1.9 |
| Component: | DojoX Form | Version: | 1.8.1 |
| Keywords: | MID, AMD | Cc: | bill, Kitson Kelly |
| Blocked By: | Blocking: |
Description
Update dojox/form/manager to use dojo/on instead of dojo/_base/connect, and clean-up the logic for how it manages connections
Attachments (11)
Change History (33)
Changed 9 years ago by
| Attachment: | patch_16288.diff added |
|---|
comment:1 Changed 9 years ago by
Started a patch file on this one. The test is such a mess for pre-AMD best practices that it's not fun so far.
Changed 9 years ago by
| Attachment: | patch_16288.2.diff added |
|---|
Patch including start of a new test file
comment:2 Changed 9 years ago by
| Status: | new → assigned |
|---|
comment:3 Changed 9 years ago by
It occurs to me that the test file is very similar to that found in the tutorial at http://dojotoolkit.org/documentation/tutorials/1.8/form_manager/demo/manager.php
Will work to merge that code shortly.
Changed 9 years ago by
| Attachment: | patch_16288.3.diff added |
|---|
More work on cleaning up the code and testing it
comment:4 Changed 9 years ago by
| Cc: | wildbill Kitson Kelly added |
|---|---|
| Keywords: | MID AMD added |
I'm close on this, but running into a pair of issues:
1) the parser is failing on this:
<div> <input type="radio" id="e021" name="e02" value="e02-1" data-dojo-observer="showValues"> <label for="e021">e02-1 input/radio</label> <input type="radio" id="e022" name="e02" value="e02-2" checked="checked" data-dojo-observer="showValues"> <label for="e022">e02-2 input/radio</label> </div>
if I change the name to the second item to anything unique, parsing works fine.
2) None of the Dijit events are captured. I'm guessing this is due to my attempt to normalize event names for on, by removing "on" from the event names, compared with using connect before.
Kitson and Bill, I could use help on this one, as the test case works ok with 1.8.3. Main changes are switching to using MIDs, changing to data-dojo-observer instead of observer, and changing the form code.
comment:5 Changed 9 years ago by
The only thing that strikes me immediately is that code like this makes no sense:
arrayUtil.forEach(c, c.remove);
"c" is an array, and arrays don't have a remove method. So it needs to be:
arrayUtil.forEach(c, function(item){ item.remove(); });
or something equivalent like
while(c.length){ c.pop().remove(); }
I think I saw that type of mistake in multiple places.
comment:6 Changed 9 years ago by
I am looking at the work done so far and attempting to finish this one up.
comment:7 Changed 9 years ago by
This one should be good to go. Like Bill suggested, there were a few places throughout that needed a switch to
arrayUtil.forEach(c, function(item){ item.remove(); });
There were also some other minor issues that were left over from the initial dojo/on conversion.
Changed 9 years ago by
| Attachment: | patch_16288.6.diff added |
|---|
comment:8 Changed 9 years ago by
Hmmm, I think your diff is missing something @bitphsr, as it's still failing my AMD test (which I didn't see in your patch).
comment:9 Changed 9 years ago by
I just updated the patch to include the AMD test page and re-verified it by applying it from the root of a fresh dojox/ pull. Hopefully this one will be good.
comment:10 Changed 9 years ago by
It looks like the latest patch was working up until http://bugs.dojotoolkit.org/changeset/30733/dojo. I will look into what changed and post an updated patch.
comment:11 Changed 9 years ago by
| Cc: | bill added; wildbill removed |
|---|
Ah, I tried to keep backwards compatibility for _attachTemplateNodes(), but wasn't expecting anyone to call it like:
_TemplatedMixin.prototype._attachTemplateNodes.call(...)
Probably defining
searchContainerNode: true
in dojox.form.Manager will fix it, although as I tried to imply in #16781, searching for attach points and events inside of this.containerNode is fragile and could break in the future.
BTW you can list links to changesets in trac simply like [30733].
Changed 9 years ago by
| Attachment: | patch_16288.9.diff added |
|---|
Patch fixing dojox/form/Manager inheritance and FormManager? tests
comment:12 Changed 9 years ago by
[30733] introduces a new _processNodes method on dijit/_AttachMixin, which is extended by _TemplatedMixin. Rather than extending _Templated or _TemplatedMixin, dojox/form/Manager manually does the following in its buildRendering method:
_TemplatedMixin.prototype._attachTemplateNodes.call(this, node, function(n, p){ return n.getAttribute(p); });
This call to _attachTemplateNodes in turn makes a call to this._processNodes, which doesn't exist due to a lack of proper _AttachMixin inheritance. It also had logic to prevent recursive calls to the destroyRecursive method on _TemplatedMixin:
destroyRendering: function(preserveDom){ // ctm: calling _TemplatedMixin if(!this.__ctm){ // avoid recursive call from _TemplatedMixin this.__ctm = true; _TemplatedMixin.prototype.destroyRendering.apply(this, arguments); delete this.__ctm; this.inherited(arguments); } }
By making Manager extend _AttachMixin, the overridden destroyRendering method is no longer needed, and the parse error is no longer thrown. Further, dijit/form/Select was failing an instanceof FormWidget check. This was changed to use isInstanceOf.
The latest patch should take care of the above issues as well as cleaning up this demo page per the original ticket.
comment:13 Changed 9 years ago by
That all sounds good, a much better design. Two things I noticed though:
Looking at _Mixin.js and _NodeMixin.js in your patch, not sure why you changed "array" to "arrayUtil", but more importantly, they are both calling this.remove when AFAIK there is no such function.
About Manager.js, do you really need these lines?
this._attachPoints = []; this._attachEvents = [];
I think all you need is something like
if(!this.containerNode){
// all widgets with descendants must set containerNode
this.containerNode = this.domNode;
}
Also, you have an unused dependency on _TemplatedMixin.
comment:14 Changed 9 years ago by
PS: Perhaps a bit outside the scope of this ticket, but in addition to extending _AttachMixin, why not also extend _WidgetsInTemplateMixin? The form is likely to contain widgets in addition to plain DOMNodes, and you want the data-dojo-attach-point's to work for them too.
I see you are using data-dojo-observer for that, but if you extend _WidgetsInTemplateMixin then (theoretically) data-dojo-attach-point will work too, and the data-dojo-observer code could be removed or deprecated.
comment:15 Changed 9 years ago by
Thanks for the review Bill (and the continued efforts on this Paul)
For array to arrayUtil, all of our docs tell people to use arrayUtil as the name for this module, as array could confuse people with the native name (Array).
As noted, We should update reliance on _Widget to _WidgetBase and _WidgetsInTemplateMixin
this.remove... honestly, this code is very difficult to follow, it was intended to replace things like this:
array.forEach(this.formWidgets[name].connections, this.disconnect, this);
There are a few different forms of that, so we need to look at each and try to understand what the original code was actually doing.
I have no idea if this._attachPoints = []; this._attachEvents = []; is actually used for anything. Would be fine with the proposed alternative assuming it works (which it should).
_TemplatedMixin does look like it can be removed now, with Paul's changes.
It would be worthwhile to see if we could remove data-dojo-observer by extending _WidgetsInTemplateMixin, though I would not want to break people using observer from past versions of Dojo, so we should do that and deprecate observer/data-dojo-observer as a separate issue.
Changed 9 years ago by
| Attachment: | patch_16288.10.diff added |
|---|
Cleans up tests, fixes Manager inheritance, etc.
comment:16 follow-up: 17 Changed 9 years ago by
- this.remove
The this.remove code has been cleaned up - Bill was correct, FormManager's destroy( ) method was blowing up on that very code. This has been fixed throughout using code similar to what was suggested above:
arrayUtil.forEach(c, function(item){ item.remove(); });
- _WidgetsInTemplateMixin
Good catch - this is now extended by dojox/form/Form (in addition the extending _AttachMixin). The unused _TemplatedMixin dependency was removed, as were following two lines:
this._attachPoints = []; this._attachEvents = [];
- Deprecating observer/data-dojo-observer
I didn't touch this yet - Bill's suggestion seems like it'd work fine, using data-dojo-attach-point instead now that we have proper _WidgetsInTemplateMixin inheritance. I would be happy to take this - do we want to open up another ticket as Dylan mentioned?
comment:17 Changed 9 years ago by
Replying to dylan:
For array to arrayUtil, all of our docs tell people to use arrayUtil as the name for this module, as array could confuse people with the native name (Array).
No, they don't, see https://raw.github.com/dojo/docs/master/dojo/_base/array.rst and https://raw.github.com/dojo/docs/master/releasenotes/migration-2.0.rst
Replying to bitpshr:
- Deprecating observer/data-dojo-observer
I didn't touch this yet - Bill's suggestion seems like it'd work fine, using
data-dojo-attach-pointinstead now that we have proper_WidgetsInTemplateMixininheritance. I would be happy to take this - do we want to open up another ticket as Dylan mentioned?
Looks like you need to set the searchContainerNode flag to true, right?
About opening another ticket, if you are just talking about supporting data-dojo-attach-point there may be nothing to do other than updating the test case. But feel free to open another ticket if you like.
Speaking of data-dojo-attach-point, this comment you added doesn't really make sense because there's no change planned for Dijit 2.0 about data-dojo-props, and I thought we were talking about data-dojo-attach-point anyway:
// This allows us to use data-dojo-observer, but doesn't // support data-dojo-props, which won't be supported on nested // widgets until Dijit 2.0, see // http://bugs.dojotoolkit.org/ticket/12820
Your patch still has a few lines that should be removed, but looks basically good (I didn't try it though). The lines to remove are:
var node = (this.domNode = this.srcNodeRef);
and
// TODO: Select is failing `instanceof FormWidget` check
comment:18 follow-up: 19 Changed 9 years ago by
Most of our tutorials, and original versions of those docs told people to use arrayUtil rather than array. I wish we would actually communicate changes to promoted best practices.
That comment was something that I added a while back... it's a different issue (nested attributes using data-dojo-props prevents us from parsing nested components with fully HTML5-compliant markup). Surely that's going to change for Dojo 2?
comment:19 Changed 9 years ago by
Replying to dylan:
Most of our tutorials, and original versions of those docs told people to use arrayUtil rather than array. I wish we would actually communicate changes to promoted best practices.
No, the original versions are https://raw.github.com/dojo/docs/1ae63052615def7b91dbdb47b4429ac21c8fedea/dojo/_base/array.rst and http://oksoclap.com/ep/pad/view/fj6YzSGsJ3/ATBnCIw3iO, and use "array".
That comment was something that I added a while back... it's a different issue (nested attributes using data-dojo-props prevents us from parsing nested components with fully HTML5-compliant markup). Surely that's going to change for Dojo 2?
See #12820. It's unclear what or where will change for Dojo 2.0 but regardless, the comment doesn't make sense because Form doesn't even want to use data-dojo-props.
comment:20 Changed 9 years ago by
This latest patch should take care of Bill's remaining comments. For the sake of sticking the original intent of this ticket, I didn't remove the data-dojo-observer code.
comment:22 Changed 9 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Landed, thanks @bitpshr for helping finish this one off. I'm closing it out.
Start of a patch file