Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16576 closed feature (fixed)

_AttachMixin: mixin to handle data-dojo-attach-point/event in template or on main page

Reported by: Nick Fenwick Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.3
Keywords: Cc: ben hockey, James Thomas
Blocked By: Blocking:

Description

dijit/_TemplatedMixin currently requires a templateString to operate, which it turns into DOM nodes and then parses for attach points, events, and so on. It would be very useful to be able to have the attach point magic without the need for a templateString.

The use case is when a templating engine, such as Catalyst, is generating HTML markup, and we want to instantiate a templated dijit around that markup. e.g.:

content.htm:

<html><body>
<div id="TheDijitContent">
  <p data-dojo-attach-point="foo">Hello</p>
</div>
</body></html>

If we had a _AttachMixin, for example:

var MyDijit = declare([ _WidgetBase, _AttachMixin ], {
    postCreate: function() {
        console.log("foo is now defined: ", this.foo);
    }
});

var mydijit = new MyDijit({}, dom.byId('TheDijitContent'));

So basically the ability to provide the template without having to bake the HTML string for the template into the dojo build. It brings the benefits of being able to dynamically generate the template using the templating engine (e.g. Catalyst).

My situation is that I'm porting a very old project using dojo 0.4.1 to 1.8.x. It makes extensive use of Catalyst to generate dialog content, among other things, and writing dijit code to handle these dialogs benefits greatly from the data-dojo-attach-* approach.

I'm currently using a custom mixin of my own which borrows some code from _TemplatedMixin and calls:

_TemplatedMixin.prototype._attachTemplateNodes.call(
    this.templateScope, this.domNode, function(n,p){ return n.getAttribute(p); });

hermo on IRC also had this problem and also had to cook up a custom solution. He ran into trouble with _WidgetsInTemplateMixin. A true solution would require some careful attention.

It seems there are two solutions:

  1. Change _TemplatedMixin so it doesn't require a templateString to do its work. If one is missing, simply attempt to use this.domNode as the base node.
  1. Split _TemplatedMixin into two. To avoid having to update the code for every dijit that currently extends _TemplatedMixin, we could possibly create a new parent mixin to _TemplatedMixin, something like _AttachMixin, that does the attach point work. Then people like us who want to just use the attach point code without needing a templateString would extend _AttachMixin rather than _TemplatedMixin.

Does this seem like something that will get attention from dojo committers? Is it worth coming up with working code and proposed patch?

Attachments (5)

patch.txt (16.8 KB) - added by Nick Fenwick 6 years ago.
git diff against dijit repo tag 1.8.3, _TemplatedMixin.js and _AttachMixin.js and test file.
patch2.txt (18.0 KB) - added by Nick Fenwick 6 years ago.
git diff against dijit repo tag 1.8.3, _TemplatedMixin.js and _AttachMixin.js and test file.
patch3.txt (22.2 KB) - added by Nick Fenwick 6 years ago.
Latest patch, against trunk, including all dijits and test file.
patch_against_trunk.txt (23.5 KB) - added by Nick Fenwick 6 years ago.
Work redone against trunk.
patch_against_trunk_with_attachscope.txt (27.4 KB) - added by Nick Fenwick 6 years ago.
As previous trunk patch, but with attachScope, improved test, and infrastructure-module.js mod.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 7 years ago by bill

It's an interesting idea. I've heard requests for server side instantiation before, which is beyond the scope of dijit, but we can provide support for it. If you can come up with working code and a proposed patch I'd definitely be interested.

comment:2 Changed 7 years ago by ben hockey

Cc: ben hockey added

Changed 6 years ago by Nick Fenwick

Attachment: patch.txt added

git diff against dijit repo tag 1.8.3, _TemplatedMixin.js and _AttachMixin.js and test file.

comment:3 Changed 6 years ago by Nick Fenwick

Finally found time to work on this. Please check patch.txt and see the new tests/test_AttachMixin.html file. I started with a tests file based on tests/CalendarLite.html because it tries to use doh/runner but I didn't notice that doh lives in util and couldn't get it working at the time. Should I base my test file on a doh test harness?

