Last modified: 2013-10-23 15:20:48 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T56675, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 54675 - VisualEditor: Render lock when changing selection prevents inspectors' changes from rendering
VisualEditor: Render lock when changing selection prevents inspectors' change...
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
ContentEditable (Other open bugs)
unspecified
All All
: High normal
: VE-deploy-2013-10-24
Assigned To: Roan Kattouw
:
Depends on:
Blocks: 54335
  Show dependency treegraph
 
Reported: 2013-09-27 01:09 UTC by Roan Kattouw
Modified: 2013-10-23 15:20 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Roan Kattouw 2013-09-27 01:09:59 UTC
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).
Comment 1 D Chan 2013-09-27 23:29:47 UTC
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.
Comment 2 Gerrit Notification Bot 2013-10-21 19:24:56 UTC
Change 90957 had a related patch set uploaded by Catrope:
Defer selection-triggered updates in ve.ui.Context

https://gerrit.wikimedia.org/r/90957
Comment 3 Gerrit Notification Bot 2013-10-23 15:20:20 UTC
Change 90957 merged by jenkins-bot:
Defer selection-triggered updates in ve.ui.Context

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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links