fix: LSDV-4747: Selecting the end character on a Paragraph phrase to the very start of other phrases only includes the first phrase selection (#1257)

* adding another test case to paragraphs for selection issues

* Look for correct text node when selection ends on div

But range stored in region should also be fixed and not have 0 as the endOffset

* changing the test to select a more easily reproducible case manually

* fix highlighting of spans

* adding test for loading paragraph ranges correctly, still need to get this passing but now closer to a solution

* making test for initializing a paragraph region pass

* for some reason using Dialogue as the default dev config causes timeout errors

* fix smoke test for audio paragraphs

* making the result text function simplified to use existing phraseElements

* removing unused function

* adding another test case, and making a slight adjustment to fix

* updating codept timeout as more tests are pushing the threshold

---------

Co-authored-by: hlomzik <[email protected]>

Author: bmartel

e2e/codecept.conf.js

@@ -7,7 +7,7 @@ const fs = require('fs');
 const FRAGMENTS_PATH = './fragments/';
 
 module.exports.config = {
-  timeout: 60 * 30, // Time out after 30 minutes
+  timeout: 60 * 40, // Time out after 40 minutes
   tests: './tests/**/*.test.js',
   output: './output',
   helpers: {

e2e/tests/paragraphs-filter.test.js

@@ -5,6 +5,28 @@ Feature('Paragraphs filter');
 
 const AUDIO = 'https://htx-misc.s3.amazonaws.com/opensource/label-studio/examples/audio/barradeen-emotional.mp3';
 
+const ANNOTATIONS = [
+  {
+    'result': [
+      {
+        'id':'ryzr4QdL93',
+        'from_name':'ner',
+        'to_name':'text',
+        'source':'$dialogue',
+        'type':'paragraphlabels',
+        'value':{
+          'start':'2',
+          'end':'4',
+          'startOffset':0,
+          'endOffset':134,
+          'paragraphlabels': ['Important Stuff'],
+          'text': 'Uncomfortable silences. Why do we feel its necessary to yak about bullshit in order to be comfortable?I dont know. Thats a good question.Thats when you know you found somebody really special. When you can just shut the fuck up for a minute, and comfortably share silence.',
+        },
+      },
+    ],
+  },
+];
+
 const DATA = {
   audio: AUDIO,
   dialogue: [
@@ -36,6 +58,7 @@ const DATA = {
     },
   ],
 };
+
 const CONFIG = `
 <View>
   <ParagraphLabels name="ner" toName="text">
@@ -461,3 +484,117 @@ Scenario(
     }
   },
 );
+
+Scenario('Selecting the end character on a paragraph phrase to the very start of other phrases includes all selected phrases', async ({ I, LabelStudio, AtSidebar, AtParagraphs, AtLabels }) => {
+  const params = {
+    data: DATA,
+    config: CONFIG,
+  };
+
+  I.amOnPage('/');
+
+  LabelStudio.setFeatureFlags(FEATURE_FLAGS);
+  LabelStudio.init(params);
+  AtSidebar.seeRegions(0);
+
+  I.say('Select 2 regions in the consecutive phrases');
+
+  AtLabels.clickLabel('Random talk');
+  AtParagraphs.setSelection(
+    AtParagraphs.locateText('Dont you hate that?'),
+    18,
+    AtParagraphs.locateText('Uncomfortable silences. Why do we feel its necessary to yak about bullshit in order to be comfortable?'),
+    0,
+  );
+
+  AtSidebar.seeRegions(1);
+
+  const result = await LabelStudio.serialize();
+
+  assert.deepStrictEqual(
+    omitBy(result[0].value, (v, key) => key === 'paragraphlabels'),
+    {
+      start: '0',
+      end: '1',
+      startOffset: 18,
+      endOffset: 10,
+      text: '?\n\nHate what?',
+    },
+  );
+});
+
+Scenario('Selecting the end character on a paragraph phrase to the very start of other phrases includes all selected phrases except the very last one', async ({ I, LabelStudio, AtSidebar, AtParagraphs, AtLabels }) => {
+  const params = {
+    data: {
+      ...DATA,
+      dialogue: DATA.dialogue.map(d => [d, { ...d, text: `${d.text}2` }]).flat(), 
+    },
+    config: CONFIG,
+  };
+
+  I.amOnPage('/');
+
+  LabelStudio.setFeatureFlags(FEATURE_FLAGS);
+  LabelStudio.init(params);
+  AtSidebar.seeRegions(0);
+
+
+  I.say('Select 2 regions in the consecutive phrases of the one person');
+  AtParagraphs.clickFilter('Vincent Vega');
+  AtLabels.clickLabel('Random talk');
+  AtParagraphs.setSelection(
+    AtParagraphs.locateText('Hate what?2'),
+    10,
+    AtParagraphs.locateText('I dont know. Thats a good question.2'),
+    0,
+  );
+
+  AtSidebar.seeRegions(2);
+
+  const result = await LabelStudio.serialize();
+
+  assert.deepStrictEqual(
+    omitBy(result[0].value, (v, key) => key === 'paragraphlabels'),
+    {
+      start: '3',
+      end: '3',
+      startOffset: 10,
+      endOffset: 11,
+      text: '2',
+    },
+  );
+  assert.deepStrictEqual(
+    omitBy(result[1].value, (v, key) => key === 'paragraphlabels'),
+    {
+      start: '6',
+      end: '6',
+      startOffset: 0,
+      endOffset: 35,
+      text: 'I dont know. Thats a good question.',
+    },
+  );
+});
+
+Scenario('Initializing a paragraph region range should not include author names in text', async ({ I, LabelStudio, AtSidebar }) => {
+  const params = {
+    data: DATA,
+    annotations: ANNOTATIONS,
+    config: CONFIG,
+  };
+
+  I.amOnPage('/');
+  LabelStudio.setFeatureFlags(FEATURE_FLAGS);
+
+  const [{ result : [region] }] = ANNOTATIONS;
+  const { paragraphlabels: _paragraphlabels, ...value } = region.value;
+
+  LabelStudio.init(params);
+  AtSidebar.seeRegions(1);
+
+  const result = await LabelStudio.serialize();
+
+  assert.deepStrictEqual(
+    omitBy(result[0].value, (v, key) => key === 'paragraphlabels'),
+    value,
+  );
+});

src/tags/object/Paragraphs/Paragraphs.js

@@ -93,15 +93,33 @@ class HtxParagraphsView extends Component {
     for (i = 0; i < selection.rangeCount; i++) {
       const r = selection.getRangeAt(i);
 
-      if (r.endContainer.nodeName === 'DIV') {
-        r.setEnd(r.startContainer, r.startContainer.length);
+      if (r.endContainer.nodeType !== Node.TEXT_NODE) {
+        // offsets work differently for nodes and texts, so we have to find #text.
+        // lastChild because most probably this is div of the whole paragraph,
+        // and it has author div and phrase div.
+        const el = this.getPhraseElement(r.endContainer.lastChild);
+        let textNode = el;
+
+        while (textNode && textNode.nodeType !== Node.TEXT_NODE) {
+          textNode = textNode.firstChild;
+        }
+
+        // most probably this div is out of Paragraphs
+        // @todo maybe select till the end of Paragraphs?
+        if (!textNode) continue;
+
+        r.setEnd(textNode, 0);
       }
+
       if (r.collapsed || /^\s*$/.test(r.toString())) continue;
 
       try {
         splitBoundaries(r);
         const [startOffset, , start, originalStart] = this.getOffsetInPhraseElement(r.startContainer, r.startOffset);
-        const [endOffset, , end, originalEnd] = this.getOffsetInPhraseElement(r.endContainer, r.endOffset, false);
+        const [endOffset, , end, _originalEnd] = this.getOffsetInPhraseElement(r.endContainer, r.endOffset, false);
+
+        // if this shifts backwards, we need to take the lesser index.
+        const originalEnd = Math.min(end, _originalEnd);
 
         if (isFF(FF_DEV_2918)) {
           const visibleIndexes = item._value.reduce((visibleIndexes, v, idx) => {
@@ -286,14 +304,35 @@ class HtxParagraphsView extends Component {
     }
   }
 
+  /**
+   * Generates a textual representation of the current selection range.
+   *
+   * @param {number} start
+   * @param {number} end
+   * @param {number} startOffset
+   * @param {number} endOffset
+   * @returns {string}
+   */
+  _getResultText(start, end, startOffset, endOffset) {
+    const phrases = this.phraseElements;
+
+    if (start === end) return phrases[start].innerText.slice(startOffset, endOffset);
+
+    return [
+      phrases[start].innerText.slice(startOffset),
+      phrases.slice(start + 1, end).map(phrase => phrase.innerText),
+      phrases[end].innerText.slice(0, endOffset),
+    ].flat().join('');
+  }
+
   _handleUpdate() {
     const root = this.myRef.current;
     const { item } = this.props;
 
     // wait until text is loaded
     if (!item._value) return;
 
-    item.regs.forEach(function(r, i) {
+    item.regs.forEach((r, i) => {
       // spans can be totally missed if this is app init or undo/redo
       // or they can be disconnected from DOM on annotations switching
       // so we have to recreate them from regions data
@@ -304,6 +343,7 @@ class HtxParagraphsView extends Component {
         const range = document.createRange();
         const startNode = phrases[r.start].getElementsByClassName(item.layoutClasses.text)[0];
         const endNode = phrases[r.end].getElementsByClassName(item.layoutClasses.text)[0];
+
         let { startOffset, endOffset } = r;
 
         range.setStart(...findNodeAt(startNode, startOffset));
@@ -333,7 +373,7 @@ class HtxParagraphsView extends Component {
             r.fixOffsets(startOffset, endOffset);
           }
         } else if (!r.text && range.toString()) {
-          r.setText(range.toString());
+          r.setText(this._getResultText(+r.start, +r.end, startOffset, endOffset));
         }
 
         splitBoundaries(range);

src/tags/object/Paragraphs/Phrases.js

@@ -20,7 +20,7 @@ export const Phrases = observer(({ item }) => {
     if (getRoot(item).settings.showLineNumbers) classNames.push(styles.numbered);
 
     return (
-      <div key={`${item.name}-${idx}`} className={classNames.join(' ')} style={style.phrase}>
+      <div key={`${item.name}-${idx}`} data-testid={`phrase:${idx}`} className={classNames.join(' ')} style={style.phrase}>
         {isContentVisible && withAudio && !isNaN(v.start) && (
           <Button
             type="text"
@@ -29,11 +29,11 @@ export const Phrases = observer(({ item }) => {
             onClick={() => item.play(idx)}
           />
         )}
-        <span className={cls.name}>{v[item.namekey]}</span>
+        <span className={cls.name} data-skip-node="true">{v[item.namekey]}</span>
         <span className={cls.text}>{v[item.textkey]}</span>
       </div>
     );
   });
 
   return val;
-});
+});

src/utils/html.js

@@ -92,7 +92,16 @@ function getNextNode(node) {
   }
 }
 
-function getNodesInRange(range) {
+export function isValidTreeNode(node, commonAncestor) {
+  while(node) {
+    if (commonAncestor && node === commonAncestor) return true;
+    if (node.nodeType === Node.ELEMENT_NODE && node.dataset.skipNode === 'true') return false;
+    node = node.parentNode;
+  }
+  return true;
+}
+
+export function getNodesInRange(range) {
   const start = range.startContainer;
   const end = range.endContainer;
   const commonAncestor = range.commonAncestorContainer;
@@ -101,20 +110,24 @@ function getNodesInRange(range) {
 
   // walk parent nodes from start to common ancestor
   for (node = start.parentNode; node; node = node.parentNode) {
-    nodes.push(node);
+    if (isValidTreeNode(node, commonAncestor)) nodes.push(node);
     if (node === commonAncestor) break;
   }
   nodes.reverse();
 
   // walk children and siblings from start until end is found
   for (node = start; node; node = getNextNode(node)) {
-    nodes.push(node);
+    if (isValidTreeNode(node, commonAncestor)) nodes.push(node);
     if (node === end) break;
   }
 
   return nodes;
 }
 
+export function getTextNodesInRange(range) {
+  return getNodesInRange(range).filter(n => isTextNode(n));
+}
+
 function documentReverse(node) {
   if (node.lastChild) return node.lastChild;
 
@@ -192,8 +205,7 @@ function highlightRange(normedRange, cssClass, cssStyle) {
     cssClass = 'htx-annotation';
   }
 
-  const allNodes = getNodesInRange(normedRange._range);
-  const textNodes = allNodes.filter(n => isTextNode(n));
+  const textNodes = getTextNodesInRange(normedRange._range);
 
   const white = /^\s*$/;