The work was extremely simple, I just ripped out the bits of _TemplatedMixin that deal with attach points and put them in a new base dijit, and arranged calls to this.inherited() appropriately. It was almost too easy, perhaps I missed something. I've tested the test_ComboBox.html for example and that templated widget still seems to operate correctly.

I haven't considered things like script blocks and other magic that might be in a templated widget.

I'd like to ask something about a possible extra feature.. in our app we sometimes have 'nested' content, for example a custom dijit that contains a TabContainer? where each pane of the TabContainer? is another custom dijit, containing various controls. So we have:

<div> -- parent dijit instance
  <div data-dojo-type='dijit/layout/TabContainer'>
    <div data-dojo-type='ourstuff/CleverPane'>
      <input data-dojo-attach-event='click: _clicked'>

We actually want the _clicked function to be on our parent dijit, not on the CleverPane dijit. In my app, I was able to provide a different 'this' reference when invoking _TemplatedMixin._attachTemplateNodes, by making our custom/_TemplatedContent dijit use .call():

this._attachTemplateNodes.call(
    this.templateScope, this.domNode, function(n,p){ return n.getAttribute(p); });

CleverPane extends our _TemplatedContent mixin. By providing a reference to the parent dijit in this.templateScope, I could get the _attachTemplateNodes code to actually assign the click handler to the parent dijit's _clicked() function.

Does this sound like something you guys would support in dijit?

comment:4 Changed 6 years ago by Nick Fenwick

I've found a flaw in my patch. A dijit that extends [ _WidgetBase, _AttachMixin, _WidgetsInTemplateMixin ] misses out attach points that are widgets, because I forgot that _TemplatedMixin does the call to _beforeFillContent which is where _WidgetsInTemplateMixin jumps in and runs the parser, and calls _attachTemplateNodes a second time for the parsed widgets.

So basically, _WidgetsInTemplateMixin is being ignored when you don't extend off _TemplatedMixin.

I'm fighting with how to make _AttachMixin provide this similar behaviour as _TemplatedMixin did, so you can have either [ _WidgetBase, _AttachMixin, _WidgetsInTemplateMixin ] or [ _WidgetBase, _TemplatedMixin, _WidgetsInTemplateMixin ], but it's proving thorny.

Changed 6 years ago by Nick Fenwick

Attachment: patch2.txt added

git diff against dijit repo tag 1.8.3, _TemplatedMixin.js and _AttachMixin.js and test file.

comment:5 Changed 6 years ago by Nick Fenwick

New patch above. I moved the _beforeFillContent call into _AttachMixin::buildRendering(), so it's invoked if you extend _TemplatedMixin or _AttachMixin.

The code is a little weird now in two ways:

  • _beforeFillContent and _fillContent used to be called sequentially in _TemplatedMixin::buildRendering. Now, _TemplatedMixin::buildRendering calls inherited, which will get _beforeFillContent invoked if necessary, and on return from that, calls _fillContent. Maybe that's not so bad.
  • _WidgetsInTemplateMixin is no longer strictly about 'templates' in the old sense.. it is detached now from relying on _TemplatedMixin, because you can extend either [ _TemplatedMixin, _WidgetsInTemplateMixin ] or [ _AttachMixin, _WidgetsInTemplateMixin ]. Perhaps it should be renamed to something like _WidgetsInAttachNodes but I'm struggling to find a catchy name. Ideas?

comment:6 in reply to:  3 Changed 6 years ago by bill

Replying to neek:

Finally found time to work on this. Please check patch.txt and see the new tests/test_AttachMixin.html file. I started with a tests file based on tests/CalendarLite.html because it tries to use doh/runner but I didn't notice that doh lives in util and couldn't get it working at the time. Should I base my test file on a doh test harness?

Cool. I looked quickly at the patch and it seems reasonable. Can you develop against trunk rather than the 1.8 branch/latest 1.8 release? Your patch didn't quite apply cleanly to trunk.

