Page MenuHomePhabricator

"ve.dm.Document.getBranchNodeFromOffset(): offset -1 is out of bounds" error
Closed, ResolvedPublic

Description

This is observed while trying to select image in "White tie" article on enwiki. It's much easier to reproduce when same article is loaded as source page in ContentTranslation.

Event Timeline

Change 441091 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.dm.Document: Handle no valid offsets in #getNearestCursorOffset

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

I am fixing a symptom here, but maybe there is a more interesting underlying issue here. As far as I can tell, what happens here is that the browser gives us a selection starting between the <figure> and <figcaption> tags, and when trying to fix it up to a valid selection in our data model, VE explodes because neither this offset, nor any offset before it in the document, are a valid place for a selection to start. We already have code to handle this case and find an offset after instead, but it was never reached due to the exception.

@dchan I did find a more interesting underlying issue.

It seems there is a bug in ve.dm.ElementLinearData#getRelativeOffset in the code which is supposed to skip over nodes "whose children should be ignored", such as figures (ve.dm.BlockImageNode). When given an invalid offset at the beginning or end of such a node (inside it), and asked to cursor outside of it, it will fail to find any valid offsets and return -1.

This limitation is actually described in the code comments: "This clearly won't work if you start inside such a node, but you shouldn't be doing that to being with". Well, we do that, ever since we allowed inline editing of figure captions (ve.dm.BlockImageCaptionNode) ;)

It should probably try the the other direction first, like it does normally, and only fail if there are no valid offsets inside at all?

The result is usually okay most of the time, since ve.dm.Document#getNearestCursorOffset also explicitly checks both directions, so as long as we fix the exception (which my patch does) the user-visible behavior will be correct. But we might want to think about this.

I added/commented some test cases in the patch, documenting the current weird behavior.

Change 441091 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.dm.Document: Handle no valid offsets in #getNearestCursorOffset

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

Change 442202 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (fab33197b)

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

Change 442202 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (fab33197b)

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

Deskana closed this task as Resolved.Jun 27 2018, 2:39 PM
Deskana assigned this task to matmarex.
Deskana added a subscriber: Deskana.

If the immediate problem has been fixed—which it appears it has—then I'm calling this resolved. Any other or bigger issues should be filed in other tasks.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJun 27 2018, 2:39 PM
Vvjjkkii renamed this task from "ve.dm.Document.getBranchNodeFromOffset(): offset -1 is out of bounds" error to 0oaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed matmarex as the assignee of this task.
Vvjjkkii raised the priority of this task from Medium to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
Petar.petkovic renamed this task from 0oaaaaaaaa to "ve.dm.Document.getBranchNodeFromOffset(): offset -1 is out of bounds" error.Jul 1 2018, 11:08 PM
Petar.petkovic closed this task as Resolved.
Petar.petkovic assigned this task to matmarex.
Petar.petkovic lowered the priority of this task from High to Medium.
Petar.petkovic updated the task description. (Show Details)
Petar.petkovic added a subscriber: Aklapper.