Opened 7 years ago

Closed 6 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)

patch_16288.diff (7.1 KB) - added by dylan 7 years ago.
Start of a patch file
patch_16288.2.diff (16.3 KB) - added by dylan 7 years ago.
Patch including start of a new test file
patch_16288.3.diff (21.9 KB) - added by dylan 6 years ago.
More work on cleaning up the code and testing it
patch_16288.4.diff (22.0 KB) - added by dylan 6 years ago.
Revised patch, cleans-up the test case
patch_16288.5.diff (12.0 KB) - added by bitpshr 6 years ago.
no more errors, finalizing cleanup
patch_16288.6.diff (23.3 KB) - added by dylan 6 years ago.
patch_16288.7%0A.diff (22.5 KB) - added by bitpshr 6 years ago.
Adding the amd test page
patch_16288.8.diff (22.5 KB) - added by bitpshr 6 years ago.
Fixing filename
patch_16288.9.diff (24.5 KB) - added by bitpshr 6 years ago.
Patch fixing dojox/form/Manager inheritance and FormManager? tests
patch_16288.10.diff (24.7 KB) - added by bitpshr 6 years ago.
Cleans up tests, fixes Manager inheritance, etc.
patch_16288.11.diff (19.5 KB) - added by bitpshr 6 years ago.
Cleans up dependencies and comments

Download all attachments as: .zip

Change History (33)

Changed 7 years ago by dylan

Attachment: patch_16288.diff added

Start of a patch file

comment:1 Changed 7 years ago by dylan

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 7 years ago by dylan

Attachment: patch_16288.2.diff added

Patch including start of a new test file

comment:2 Changed 7 years ago by dylan

Status: newassigned

comment:3 Changed 7 years ago by dylan

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 6 years ago by dylan

Attachment: patch_16288.3.diff added

More work on cleaning up the code and testing it

Changed 6 years ago by dylan

Attachment: patch_16288.4.diff added

Revised patch, cleans-up the test case

comment:4 Changed 6 years ago by dylan

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">
	&nbsp;<label for="e021">e02-1 input/radio</label>
	&nbsp;
	<input type="radio" id="e022" name="e02" value="e02-2" checked="checked" data-dojo-observer="showValues">
	&nbsp;<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 6 years ago by bill

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 6 years ago by bitpshr

I am looking at the work done so far and attempting to finish this one up.

Changed 6 years ago by bitpshr

Attachment: patch_16288.5.diff added

no more errors, finalizing cleanup

comment:7 Changed 6 years ago by bitpshr

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 6 years ago by dylan

Attachment: patch_16288.6.diff added

comment:8 Changed 6 years ago by dylan

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).

Changed 6 years ago by bitpshr

Attachment: patch_16288.7%0A.diff added

Adding the amd test page

Changed 6 years ago by bitpshr

Attachment: patch_16288.8.diff added

Fixing filename

comment:9 Changed 6 years ago by bitpshr

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 6 years ago by bitpshr

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 6 years ago by bill

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 6 years ago by bitpshr

Attachment: patch_16288.9.diff added

Patch fixing dojox/form/Manager inheritance and FormManager? tests

comment:12 Changed 6 years ago by bitpshr

[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 6 years ago by bill

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 6 years ago by bill

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 6 years ago by dylan

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 6 years ago by bitpshr

Attachment: patch_16288.10.diff added

Cleans up tests, fixes Manager inheritance, etc.

comment:16 Changed 6 years ago by bitpshr

  1. 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(); });
  1. _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 = [];
  1. 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 in reply to:  16 Changed 6 years ago by bill

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:

  1. 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?

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 Changed 6 years ago by 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.

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 in reply to:  18 Changed 6 years ago by bill

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.

Changed 6 years ago by bitpshr

Attachment: patch_16288.11.diff added

Cleans up dependencies and comments

comment:20 Changed 6 years ago by bitpshr

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:21 Changed 6 years ago by dylan

In [30780]:

refs #16288 #16289 #13449, AMD clean-up and refactor for dojox/form/manager

comment:22 Changed 6 years ago by dylan

Resolution: fixed
Status: assignedclosed

Landed, thanks @bitpshr for helping finish this one off. I'm closing it out.

Note: See TracTickets for help on using tickets.