Page MenuHomePhabricator

Repeatedly infusing the same node generates a new Widget every time
Closed, ResolvedPublic

Description

Repeatedly infusing the same node generates a new Widget every time. The documentation we have does not seem to actually specify that the same Widget will be returned on repeated OO.ui.infuse() calls, but I think that's what we'd been assuming, and that's what the code seems to be intending to do.

Try the following console commands on https://doc.wikimedia.org/oojs-ui/master/demos/widgets.php:

e = document.getElementById( 'ooui-0' )
w1 = OO.ui.infuse( e )
w2 = OO.ui.infuse( e )
w1 === w2 // Returns false!

Consider the following snippet from OO.ui.Element.static.unsafeInfuse:

	data = $elem.data( 'ooui-infused' );
	if ( data ) {
		// cached!
		if ( data === true ) {
			throw new Error( 'Circular dependency! ' + id );
		}
		return data;
	}
	...
	$elem.data( 'ooui-infused', true ); // prevent loops
	...
	obj = new cls( data ); // rebuild widget
	// now replace old DOM with this new DOM.
	if ( top ) {
		$elem.replaceWith( obj.$element );
	}
	obj.$element.data( 'ooui-infused', obj );

There are two problems here:

  • We never update the data on $elem to point to the newly constructed widget.
  • $elem.replaceWith(…) clears all data, including the true marker we set, that would otherwise cause a loud error.

This is not a problem when infusing by id, since we get the new element then (as the original has been removed from DOM), which has the right data set.

Event Timeline

matmarex created this task.Jul 14 2015, 8:05 PM
matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex added a project: OOUI.
matmarex added subscribers: matmarex, cscott.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 14 2015, 8:05 PM

I think you just need to throw a loud error if the element you give to
infuse does not have an ownerDocument (or has a different ownerDocument
than the current page's document). The element has been removed from the
document, it's not valid to infuse it again.

You can add to the documentation that the argument to infuse must be in the
current document, and that the infusion process *may* (is not guaranteed
to) remove the argument element from the document (but that ids are
preserved). In the future we may be more clever about modifying some DOM
trees in place, rather than rebuilding them from scratch, but I don't think
our API should guarantee either behavior exclusively.

(For the record, this can result in broken infusions in MediaWiki, although no code on current master seems affected. It did affect new code that I've been writing, which is how I found it. Workaround is https://gerrit.wikimedia.org/r/224719.)

Jdforrester-WMF triaged this task as Medium priority.Jul 16 2015, 7:36 PM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Change 229668 had a related patch set uploaded (by Bartosz Dziewoński):
Element: DWIM when repeatedly infusing the same node

https://gerrit.wikimedia.org/r/229668

Change 229671 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "mediawiki.page.ready: When infusing, infuse by id, not by element"

https://gerrit.wikimedia.org/r/229671

matmarex updated the task description. (Show Details)Aug 6 2015, 9:16 AM
matmarex set Security to None.
Jdforrester-WMF moved this task from Backlog to Doing on the OOUI board.Aug 7 2015, 6:22 PM
Jdforrester-WMF closed this task as Resolved.Aug 11 2015, 8:16 PM
Jdforrester-WMF assigned this task to matmarex.

Change 229668 merged by jenkins-bot:
Element: DWIM when repeatedly infusing the same node

https://gerrit.wikimedia.org/r/229668

Change 229671 merged by jenkins-bot:
Revert "mediawiki.page.ready: When infusing, infuse by id, not by element"

https://gerrit.wikimedia.org/r/229671

Jdforrester-WMF moved this task from Doing to Reviewing on the OOUI board.Aug 26 2015, 1:38 AM