Page MenuHomePhabricator

getNodeAndOffset bug causes the cursor to jump after the close of a non-focusable node
Closed, ResolvedPublic8 Estimate Story Points

Description

Steps to reproduce:

  1. Edit the following HTML:
<h1><a href='x'><i>A<span rel='ve:Alien'>X</span></i>B</a>x</h1>
  1. Press Right Arrow three times. The 'X' should be selected.
  2. Press Right Arrow once more.

Observed behaviour: The cursor jumps back to before the 'A' (inside the link).

Expected behaviour: The cursor should be just after the 'X'.

Cause:

The getNodeAndOffset function returns incorrect values for the offset after the close of a non-focusable node. In this example, the linear model is:

[ '<heading>', 'A', '<alienInline>', '</alienInline>', 'B', 'x', '</heading>' ]

For offset 4 (immediately after the alienInline), the position returned is:

{node: <i class="ve-ce-textStyleAnnotation ve-ce-italicAnnotation">, offset: 0}

This is definitely wrong, since it is back at the start of the containing element (hence the cursor jump).

Details

Related Gerrit Patches:
VisualEditor/VisualEditor : masterWIP: Fix node and offset calculations
VisualEditor/VisualEditor : masterRefactor getNodeAndOffset

Event Timeline

DLynch created this task.Jan 21 2016, 6:33 PM
DLynch raised the priority of this task from to Needs Triage.
DLynch updated the task description. (Show Details)
DLynch added subscribers: DLynch, dchan.
Restricted Application added a project: VisualEditor. · View Herald TranscriptJan 21 2016, 6:33 PM
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald Transcript
Jdforrester-WMF triaged this task as Medium priority.Jan 26 2016, 8:28 PM
Jdforrester-WMF moved this task from To Triage to TR3: Language support on the VisualEditor board.
Jdforrester-WMF set Security to None.

Change 267292 had a related patch set uploaded (by Divec):
WIP: Fix node and offset calculations

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

dchan added a comment.Jan 31 2016, 9:05 PM

Change 267292 shows one way to fix this getNodeAndOffset bug. But would be cleaner to consider systematically the different node configurations getNodeAndOffset should handle, and the "best" return value for each configuration, then rewrite getNodeAndOffset[UnadjustedForUnicorn] from scratch accordingly.

Use of getNodeAndOffset

ve.ce.Document#getNodeAndOffset maps each model offset to a DOM position (node+offset). But the mapping model offset->DOM position is one-many, so in some cases we need to consider which DOM position we'd like returned.

ve.ce.Document#getNodeAndOffset is called for the following uses:

  • In ve.ce.Surface#focus to focus the ContentEditable
  • In ve.ce.Surface#getFocusedNodeDirectionality to get the (logically) focused node
  • In ve.ce.Surface#getSelectionState to build a SelectionState from a ve.Range

ve.ce.Surface#getSelectionState is called for the following uses:

  • In ve.ce.Surface#showModelSelection to apply the DM selection to the DOM
  • In ve.ce.Surface#getNativeRange to get the native Range of the current selection

ve.ce.Surface#getNativeRange is called for the following uses:

  • In ve.init.Target#scrollCursorIntoView for bounding rectangle calculation
  • In ve.ce.LinearSelection#getSelectionRects for bounding rectangle calculation
  • In ve.ce.LinearSelection#getSelectionBoundingRect for bounding rectangle calculation

The choice of DOM position only matters in one of these cases: showModelSelection. There it affects active annotations, link entry, cursoring,
highlighting, etc. Therefore we can make the choice to optimise for the most useful selections being produced.

dchan added a comment.Feb 2 2016, 8:37 AM

I've looked at this in detail, and I'm working on a cleaner rewrite on the following lines:

  1. Step with ve.adjacentDomPosition until we hit a position at the correct offset (which is guaranteed to be the first such position in document order).
  2. Use ve.adjacentDomPosition( ..., { skipSoft: false } ) to return all subsequent positions at the same offset.
  3. Examine these possible positions and pick accordingly, looking around a little further if necessary (e.g. to find matching unicorns).

Change 270315 had a related patch set uploaded (by Divec):
WIP: Refactor getNodeAndOffset

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

dchan added a comment.Mar 2 2016, 10:51 AM

The above patch fixes the offset calculation bug. (Cursoring rightwards off the focusable node of the example HTML is still glitchy, but for unrelated reasons)

dchan renamed this task from getNodeAndOffset has incorrect returns if given the offset after the close of a non-focusable node to getNodeAndOffset bug causes the cursor to jump after the close of a non-focusable node.Mar 25 2016, 4:27 PM
dchan updated the task description. (Show Details)

Change 270315 merged by jenkins-bot:
Refactor getNodeAndOffset

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

Jdforrester-WMF closed this task as Resolved.Mar 26 2016, 12:42 AM

Change 267292 abandoned by Divec:
WIP: Fix node and offset calculations

Reason:
Rewrote getNodeAndOffset in I8b833feee813b10004fc9f1cf862a6fdbf7e1634 instead of merging this quick fix.

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