Page MenuHomePhabricator

Check preventing identical re-renderings in renderContents doesn't work if node contains slugs
Closed, ResolvedPublic8 Story Points

Description

ve.ce.ContentBranchNode#renderContents contains the following code (some lines removed for brevity):

	rendered = this.getRenderedContents();
	// Return if unchanged. Test by building the new version and checking DOM-equality.
	// However we have to normalize to cope with consecutive text nodes. We can't normalize
	// the attached version, because that would close IMEs.
	oldWrapper = this.$element[0].cloneNode( true );
	newWrapper = this.$element[0].cloneNode( false );
	while ( rendered.firstChild ) {
		newWrapper.appendChild( rendered.firstChild );
	}
	ve.normalizeNode( oldWrapper );
	ve.normalizeNode( newWrapper );
	if ( newWrapper.isEqualNode( oldWrapper ) ) {
		return false;
	}

If the node contains slugs, oldWrapper will contain these (because it's a clone of this.$element) but newWrapper won't (because slugs are added not by getRenderedContents but by setupSlugs, which is called later), so .isEqualNode() will never return true.

This could probably be circumvented by removing slugs from oldWrapper, but I wonder how much we really need this check. It seems like we only need it because sometimes a rerender is forced not because a transaction happened but for unicorn-related reasons, and if the unicorn situation turns out not to have changed, we shouldn't make a no-op modification to the DOM. That sounds somewhat sensible, but I wonder if 1) this can ever really occur and 2) this couldn't be detected by comparing transaction and unicorn states.

Event Timeline

Catrope created this task.Mar 2 2015, 10:31 AM
Catrope assigned this task to dchan.
Catrope raised the priority of this task from to Needs Triage.
Catrope updated the task description. (Show Details)
Catrope added a subscriber: Catrope.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 2 2015, 10:31 AM

https://gerrit.wikimedia.org/r/#/c/193788/ is somewhat related: it prevents this check from being performed on the first render

Jdforrester-WMF renamed this task from Check preventing identical rerenderings in renderContents doesn't work if node contains slugs to Check preventing identical re-renderings in renderContents doesn't work if node contains slugs.Mar 2 2015, 3:37 PM
Jdforrester-WMF triaged this task as High priority.
Jdforrester-WMF set Security to None.
dchan added a comment.Mar 25 2015, 8:34 AM

See https://gerrit.wikimedia.org/r/198705/ , which relates to this bug (without fixing it).

dchan added a comment.EditedSep 25 2015, 6:57 PM

The renderContents method will get triggered when text input gets analysed as a complex change (i.e. not a simple insertion or a simple removal). This might be because an IME has changed candidate text, or inserted text without the selection being collapsed. If so, then an unwanted rerender will potentially be triggered, and potentially kill the IME.

Change 241124 had a related patch set uploaded (by Divec):
Ignore slugs and chimeras when preventing identical re-renderings

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

Change 241124 merged by jenkins-bot:
Ignore slugs and chimeras when preventing identical re-renderings

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