Page MenuHomePhabricator

De-Houdini-fy the handling of observed DOM changes
Closed, ResolvedPublic8 Story Points

Description

The structural straitjacket of ve.ce.Surface#handleObservedChanges has provoked some contorted escapology:

  • It handles content changes before range changes (escaped by catapulting to some vague future moment with setTimeout)
  • It infers prior cursor positions with respect to annotations by looking at the state at the last keydown event; but there may not have *been* a keydown event (e.g. with IME text input).
  • It hides central pure functional logic (building transactions from observed state changes) inside a method that has side effects, forcing some contortion on unit tests.

Refactor it to be clearer, mundaner and less heroic.

Event Timeline

dchan created this task.Sep 25 2015, 8:05 PM
dchan claimed this task.
dchan raised the priority of this task from to Normal.
dchan updated the task description. (Show Details)
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 25 2015, 8:05 PM

Change 241144 had a related patch set uploaded (by Divec):
WIP: De-Houdinify the handling of observed DOM changes

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

Esanders updated the task description. (Show Details)Sep 25 2015, 9:09 PM
Esanders set Security to None.
Jdforrester-WMF renamed this task from De-Houdinify the handling of observed DOM changes to De-Houdini-fy the handling of observed DOM changes.Sep 26 2015, 1:23 PM
Jdforrester-WMF edited a custom field.
Jdforrester-WMF moved this task from To Triage to TR3: Language support on the VisualEditor board.

Change 241144 merged by jenkins-bot:
De-Houdini-fy the handling of observed DOM changes

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

DLynch added a subscriber: DLynch.Oct 21 2015, 1:07 AM

This change seems to have introduced a bug reproducible via triggering sequences: open a new document, type * , observe that you get • * instead of just , with the lingering * not being in the linear model.