About doh, yes you should be using it. FYI, the default packageMap maps "doh" to "util/doh", which is why the test files just say require(["doh/runner"]) instead of require(["util/doh/runner"]).


The work was extremely simple, I just ripped out the bits of _TemplatedMixin that deal with attach points and put them in a new base dijit, and arranged calls to this.inherited() appropriately. It was almost too easy, perhaps I missed something. I've tested the test_ComboBox.html for example and that templated widget still seems to operate correctly.

Well, probably you didn't miss much (other than the stuff you mentioned in subsequent comments).


I haven't considered things like script blocks and other magic that might be in a templated widget.

Oof, I hope no one has <script> tags in their templates.


I'd like to ask something about a possible extra feature.. in our app we sometimes have 'nested' content, for example a custom dijit that contains a TabContainer where each pane of the TabContainer is another custom dijit, containing various controls. ... Does this sound like something you guys would support in dijit?

Yes, there's an open ticket for supporting nested widgets in templates, see #1983. I'd like it to work. Not sure if your change is the only thing needed to make it work or not.


New patch above. .. The code is a little weird now in two ways... _WidgetsInTemplateMixin is no longer strictly about 'templates' in the old sense.. it is detached now from relying on _TemplatedMixin, because you can extend either [ _TemplatedMixin, _WidgetsInTemplateMixin ] or [ _AttachMixin, _WidgetsInTemplateMixin ]. Perhaps it should be renamed to something like _WidgetsInAttachNodes but I'm struggling to find a catchy name. Ideas?

No ideas at the moment. _WidgetsInAttachNodes probably makes more sense than the current name but maybe it's not worth changing. Plus, technically the widgets being instantiated might not even be attach nodes.

Here's a question: if your server pre-expands the template for a _WidgetsInTemplatedMixin subclass, how do you stop the main parser run (running on <body>) from instantiating those supporting widgets, like the TabContainer and CleverPane in your example above?

And also, the parse() call in _WidgetsInTemplatedMixin shouldn't try to instantiate widgets inside of this.containerNode. In the current code that's achieved by running the parser before calling fillContent(). How about after your patch, does it still work right?

comment:7 Changed 6 years ago by Nick Fenwick

I've been super busy and not managed to address the concerns above in detail, yet. Project work is taking priority.

About your last point, how to stop the parser parsing widgets in pre-expanded content.. not sure I understand you, but I have run into a problem with dijit/Editor where the data-dojo-type attribute is left on its dom node after being parsed. This means that if you have some markup in the HTML served from the server that you want to use with _AttachMixin and _WidgetsInTemplateMixin to turn into a dijit of your own, for example:

In javascript my/funky/Stuff.js:

declare([ _WidgetBase, _AttachMixin, _WidgetsInTemplateMixin ], ....)

In markup:

<div data-dojo-type="my/funky/Stuff">
  <div data-dojo-type="dijit/Editor"></div>
</div>

..and parseOnLoad: true, then my/funky/Stuff will be instantiated on page load, which is correct, and while it is being parsed the Editor turned into a real editor, but the data-dojo-type is left on its DOM node which is still in the DOM. As the on-load parser continues on its run through the DOM, it runs straight into that inner div and picks up on the data-dojo-type still existing, tries to parse it, and we get an id clash.

I've made a sample sandbox demonstrating that data-dojo-type is left on dijit/Editor but not on, for example, dijit/form/Button: http://dojo-sandbox.net/neek/c8b61/1

So, question: can we call it a bug that data-dojo-type is left on nodes after parsing, so I can put in a removeAttribute() or similar call in my patch to remove it, which will prevent nested dijits in the template from being re-parsed?

comment:8 Changed 6 years ago by bill

I'm not sure if the parser leaving data-dojo-type on a node should be considered a bug or not. FYI, it's not that the data-dojo-type attribute is removed from the Button widget's DOM, it's that the Button widget uses a template, so the entire original DOM (including that data-dojo-type attribute) is thrown away and replaced by new DOM.

