Merge pull request #1225 from substance/footnote-fixes

Numerous bug fixes.

app/main.js

@@ -284,7 +284,7 @@ function _createMenu () {
               }
             },
             {
-              label: 'Figure Pacakge',
+              label: 'Figure Package',
               click () {
                 _openNew('figure-package')
               }

src/article/ArticleAPI.js

@@ -6,6 +6,7 @@ import renderEntity from './shared/renderEntity'
 import FigurePanel from './models/FigurePanel'
 import SupplementaryFile from './models/SupplementaryFile'
 import { BlockFormula } from './models'
+import { INTERNAL_BIBR_TYPES } from './ArticleConstants'
 
 export default class ArticleAPI {
   constructor (editorSession, archive, config, articleSession, contextProvider) {
@@ -207,18 +208,22 @@ export default class ArticleAPI {
     })
   }
 
-  _removeReference (ref) {
-    this.editorSession.transaction(tx => {
-      documentHelpers.remove(tx, ['article', 'references'], ref.id)
-      const referenceManager = this._articleSession.getReferenceManager()
-      referenceManager._getXrefs().forEach(xref => {
-        const index = xref.refTargets.indexOf(ref.id)
-        if (index > -1) {
-          tx.update([xref.id, 'refTargets'], { type: 'delete', pos: index })
-        }
-      })
-      documentHelpers.deepDeleteNode(tx, ref)
-      tx.selection = null
+  // This method is used to cleanup xref targets
+  // during footnote or reference removing
+  _removeCorrespondingXrefs (tx, node) {
+    let manager
+    if (INTERNAL_BIBR_TYPES.indexOf(node.type) > -1) {
+      manager = this._articleSession.getReferenceManager()
+    } else if (node.type === 'footnote') {
+      manager = this._articleSession.getFootnoteManager()
+    } else {
+      return
+    }
+    manager._getXrefs().forEach(xref => {
+      const index = xref.refTargets.indexOf(node.id)
+      if (index > -1) {
+        tx.update([xref.id, 'refTargets'], { type: 'delete', pos: index })
+      }
     })
   }
 

src/article/converter/r2t/FigurePanelConverter.js

@@ -153,7 +153,7 @@ export default class FigurePanelConverter {
     for (let i = L - 1; i >= 0; i--) {
       let child = children[i]
       if (!child.is('p')) {
-        let p = el.createElement('p').attr('specific-use', 'display-element-wrapper')
+        let p = el.createElement('p').attr('specific-use', 'display-element-wrapper').append(child.clone(true))
         el.replaceChild(child, p)
       }
     }

src/article/converter/r2t/FootnoteConverter.js

@@ -1,4 +1,4 @@
-import { findChild, findAllChildren } from '../util/domHelpers'
+import { findAllChildren } from '../util/domHelpers'
 import { getLabel } from '../../shared/nodeHelpers'
 
 // TODO: at some point we want to retain the label and determine if the label should be treated as custom
@@ -8,12 +8,9 @@ export default class FootnoteConverter {
 
   get tagName () { return 'fn' }
 
+  // NOTE: we don’t support custom labels at the moment, so we will ignore input from fn > label
   import (el, node, importer) {
-    let labelEl = findChild(el, 'label')
     let pEls = findAllChildren(el, 'p')
-    if (labelEl) {
-      node.label = labelEl.text()
-    }
     node.content = pEls.map(el => importer.convertElement(el).id)
   }
 

src/article/editor/EditorPackage.js

@@ -65,7 +65,7 @@ import {
 } from './TableCommands'
 import InsertTableTool from './InsertTableTool'
 import OpenFigurePanelImageTool from '../shared/OpenFigurePanelImageTool'
-import RemoveItemCommand from './RemoveItemCommand'
+import RemoveItemCommand from '../shared/RemoveItemCommand'
 import ReplaceFigurePanelTool from '../shared/ReplaceFigurePanelTool'
 import ReplaceSupplementaryFileCommand from './ReplaceSupplementaryFileCommand'
 import ReplaceSupplementaryFileTool from './ReplaceSupplementaryFileTool'

src/article/metadata/MetadataPackage.js

@@ -31,6 +31,7 @@ import {
   AddCustomMetadataFieldCommand, MoveCustomMetadataFieldCommand, RemoveCustomMetadataFieldCommand
 } from '../shared/CustomMetadataFieldCommands'
 import RemoveReferenceCommand from './RemoveReferenceCommand'
+import RemoveItemCommand from '../shared/RemoveItemCommand'
 import SwitchViewCommand from '../shared/SwitchViewCommand'
 
 export default {
@@ -109,6 +110,10 @@ export default {
     config.addCommand('remove-col-item', RemoveCollectionItemCommand, {
       commandGroup: 'collection'
     })
+    config.addCommand('remove-footnote', RemoveItemCommand, {
+      nodeType: 'footnote',
+      commandGroup: 'footnote'
+    })
     config.addCommand('remove-metadata-field', RemoveCustomMetadataFieldCommand, {
       commandGroup: 'custom-metadata-fields'
     })
@@ -247,7 +252,7 @@ function registerCollectionCommand (config, itemType, collectionPath, options =
   if (options.keyboardShortcut) {
     config.addKeyboardShortcut(options.keyboardShortcut, { command: `add-${itemType}` })
   }
-  if (!config.automaticOrder) {
+  if (!options.automaticOrder) {
     config.addCommand(`move-up-${itemType}`, MoveCollectionItemCommand, {
       direction: 'up',
       commandGroup: 'collection',

src/article/metadata/RemoveReferenceCommand.js

@@ -1,21 +1,15 @@
-import { Command, isNil } from 'substance'
-
-export default class RemoveReferenceCommand extends Command {
-  getCommandState (params, context) {
-    return { disabled: this.isDisabled(params, context) }
-  }
+import RemoveItemCommand from '../shared/RemoveItemCommand'
 
+export default class RemoveReferenceCommand extends RemoveItemCommand {
   isDisabled (params, context) {
-    const xpath = params.selectionState.xpath
+    const node = this._getNode(params)
     const isCustomSelection = params.selection.isCustomSelection()
-    if (!isCustomSelection || isNil(xpath) || xpath.length === 0) return true
+    if (!isCustomSelection || !node) return true
     // Every reference should be inside article references property
-    return xpath[xpath.length - 1].property !== 'references'
+    return node.getXpath().property !== 'references'
   }
 
-  execute (params, context) {
-    const api = context.api
-    const reference = params.selectionState.node
-    api._removeReference(reference)
+  _getNode (params) {
+    return params.selectionState.node
   }
 }

src/article/editor/RemoveItemCommand.js → src/article/shared/RemoveItemCommand.js

@@ -1,36 +1,37 @@
 import { Command, documentHelpers } from 'substance'
-import { findParentByType } from '../shared/nodeHelpers'
+import { findParentByType } from './nodeHelpers'
 
-// TODO: We already have collection commands for metadata,
-// maybe we should use set of commands
 export default class RemoveItemCommand extends Command {
   getCommandState (params, context) {
     return { disabled: this.isDisabled(params, context) }
   }
 
+  isDisabled (params, context) {
+    const nodeType = this.config.nodeType
+    const xpath = params.selectionState.xpath
+    return !xpath.find(n => n.type === nodeType)
+  }
+
   execute (params, context) {
-    const node = this._getNodeForSelection(params, context)
+    const api = context.api
+    const node = this._getNode(params)
     const path = this._getPath(context, node)
-    let editorSession = context.editorSession
+    const editorSession = context.editorSession
     editorSession.transaction(tx => {
       documentHelpers.remove(tx, path, node.id)
+      api._removeCorrespondingXrefs(tx, node)
       documentHelpers.deepDeleteNode(tx, node.id)
       tx.selection = null
     })
   }
 
-  isDisabled (params, context) {
-    const nodeType = this.config.nodeType
-    const xpath = params.selectionState.xpath
-    return !xpath.find(n => n.type === nodeType)
-  }
-
-  _getNodeForSelection (params, context) {
+  _getNode (params) {
     const nodeType = this.config.nodeType
     const sel = params.selection
     const doc = params.editorSession.getDocument()
     const nodeId = sel.getNodeId()
     const selectedNode = doc.get(nodeId)
+    if (selectedNode.type === nodeType) return selectedNode
     return findParentByType(selectedNode, nodeType)
   }
 

src/article/shared/TableFootnoteManager.js

@@ -25,7 +25,10 @@ export default class TableFootnoteManager extends AbstractCitationManager {
     let doc = this._getDocument()
     let footnotes = this.tableFigure.footnotes
     if (footnotes) {
-      return footnotes.map(id => doc.get(id))
+      // NOTE: in case of table removing there might be already no footnotes
+      // in the document, so we need to filter out undefined values
+      // TODO: can we solve it differently?
+      return footnotes.map(id => doc.get(id)).filter(Boolean)
     } else {
       return []
     }

test/Footnotes.test.js

@@ -1,15 +1,32 @@
 import { test } from 'substance-test'
+import { DefaultDOMElement } from 'substance'
 import {
   openManuscriptEditor, getDocument, getSelectionState, setSelection, fixture,
   openContextMenuAndFindTool, openMenuAndFindTool, getSelection, selectNode
 } from './shared/integrationTestHelpers'
+import { importElement } from './shared/testHelpers'
 import setupTestApp from './shared/setupTestApp'
 import { getLabel } from '../index'
 
+const emptyLabel = '???'
 const manuscriptFootnoteSelector = '.sc-manuscript > .sc-manuscript-section.sm-footnotes .sc-footnote'
 const tableFootnoteSelector = '.sc-table-figure > .se-footnotes > .sc-footnote'
 const tableFootnoteContentXpath = ['article', 'body', 'table-figure', 'footnote', 'paragraph']
 
+const FOOTNOTE_WITH_LABEL = `
+  <fn>
+    <label>Label</label>
+    <p>Footnote content</p>
+  </fn>
+`
+
+test('Footnotes: import footnote', t => {
+  let el = DefaultDOMElement.parseSnippet(FOOTNOTE_WITH_LABEL.trim(), 'xml')
+  let footnoteNode = importElement(el)
+  t.equal(footnoteNode.label, '', 'custom label should be ignored during import')
+  t.end()
+})
+
 test('Footnotes: add a footnote while selection is in the manuscript body', t => {
   let { app } = setupTestApp(t, fixture('cross-references'))
   let editor = openManuscriptEditor(app)
@@ -87,13 +104,16 @@ test('Footnotes: remove a manuscript footnote', t => {
   let { app } = setupTestApp(t, fixture('cross-references'))
   let editor = openManuscriptEditor(app)
   let doc = getDocument(editor)
-  t.notNil(editor.find('[data-id=fn-1]'), 'the footnote should be visible')
+  const getFirstFootnote = () => editor.find('[data-id=fn-1]')
+  const getFirstFootnoteXref = () => editor.find('[data-id=p1-xref-1]')
+  t.notNil(getFirstFootnote(), 'the footnote should be visible')
   setSelection(editor, 'fn-1-p-1.content', 1)
   const removeTool = openContextMenuAndFindTool(editor, '.sm-remove-footnote')
   t.ok(removeTool, 'there should be a remove button')
   removeTool.click()
   t.isNil(doc.get('fn-1'), 'the footnote should have been removed from the model')
   t.isNil(editor.find('[data-id=fn-1]'), '.. and should not be visible anymore')
+  t.equal(getFirstFootnoteXref().text(), emptyLabel, 'xref label should not contain reference')
   t.end()
 })
 

test/ManuscriptEditor.test.js

@@ -4,7 +4,7 @@ import {
   loadBodyFixture, getDocument, setSelection, LOREM_IPSUM,
   openContextMenuAndFindTool, openMenuAndFindTool, clickUndo,
   isToolEnabled, createKeyEvent, selectNode, getSelection, selectRange,
-  getCurrentViewName
+  getCurrentViewName, deleteSelection
 } from './shared/integrationTestHelpers'
 import setupTestApp from './shared/setupTestApp'
 import { doesNotThrowInNodejs, DOMEvent, ClipboardEventData } from './shared/testHelpers'
@@ -354,6 +354,38 @@ test('ManuscriptEditor: inserting a table figure', t => {
   t.end()
 })
 
+const TABLE_WITH_FOOTNOTE = `
+  <table-wrap id="table1">
+    <table>
+      <tbody>
+        <tr id="t1_5">
+          <td id="t1_5_1">Table footnote<xref id="t1-xref-1" ref-type="table-fn" rid="tfn1">*</xref></td>
+        </tr>
+      </tbody>
+    </table>
+    <table-wrap-foot>
+      <fn-group>
+        <fn id="tfn1">
+          <label>*</label>
+          <p id="tfn1-p1">This is a table-footnote.</p>
+        </fn>
+      </fn-group>
+    </table-wrap-foot>
+  </table-wrap>
+`
+
+test('ManuscriptEditor: removing a table figure', t => {
+  let { app } = setupTestApp(t, { archiveId: 'blank' })
+  let editor = openManuscriptEditor(app)
+  loadBodyFixture(editor, TABLE_WITH_FOOTNOTE)
+  selectNode(editor, 'table1')
+  doesNotThrowInNodejs(t, () => {
+    deleteSelection(editor)
+  }, 'table removing should not throw')
+  t.isNil(editor.find('[data-id=table1]'), 'There should be no table anymore')
+  t.end()
+})
+
 const SPECS = [
   {
     'type': 'block-formula',

test/SupplementaryFile.test.js

@@ -1,11 +1,11 @@
-import { platform } from 'substance'
+import { DefaultDOMElement, platform } from 'substance'
 import { test } from 'substance-test'
 import {
   setCursor, openManuscriptEditor, PseudoDropEvent, PseudoFileEvent,
   fixture, loadBodyFixture, getDocument, openContextMenuAndFindTool,
   openMenuAndFindTool, deleteSelection, clickUndo, isToolEnabled, selectNode
 } from './shared/integrationTestHelpers'
-import { doesNotThrowInNodejs } from './shared/testHelpers'
+import { doesNotThrowInNodejs, exportNode, importElement } from './shared/testHelpers'
 import setupTestApp from './shared/setupTestApp'
 
 // TODO: test download for a just uploaded file
@@ -192,6 +192,32 @@ test('SupplementaryFile: replace a file', t => {
   t.end()
 })
 
+const FIGURE_WITH_FILE = `
+  <fig id="fig1a">
+    <label>Figure 1A</label>
+    <caption id="fig1a-caption">
+      <title>First panel</title>
+      <p specific-use="display-element-wrapper">
+        <supplementary-material id="sm1" mimetype="application" mime-subtype="zip" xlink:href="example.zip" xmlns:xlink="http://www.w3.org/1999/xlink">
+          <caption>
+            <p id="sm1-caption-p1">Description of Supplementary File</p>
+          </caption>
+        </supplementary-material>
+      </p>
+    </caption>
+    <graphic />
+  </fig>
+`
+
+test('SupplementaryFile: export from figure caption', t => {
+  let el = DefaultDOMElement.parseSnippet(FIGURE_WITH_FILE.trim(), 'xml')
+  let figureNode = importElement(el)
+  let figureEl = exportNode(figureNode)
+  let supFile = figureEl.find('supplementary-material')
+  t.isNotNil(supFile, 'supplementary-material should be exported')
+  t.end()
+})
+
 function _testDownloadTool (mode, bodyXML, expectedDownloadUrl) {
   test(`SupplementaryFile: download a ${mode} file`, t => {
     let { app } = setupTestApp(t, fixture('assets'))