Page MenuHomePhabricator

[Regression pre-wmf.18] Clicking on a slug positioned after References list or before a map produces error "Uncaught TypeError: Cannot read property 'getModel' of undefined"
Closed, ResolvedPublic

Description

  1. Insert a References list or a map.
  2. Click on the slug created right after the References list or before the map.

The following error appears:

Uncaught TypeError: Cannot read property 'getModel' of undefined.

I am unable to reproduce this on test2 or production, so assuming this is a regression. Feel free to remove the tag if it's not.

Event Timeline

I can't reproduce this on Beta or locally.

Ok, I tried to reproduce this, it happens only when the slug is between a map and a code block/graph/hiero.

and incase of references list, when it follows those nodes.

Couple of screenshots showing the position of the slug and the nodes:

Screen Shot 2018-08-20 at 11.45.07 PM.png (740×1 px, 208 KB)

Screen Shot 2018-08-20 at 11.21.28 PM.png (737×1 px, 227 KB)

Screen Shot 2018-08-20 at 11.41.49 PM.png (701×1 px, 231 KB)

This is very inconsistent to reproduce. I fiddled with the editor for ten minutes before I triggered the error, and I have no idea what I did to cause it. I got it with the hiero-reflist case, when clicking on the slug after the reflist.

The error comes from this line in ve.ce.getOffsetOfSlug:

model = $element.prev().data( 'view' ).getModel();

At this point, $element.prev() is a <div class="ve-ce-cursorHolder ve-ce-cursorHolder-after" …>…</div> and its .data( 'view' ) is null, but apparently the code is expecting to find the <div class="… ve-ce-mwReferencesListNode …">…</div>, which is the previous node. I don't know if the cursorHolder should have been removed, or if we should skip over it. I'm not even sure what it is for, heh.

I'll give up here for now, cursor stuff sounds like @dchan's area.

In case it helps (and maybe saves you some time trying to reproduce), I dumped the DM and CE I had when I encountered the issue:

> JSON.stringify(ve.init.target.surface.model.documentModel.data.data)
"[{"type":"mwHiero","attributes":{"mw":{"name":"hiero","attrs":{},"body":{"extsrc":"\nb\n"}}}},{"type":"/mwHiero"},{"type":"mwReferencesList","attributes":{"listGroup":"mwReference/","refGroup":"","isResponsive":true}},{"type":"/mwReferencesList"},{"type":"internalList"},{"type":"/internalList"}]"
> ve.init.target.surface.view.$element.html()
"<div class="ve-ce-branchNode ve-ce-documentNode mw-content-ltr mw-parser-output" contenteditable="true" spellcheck="true" data-gramm="false" lang="en" dir="ltr"><div class="ve-ce-branchNode-slug ve-ce-branchNode-blockSlug"><span class="oo-ui-widget oo-ui-widget-enabled oo-ui-buttonElement oo-ui-buttonElement-frameless oo-ui-iconElement oo-ui-labelElement oo-ui-buttonWidget" aria-disabled="false"><a class="oo-ui-buttonElement-button" role="button" tabindex="0" aria-disabled="false" rel="nofollow"><span class="oo-ui-iconElement-icon oo-ui-icon-add"></span><span class="oo-ui-labelElement-label">Insert paragraph</span><span class="oo-ui-indicatorElement-indicator oo-ui-indicatorElement-noIndicator"></span></a></span></div><table class="mw-hiero-table mw-hiero-outer ve-ce-leafNode ve-ce-focusableNode" dir="ltr" typeof="mw:Extension/hiero" about="#mwt3" data-mw="{&quot;name&quot;:&quot;hiero&quot;,&quot;attrs&quot;:{},&quot;body&quot;:{&quot;extsrc&quot;:&quot;\nb\n&quot;}}" id="mwAQ" contenteditable="false"><tbody><tr><td><table class="mw-hiero-table"><tbody><tr> <td><img style="margin: 1px;" src="https://en.wikipedia.beta.wmflabs.org/w/extensions/wikihiero/img/hiero_D58.png?12f84" height="35" title="D58 [b]" alt="b"></td></tr></tbody></table></td></tr></tbody></table><span about="#mwt3" class="ve-ce-leafNode ve-ce-focusableNode" contenteditable="false">
</span><div class="ve-ce-branchNode-slug ve-ce-branchNode-blockSlug"><span class="oo-ui-widget oo-ui-widget-enabled oo-ui-buttonElement oo-ui-buttonElement-frameless oo-ui-iconElement oo-ui-labelElement oo-ui-buttonWidget" aria-disabled="false"><a class="oo-ui-buttonElement-button" role="button" tabindex="0" aria-disabled="false" rel="nofollow"><span class="oo-ui-iconElement-icon oo-ui-icon-add"></span><span class="oo-ui-labelElement-label">Insert paragraph</span><span class="oo-ui-indicatorElement-indicator oo-ui-indicatorElement-noIndicator"></span></a></span></div><div class="ve-ce-leafNode ve-ce-focusableNode ve-ce-mwReferencesListNode ve-ce-focusableNode-focused" contenteditable="false"><p class="ve-ce-mwReferencesListNode-muted">There are no references on this page to include in this list.</p></div><div class="ve-ce-cursorHolder ve-ce-cursorHolder-after" contenteditable="true"><img class="ve-ce-cursorHolder-img"></div><div class="ve-ce-branchNode-slug ve-ce-branchNode-blockSlug"><span class="oo-ui-widget oo-ui-widget-enabled oo-ui-buttonElement oo-ui-buttonElement-frameless oo-ui-iconElement oo-ui-labelElement oo-ui-buttonWidget" aria-disabled="false"><a class="oo-ui-buttonElement-button" role="button" tabindex="0" aria-disabled="false" rel="nofollow"><span class="oo-ui-iconElement-icon oo-ui-icon-add"></span><span class="oo-ui-labelElement-label">Insert paragraph</span><span class="oo-ui-indicatorElement-indicator oo-ui-indicatorElement-noIndicator"></span></a></span></div></div><div class="ve-ce-surface-paste noime" tabindex="-1" contenteditable="true">☢</div>"

@matmarex: I am actually getting this pretty much everyday, almost every single time when I click on the slug that appears after adding a reflist. I just hope it's not that harmful, at-least I have not seen any bad effect from front-end perspective.

Looks like we assume $element.prev() is the previous focusable, but in the case given (insert hiero then insert reflist) the previous node is a ve-ce-cursorholder.

Change 463691 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] ve.ce.getOffsetOfSlug: Only match leaf/branch nodes when traversing

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

Reproduction steps:

  • Open an empty document
  • Insert a reflist
  • Click the slug after it
  • Insert another reflist
  • Clikc the slug after it

Change 463691 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.ce.getOffsetOfSlug: Only match leaf/branch nodes when traversing

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

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

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

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

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