Opened 8 years ago
Closed 7 years ago
#16482 closed defect (fixed)
Regression: _WidgetBase#placeAt() can't be used on a DocumentFragment
Reported by: | Bryan Forbes | Owned by: | Bryan Forbes |
---|---|---|---|
Priority: | high | Milestone: | 1.10 |
Component: | Dijit | Version: | 1.8.0 |
Keywords: | Cc: | ben hockey | |
Blocked By: | Blocking: |
Description
var frag = document.createDocumentFragment(); widget.placeAt(frag);
This will fail because reference.tagName
is checked in placeAt
which causes refWidget
to be set to the document fragment. In the else condition, ref
is set to undefined
because refWidget
is truthy and both containerNode
and domNode
are undefined on the fragment. This should be checking for appendChild
or nodeType
rather than tagName
. This is also a regression because this used to work in 1.7.
Change History (13)
comment:1 Changed 8 years ago by
Component: | General → Dijit |
---|---|
Milestone: | tbd → 1.8.3 |
Owner: | set to bill |
Priority: | undecided → high |
Status: | new → assigned |
comment:2 Changed 8 years ago by
Cc: | ben hockey added |
---|
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
Owner: | changed from bill to Bryan Forbes |
---|---|
Status: | assigned → pending |
Version: | 1.8.2rc1 → 1.8.0 |
Hmm, placeAt() isn't documented to be able to work on a DocumentFragment. The API doc says it takes a Widget, DOMNode, or id of widget or DOMNode. I guess you are saying you want _WidgetBase.placeAt(docFrag) it to call through to domConstruct.place(), but domConstruct.place() is also not documented to work with a DocumentFragment.
This will fail because reference.tagName is checked in placeAt which causes refWidget to be set to the document fragment.
I'm not following you there. The code checking tagName is:
var refWidget = !reference.tagName && registry.byId(reference);
Regardless of the first clause in the expression, refWidget will be null (or undefined), because registry.byId(frag) will return nothing.
comment:5 Changed 8 years ago by
Status: | pending → new |
---|
byId: function(/*String|Widget*/ id){ // summary: // Find a widget by it's id. // If passed a widget then just returns the widget. return typeof id == "string" ? hash[id] : id; // dijit/_WidgetBase },
Since id
is not a string (it's a DocumentFragment), it is returned. The same would happen with a DOMNode if not for the tagName
check.
comment:6 Changed 8 years ago by
Status: | new → assigned |
---|
Funny how registry.byId() can return something other than a widget.
Anyway, I talked to Bryan about this over IM. If he adds DOH tests for dojo.place() and _WidgetBase.placeAt() to work with a DocumentFragment target (in addition to an Element target), and updates the API docs to say that a DocumentFragment is also allowed, I'm OK with the suggested change.
Apparently, a DocumentFragment target is supposed to work exactly the same as an unattached Element target: the position parameter can be "first", "last"/undefined, or a number, but not "before", "after", or "replace".
comment:7 Changed 8 years ago by
Milestone: | 1.8.3 → 1.8.4 |
---|
comment:8 Changed 8 years ago by
Milestone: | 1.8.4 → 1.8.5 |
---|
Moving incomplete issues to next minor release.
comment:9 Changed 8 years ago by
Milestone: | 1.8.5 → 1.8.6 |
---|
1.8.5 is released; moving all outstanding tickets to next milestone.
comment:10 Changed 7 years ago by
Milestone: | 1.8.6 → 1.8.7 |
---|
1.8.6 is released; moving all outstanding tickets to next milestone.
comment:12 Changed 7 years ago by
Milestone: | 1.8.7 → 1.10 |
---|
comment:13 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This broke as part of the change in [28251].