Anyway, you've got a template for my/funky/Stuff like:

<div>
   <div data-dojo-type="dijit/Editor" data-dojo-attach-point="editor"></div>
</div>

The Editor widget is part of my/funky/Stuff, so it should be instantiated by the parse() call in _WidgetsInTemplateMixin, rather than the top level parse() call on the whole page. That can be achieved by setting stopParser: true in the my/funky/Stuff prototype, just like ContentPane does. Or to follow James' advice, you could mixin something that sets that flag, i.e. the index.html file looks like:

<div data-dojo-type="my/funky/Stuff" data-dojo-mixin="PreExpandedTemplate">
  <div data-dojo-type="dijit/Editor"></div>
</div>

where PreExpandedTemplate defines stopParser: true.

Likewise, if Editor is pre-expanded it should do the same thing.


Now, if you want to pre-expand a template for a widget with a containerNode, things get more complicated. For example, say that my/funky/Stuff's template is:

<div>
   <span data-dojo-type="StuffSupportingWidget1></span>
   <span data-dojo-type="StuffSupportingWidget2></span>
   <div data-dojo-attach-point="containerNode"></div>
</div>

so that some original markup like:

<div data-dojo-type="my/funky/Stuff">
       <div data-dojo-type="dijit/Editor"></div>
</div>

gets pre-expanded to:

<div data-dojo-type="my/funky/Stuff">
   <span data-dojo-type="StuffSupportingWidget1></span>
   <span data-dojo-type="StuffSupportingWidget2></span>
   <div data-dojo-attach-point="containerNode">
       <div data-dojo-type="dijit/Editor"></div>
   </div>
</div>

In that case, I would suggest that my/funky/Stuff extend ContentPane. That sets the stopParser:true flag for my/funky/Stuff and also calls the parser recursively on the stuff inside this.containerNode. But also, when _WidgetsInTemplateMixin calls the parser, you need to make the parser ignores anything inside of this.containerNode. I think you can achieve that by _WidgetsInTemplateMixin setting:

this.containerNode.stopParser = true;

and then in the parser, wrap:

// Recurse, collecting <script type="dojo/..."> children, and also looking for
// descendant nodes with dojoType specified (unless the widget has the stopParser flag).
// When finished with children, go to my next sibling.
node = firstChild;
scripts = childScripts;
scriptsOnly = ctor && ctor.prototype.stopParser && !(options.template);
parent = current;

