Page MenuHomePhabricator

Inspector crashes while trying to open it in a position where another inspector was opened previously
Closed, ResolvedPublic8 Story Points


Steps to reproduce:

1.Open a page with VE
2.Open link inspector and add a link there
3.Delete the link by pressing backspace
4.Now while the cursor at the start position of that line, open any inspector.

Observed Result:
The inspector crashes upon opening, cannot close it anymore
And the following error appears in the console:
Error: Offset could not be translated to a DOM element and offset: 22

Event Timeline

Ryasmeen created this task.Aug 26 2015, 7:08 PM
Ryasmeen raised the priority of this task from to Needs Triage.
Ryasmeen updated the task description. (Show Details)
Ryasmeen added a project: VisualEditor.
Ryasmeen added a subscriber: Ryasmeen.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 26 2015, 7:08 PM

This happens for all inspectors, if you delete any previous node for example: Gallery/Link/Math/Language and try to open another link inspector , it will get stuck in that position

Jdforrester-WMF triaged this task as High priority.Sep 1 2015, 7:08 PM
Jdforrester-WMF set Security to None.
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
DLynch added a subscriber: DLynch.Oct 14 2015, 4:37 PM

I went off looking, on the assumption that it might be something to do with the link annotations leaving unexpected stuff in the DOM. However, it turns out that it's easy to trigger without any use of links.

  1. start a new paragraph
  2. type something
  3. press enter to start a new paragraph
  4. press backspace until what you typed is deleted (DOM now contains: <p><br></p>)
  5. ...and now opening any inspector while your cursor is there will cause the error

The "bad state" seems to be: an paragraph containing no text, which isn't marked up with a slug. getNodeAndOffsetUnadjustedForUnicorn can't handle that.

If you click to move your cursor anywhere else, the document state will be fixed up via empty link nodes being removed and slugs being added to now-empty paragraphs.

dchan added a subscriber: dchan.Oct 14 2015, 9:09 PM

We should probably fix this at both ends:

  1. Refactor getNodeAndOffset so it doesn't crash in slugless textless paragraphs. (I expect this to be fairly straightforward)
  1. Fix ve.ce.Surface#handleLinearDelete so that it doesn't create slugless textless paragraphs. This doesn't completely guarantee that slugless textless paragraphs will never happen, because IMEs can remove text without triggering handleLinearDelete, at times when it's not safe to modify the surface (because the IME might get closed prematurely), but at least then the problem would only hit people using pathological IMEs.

So maybe split this into two tasks?

Change 246702 had a related patch set uploaded (by DLynch):
Run renderContents before finding nodes by offset

So, that's one approach to task 1. It's sort of the brute-force approach, but it does let me ignore the question of what should be returned as the node in those cases, by pushing the document to a rendered-correctly status and just returning what's already used.

The alternative seems like it'd be to return the node that rendering removes (generally a <br> or an empty <a>), or to find an empty text-node in there and return that. The fact that we return the slug nodes already seems to suggest that returning things which aren't strictly in the data-model is okay, but I don't know how much other code out there is going "is this a slug? okay, special-case treatment!"

Change 246702 merged by jenkins-bot:
Run renderContents before finding nodes by offset

Change 247090 had a related patch set uploaded (by Divec):
getNodeAndOffset: handle slugless nodes with zero DM length

Change 247090 merged by jenkins-bot:
getNodeAndOffset: handle slugless nodes with zero DM length

Ryasmeen reopened this task as Open.Oct 29 2015, 12:06 AM

This is still happening when you delete the link along with that empty blue box that appears even after deleting it

See the screen capture:

Esanders closed this task as Resolved.Oct 29 2015, 4:40 PM
Esanders added a subscriber: Esanders.

Bug described in last comment is T114064