Page MenuHomePhabricator
Create Task
Maniphest T193062

Undo/redo on citations doesn't work correctly
Closed, ResolvedPublic
Actions

Description

  1. Change something in a citation.
  2. Click Undo.
  3. Click Redo.
  4. Click away. Why's that box still open?

It turns out that I can still edit (e.g., double-click another word and change its formatting), but I can't get rid of the box until I click the Cite button.

Reported by User:SUM1 at https://en.wikipedia.org/wiki/Wikipedia:VisualEditor/Feedback#Major_bug:_VisualEditor_stops_working_when_a_certain_sequence_of_operations_is_performed_to_a_citation

Details

SubjectRepoBranchLines +/-
Update VE core submodule to master (c4ad0a7e3)mediawiki/extensions/VisualEditormaster+1 -1
TreeModifier: Clone mutable linear data when insertingVisualEditor/VisualEditormaster+7 -3
Update VE core submodule to master (099902ddb)mediawiki/extensions/VisualEditormaster+1 -1
Copy attribute values when building transactionsVisualEditor/VisualEditormaster+18 -2
Customize query in gerrit

Related Objects

Event Timeline

• Whatamidoing-WMF created this task.Apr 25 2018, 6:00 PM
Restricted Application added a project: VisualEditor. · View Herald TranscriptApr 25 2018, 6:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
• Whatamidoing-WMF added a comment.Apr 26 2018, 3:01 PM
  1. Change something in a citation.
  2. Click Undo.
  3. Click Redo.
  4. Click away. Why's that box still open?
  5. Click the Edit button on the box. Now it won't open, and now I can't save, even if I can do basic text formatting.
• Elitre subscribed.Apr 26 2018, 3:02 PM
• Deskana triaged this task as High priority.Apr 26 2018, 3:09 PM
• Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.
• Deskana subscribed.

Following the above steps, when you click redo, there are two separate errors:

Uncaught Error: Removed data not as expected
    at checkExpected (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:401)
    at VeDmTreeModifier.ve.dm.TreeModifier.processRemove (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:402)
    at VeDmTreeModifier.ve.dm.TreeModifier.processOperation (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:395)
    at VeDmTreeModifier.ve.dm.TreeModifier.process (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:395)
    at VeDmTransactionProcessor.ve.dm.TransactionProcessor.process (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:347)
    at VeDmDocument.ve.dm.Document.commit (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:458)
    at VeDmSurface.ve.dm.Surface.changeInternal (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:420)
    at VeDmSurface.ve.dm.Surface.redo (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:422)
    at VeUiHistoryAction.ve.ui.HistoryAction.redo (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:886)
    at VeUiSurface.ve.ui.Surface.execute (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:818)
Uncaught TypeError: Cannot read property 'getInternalList' of null
    at VeDmMWReferenceNode.ve.dm.MWReferenceNode.getInternalItem (<anonymous>:742:871)
    at Object.ve.ui.MWCitationDialogTool.static.isCompatibleWith (<anonymous>:778:399)
    at collect (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:831)
    at VeUiContextItemFactory.ve.ui.ModeledFactory.getRelatedItems (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:831)
    at VeUiDesktopContext.ve.ui.LinearContext.getRelatedSources (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:827)
    at VeUiDesktopContext.ve.ui.LinearContext.isInspectable (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:826)
    at VeUiDesktopContext.ve.ui.LinearContext.afterContextChange (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:825)
    at VeUiDesktopContext.ve.ui.DesktopContext.afterContextChange (<anonymous>:1136:342)
Esanders added subscribers: dchan, Esanders.Apr 28 2018, 9:46 PM

Exception is inside TreeModifier, pinging @dchan

Esanders added a comment.Apr 28 2018, 10:14 PM

This bug has been present for at least a year, probably since TreeModifier was introduced, and possibly before that.

dchan added a comment.Apr 29 2018, 2:52 AM

Ooooh, this happens because ve.dm.TransactionBuilder#pushAttributeChanges( { key: val }, ... ) puts a live reference to val into the Transaction object. So if val subsequently gets mutated, that invalidates the Transaction object.

gerritbot subscribed.Apr 29 2018, 3:59 AM