within an

    if(!node.stopParser){
      ...

Alternately, perhaps _WidgetsInTemplateMixin can call parser.instantiate() on the list of nodes that need to be instantiated, rather than parser.parse().

Last edited 6 years ago by bill (previous) (diff)

comment:9 Changed 6 years ago by bill

Cc: James Thomas added

PS: http://jamesthom.as/blog/2013/01/15/server-side-dijit/ is useful reading, although I don't think James tried to server-side render anything that extends _WidgetsInTemplateMixin. (Of course, the easy thing for you would be to not try to do that either.)

comment:10 Changed 6 years ago by Nick Fenwick

bill has pointed out that setting stopParser: true on your dijit prototype solves the problem nicely. I'm removing a long rant of mine here.

Now it would be nice to find a 'dijit' way of providing stopParser: true, perhaps by using a new mixin.

A further edit on this comment. I incorrectly thought I'd 'solved' the problem James had with monkey patching _TemplatedMixin. However, his problem was that he was instantiating dijit/form/Button, which via one of its bases extends _TemplatedMixin, and he was not in a position to use the new _AttachMixin if it had existed. He must work with the Button dijit hierarchy as-is, and so must work around _TemplatedMixin's intention to replace the dom nodes with its own.

Last edited 6 years ago by Nick Fenwick (previous) (diff)

comment:11 Changed 6 years ago by Nick Fenwick

So I think this work is pretty near completion and it's working well in our project.

Should I write a documentation page for _AttachMixin (or would someone else have that task) and if so, I guess http://livedocs.dojotoolkit.org/developer/metadoc is the guide to use.

comment:12 Changed 6 years ago by bill

Great, can you attach a patch against trunk?

And yes, please write a doc page too, according to that guide.

Changed 6 years ago by Nick Fenwick

Attachment: patch3.txt added

Latest patch, against trunk, including all dijits and test file.

comment:13 Changed 6 years ago by Nick Fenwick

Just attached a new patch, supercedes the other ones.

It ought to be against trunk unless I've buggered something up. It's diffed against commit 6ed79d3b2c1343c77a9f5c6a6f86a7217aa5f616 which is where my repo thinks the last commit was on trunk.

Tests file now uses doh/runner and has proper synthetic event firing to trigger event handlers and pre/post tests on values to ensure things happen as they should. All works for me.

5 tests to run in 1 groups _browserRunner.js:395
------------------------------------------------------------ _browserRunner.js:395
GROUP "_AttachMixin" has 5 tests to run _browserRunner.js:395
Attach points: [object HTMLHeadingElement], [object HTMLInputElement] test_AttachMixin.html:71
PASSED test: createWidgetWithAttachMagic 3 ms _browserRunner.js:395
PASSED test: createTemplatedAttachCombo 3 ms _browserRunner.js:395
PASSED test: createTemplatedAttachWidgetsCombo 7 ms _browserRunner.js:395
PASSED test: createAttachWidgetsCombo 5 ms _browserRunner.js:395
PASSED test: createAttachWidgetsCombo2 50 ms _browserRunner.js:395
Total time for GROUP " _AttachMixin " is  68ms _browserRunner.js:395
Total time for GROUP " _AttachMixin " is  68ms _browserRunner.js:395
WOOHOO!! _browserRunner.js:395
------------------------------------------------------------ _browserRunner.js:395
| TEST SUMMARY: _browserRunner.js:395
------------------------------------------------------------ _browserRunner.js:395
	 5 tests in 1 groups _browserRunner.js:395
	 0 errors _browserRunner.js:395
	 0 failures

comment:14 Changed 6 years ago by bill

Milestone: tbd1.9

Thanks for the automated test, it looks good. It should be added to dijit/tests/infrastructure-module.js.

About the code itself, unfortunately you rolled back changes from _TemplatedMixin that happened after 1.8. You also removed the dependency on dojo/_base/unload that's used by IE.

Also, the module: ... comment in _AttachModule is wrong, and it's missing a summary for the class.

Last edited 6 years ago by bill (previous) (diff)

Changed 6 years ago by Nick Fenwick

Attachment: patch_against_trunk.txt added

Work redone against trunk.

Changed 6 years ago by Nick Fenwick

As previous trunk patch, but with attachScope, improved test, and infrastructure-module.js mod.

comment:15 Changed 6 years ago by Nick Fenwick

Just added two more patches.. only one is really relevant.

patch_against_trunk.txt - same as previous work, but done against trunk from scratch instead of ported over from 1.8.x branch work (I admit that was stupid) patch_against_trunk_with_attachscope.txt - has 'attachScope' added to _AttachMixin.

attachScope seems to work well. Note the added magic in this._attachPoints, we need to store the scope of each attach point so we may properly remove the reference on destruction. I've added a test that destroys an inner dijit 'id' and ensure that the outer dijit 'od' has the attach point 'field' removed from it properly. I hope you find this extra dance acceptable. It does change the structure of _attachPoints which may break some existing code. An alternative solution might be to store scope information for attach points in a separate array, but this would be purely to maintain the _attachPoint data structure and seems overkill for a private data structure.

I added a Summary and other stuff you mentioned, please give it another gander.

comment:16 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

In [30611]:

Split _TemplatedMixin data-dojo-attach-point and data-dojo-attach-event logic to separate _AttachPoint mixin, so we may have attach-points in HTML markup, i.e. for widgets rendered on the server. Patch from Nicholas Fenwick (CLA on file), thanks! Fixes #16576 !strict.

comment:17 Changed 6 years ago by Nick Fenwick

I think I've found a serious problem with this patch. The line in _TemplatedMixin used to be:

this._attachEvents.push(this.connect(baseNode, touch[event] || event, thisFunc));

I've noticed that dijit/Dialog close buttons no longer work, and they relied on ondijitclick events, which use _OnDijitClickMixin which overrides connect() for dijits. This code in _AttachMixin now uses the line:

this._attachEvents.push(this.own(on(baseNode, event, lang.hitch(_attachScope, thisFunc)))[0]);

This no longer uses this.connect and so the ondijitclick events fail.

The problem with using this.connect was that it calls into _WidgetBase::connect where 'this' is hardcoded as the scope::

return this.own(connect.connect(obj, event, this, method))[0];	// handle

We need to use this.attachScope at that point.

Last edited 6 years ago by Nick Fenwick (previous) (diff)

comment:18 Changed 6 years ago by Nick Fenwick

Simple fix?

this._attachEvents.push(this.own(this.connect(baseNode, event, lang.hitch(_attachScope, thisFunc)))[0]);

comment:19 Changed 6 years ago by bill

Closing Dialogs works fine for me. _AttachMixin.js has the following code to handle ondijitclick:

event = event.replace(/^on/, "").toLowerCase();
if(event == "dijitclick"){
	event = a11yclick || (a11yclick = require("./a11yclick"));
}else{
	event = synthEvents[event] || event;
}

comment:20 Changed 6 years ago by bill

@neek - Thanks for starting the manual page for this. If you are writing examples it's probably easier to do it locally (so you can test the examples) and then submit the updates as a pull request to https://github.com/dojo/docs.

comment:21 Changed 6 years ago by bill

PS: I updated the version of dijit in https://github.com/phiggins42/rstwiki to include _AttachMixin. Not sure when that change gets reflected to livedocs, which is why I mentioned running rstwiki locally to test your example.

comment:22 Changed 6 years ago by Nick Fenwick

Thanks for the advice bill, the docs clone and rst work went well. I've issued a pull request https://github.com/dojo/docs/pull/73

comment:23 Changed 6 years ago by bill

In [30733]:

Fix _AttachMixin to (by default) not scan DOMNodes inside this.containerNode. Also modify _AttachMixin tests that assumed the opposite behavior. Leaving _TemplatedMixin with the old behavior for backwards compatibility of widget like _ComboBoxMenu that have templates with markup inside of this.containerNode. Fixes #16781, refs #16576 !strict.

comment:24 Changed 6 years ago by bill

BTW, the other use for _AttachMixin is to support other (client-side) rendering engines better. For example, a MustacheTemplated mixin (supporting mustache templates) would probably want to extend _AttachMixin rather than extending _TemplatedMixin.

comment:25 Changed 6 years ago by bill

In [30771]:

Move _WidgetsInTemplateMixin specific code from _AttachMixin to _WidgetsInTemplateMixin. Refs #16576 tangentially, !strict.

comment:26 Changed 6 years ago by bill

In [30779]:

Move _WidgetsInTemplateMixin tests from _AttachMixin.html to _WidgetsInTemplateMixin.html. Also removing a _TemplatedMixin test that seems redundant with tests already in _TemplatedMixin.html. Refs #16576.

comment:27 Changed 6 years ago by bill

In [30789]:

Fix _WidgetsInTemplateMixin to not parse widgets inside of this.containerNode (except if this.searchContainerNode is set to true). Refs #16576 !strict.

comment:28 Changed 6 years ago by bill

In [30801]:

IE gives exceptions in console if you try to delete an expando property that doesn't exist, refs #16576 !strict.

comment:29 Changed 6 years ago by bill

Summary: Split _TemplatedMixin into two, so we may have attach-points in HTML markup_AttachMixin: mixin to handle data-dojo-attach-point/event in template or on main page
Note: See TracTickets for help on using tickets.