Opened 11 years ago

Closed 10 years ago

Last modified 8 years ago

#8224 closed defect (fixed)

dojo.html / dijit._Templated overlap in <table> code

Reported by: dante Owned by: Sam Foster
Priority: low Milestone: 1.4
Component: General Version: 1.2.1
Keywords: Cc: Sam Foster, bill
Blocked By: Blocking:

Description

dijit._Templated uses a pretty clever technique for building up tables, and a very similar technique is used in dojo.html for the contentsetter code. perhaps something generic could be extracted from the two to avoid the duplication.

Attachments (1)

setNodeContent_8224.patch (5.5 KB) - added by Sam Foster 10 years ago.
[CLA] [PATCH] new _setNodeContent implementation + UT, uses dojo.place but still supports nodelists and arbirary html strings

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by dante

Cc: Sam Foster added
Milestone: future1.4

comment:2 Changed 10 years ago by dante

Owner: changed from dante to Sam Foster
Type: enhancementdefect

_toDom fixed most of this, there is still table-walking code in dojo.html, but it seems most of that is duplicated and can defer to _toDom as well, then apply it's instantiation stuff?

comment:3 Changed 10 years ago by Sam Foster

Status: newassigned

Yeah, I've been meaning to revisit dojo.html now we have dojo.create and _toDom. To fix the _Templated overlap this will likely make dojo.html (or at least the ContentSetter?) a dependency, I assume that's ok (it's already a dependency of ContentPane?)

Changed 10 years ago by Sam Foster

Attachment: setNodeContent_8224.patch added

[CLA] [PATCH] new _setNodeContent implementation + UT, uses dojo.place but still supports nodelists and arbirary html strings

comment:4 Changed 10 years ago by Sam Foster

Resolution: fixed
Status: assignedclosed

(In [20192]) new dojo.html._setNodeContent implementation to leverage dojo.place and dojo._toDom, plus nodelist, mixed content unit tests. Fixes #8224

comment:5 Changed 10 years ago by Sam Foster

(In [20193]) Remove defunct shouldEmptyFirst 3rd param to dojo.html._setNodeContent Refs #8224

comment:6 Changed 10 years ago by Sam Foster

Cc: bill added

Would appreciate review on this, as it is code that ContentPane? depends on, so much used in the wild. The unit tests pass for this component, also dijit ContentPane? tests pass. There's a pre-existing -and I think unrelated- test failure in dojox.html test_set.html tests that I'll investigate seperately.

There's a couple new tests for setting node content with nodelists, and also mixed text & element html strings.

The 3rd shouldEmptyFirst param is removed as it was always true.

comment:7 Changed 10 years ago by Sam Foster

(In [20194]) DocumentFragments? can just pass thru to dojo.place, cleaning up enumerable handling. Refs #8224

comment:8 Changed 10 years ago by bill

Well it's definitely a lot shorter, so that's good.

This line looks suspicious:

if(!cont.nodeType && d.isArrayLike(cont)) {

I know that toDom() can return a Document or a DocumentFragment but I've never heard of it returning an array; you sure that's needed? (I could be wrong.)

Other than that, looks good to me.

comment:9 Changed 8 years ago by bill

(In [24255]) fix tabbing etc, refs #8224

comment:10 Changed 8 years ago by bill

(In [24256]) fix tabbing etc, refs #8224

Note: See TracTickets for help on using tickets.