Change 429649 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] Copy attribute values when building transactions

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

gerritbot added a project: Patch-For-Review.Apr 29 2018, 3:59 AM
Esanders added a comment.Apr 29 2018, 12:36 PM

Sounds about right, I assumed this was a shallow copy issue.

gerritbot added a comment.Apr 29 2018, 8:39 PM

Change 429649 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Copy attribute values when building transactions

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

Esanders assigned this task to dchan.Apr 29 2018, 9:10 PM
Esanders moved this task from Incoming to Code review on the VisualEditor (Current work) board.
• Deskana closed this task as Resolved.Apr 30 2018, 3:52 PM
• Deskana moved this task from Code review to Product owner review on the VisualEditor (Current work) board.
• Deskana edited projects, added MW-1.32-notes (WMF-deploy-2018-05-01 (1.32.0-wmf.2)); removed Patch-For-Review.

Testing on the beta cluster, that seems to have fixed it.

gerritbot added a comment.Apr 30 2018, 4:10 PM

Change 429832 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (099902ddb)

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

gerritbot added a project: Patch-For-Review.Apr 30 2018, 4:10 PM
gerritbot added a comment.Apr 30 2018, 5:09 PM

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

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

Jdforrester-WMF removed a project: Patch-For-Review.Apr 30 2018, 5:11 PM
Jdforrester-WMF set the point value for this task to 1.
Ryasmeen reopened this task as Open.May 1 2018, 8:35 PM
Ryasmeen subscribed.

I can still reproduce this on Beta cluster, re-opening it.

Ryasmeen added a comment.May 1 2018, 11:45 PM

Okay this does not happen when you try with basic reference but for all other types of citations it is reproducible 100% time.

dchan added a comment.May 2 2018, 9:10 AM

Hmm, thanks, it seems I overlooked that there are a few different ways reference-instead-of-copy can happen here - investigating further.

gerritbot added a comment.May 2 2018, 4:50 PM

Change 430413 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] TreeModifier: Clone mutable linear data when inserting

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

gerritbot added a project: Patch-For-Review.May 2 2018, 4:50 PM
dchan added a comment.May 2 2018, 4:53 PM

With change 430413, "change citation, undo, redo" seems to work fine in all the cases I've tried.

Ryasmeen added a comment.May 4 2018, 8:16 PM

Great! Can we merge the fix now?

gerritbot added a comment.May 7 2018, 9:47 AM

Change 430413 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] TreeModifier: Clone mutable linear data when inserting

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

gerritbot added a comment.May 7 2018, 3:22 PM

Change 431590 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c4ad0a7e3)

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

gerritbot added a comment.May 7 2018, 4:42 PM

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

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

Jdforrester-WMF removed a project: Patch-For-Review.May 7 2018, 4:43 PM
ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-05-08 (1.32.0-wmf.3)); removed MW-1.32-notes (WMF-deploy-2018-05-01 (1.32.0-wmf.2)).May 7 2018, 5:00 PM
Ryasmeen added a project: Verified.May 7 2018, 9:42 PM
• Deskana closed this task as Resolved.May 8 2018, 10:14 AM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMay 8 2018, 10:14 AM
• Vvjjkkii renamed this task from Undo/redo on citations doesn't work correctly to h8daaaaaaa.Jul 1 2018, 1:14 AM
• Vvjjkkii reopened this task as Open.
• Vvjjkkii removed dchan as the assignee of this task.
• Vvjjkkii added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).
• Vvjjkkii updated the task description. (Show Details)
• Vvjjkkii removed the point value for this task.
• Vvjjkkii removed subscribers: gerritbot, Aklapper.
Ryasmeen renamed this task from h8daaaaaaa to Undo/redo on citations doesn't work correctly.Jul 1 2018, 4:53 AM
Ryasmeen closed this task as Resolved.
Ryasmeen assigned this task to dchan.
Ryasmeen removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.
Ryasmeen updated the task description. (Show Details)
Ryasmeen added subscribers: GerritBot, Aklapper.
Aklapper edited subscribers, added: gerritbot; removed: GerritBot.May 16 2023, 10:18 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL