Page MenuHomePhabricator

VisualEditor: Render lock when changing selection prevents inspectors' changes from rendering
Closed, ResolvedPublic

Description

This is the root cause of bug 54335. Basically, you open the language inspector (or the link inspector, for that matter), make a change, then close the inspector by clicking elsewhere into the document (as opposed to by using the arrow or by pressing enter or escape in the link inspector's text input).

Narrated call stack:

  • A mouseup event fires on the document
  • ve.ce.Surface.onDocumentMouseUp() starts the observer and polls
  • The observer notices that the selection has changed and emits a selectionChange event, which ends up invoking ve.ce.Surface.onSelectionChange
  • onSelectionChange sets a render lock and changes the model selection
  • ve.dm.Surface.change() emits a change event
  • ve.ui.Context.onChange() notices that the selection changed while an inspector was visible, so it closes the inspector
  • ve.ui.AnnotationInspector.onClose() saves the changes the user made to the model, by indirectly calling ve.dm.Surface.change()
  • (at this point, we have a change() call stack frame nested inside another change() frame, which is always a bad sign)
  • The transaction processed by onClose() annotates text, which causes an update event to be emitted
  • ve.ce.ContentBranchNode.onChildUpdate() responds to this event and calls renderContents()
  • renderContents() checks to see if the surface is locked for rendering; it is, so it bails and doesn't render the change

When I briefly talked to Trevor about this issue, he said something about emitting an event asynchronously. I dismissed it at the time, because it would just move both problems (having to lock to prevent the model normalizing the selection / event storms, but having to not lock to allow inspector changes to render), but now I think about it I think it has merit. We could have ve.ui.Context.onChange() asynchronously close the inspector, from a setTimeout(). That would avoid the nested change() thing, and it would allow the render lock to be lifted before the inspector is closed.

Alternatively, onSelectionChange could only lock against selection changes and still allow transactions. But the nested change() seems like a code smell anyway, the order of event handlers might get messed up for instance, and the caller would observe multiple changes from calling change() once (that's the root of the problem here).


Version: unspecified
Severity: normal

Details

Reference
bz54675

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 2:19 AM
bzimport set Reference to bz54675.
Catrope created this task.Sep 27 2013, 1:09 AM

From #mediawiki-visualeditor IRC with RoanKattouw:

  • It seems wrong to implement click-out-to-close by detecting a selection change.
  • Should we bind to document mousedown instead? We need to avoid tripping over other mouse handling of course.

Aside from this,

  • In general DM selection changes should emit something less drastic than 'change', because we don't want them to get back to the CE layer (which has a somewhat different set of possible cursor positions).
  • I (David) am currently working on a fairly big refactor of ve.ce.Surface will include this.

Change 90957 had a related patch set uploaded by Catrope:
Defer selection-triggered updates in ve.ui.Context

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

Change 90957 merged by jenkins-bot:
Defer selection-triggered updates in ve.ui.Context

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