fix: DEV-1555: RichText regions initialization (#473)

* First nice but not perfect try; also some comments

* Allow to replace history step if needed

Good for initialization, when you only need one step in history,
but some changes arrived.

by @Gondragos

* squash

* Remove fixRegionsXPath(); this should be on init

* use refs from model

* fix text; fix xpath; remove useless code

* Reset replaceNextUndoState on unfreeze

* remove highlightedText, just use a text from model

* fixes after review

* trying to fix initialization

inline elements skips init process because of messed up logic

* set correct order of initialization

load object
mark it as loaded
initialize regions
mark object is ready

also initialize regions from suggestions

* Fix history during initialization

mobx snapshots are generated after action, so we have to freeze
outside of actions, moved this to view/store

* try-catch around every region to skip only broken

* Detect initial load; simplify logic

Add `calculated` hidden field to `globalOffsets` to track initialized
state. It should be in the model to avoid reinit on undo/redo.
cDU actually useless, we call `needsUpdate()` explicitly on every
related step.

* Don't use cahcedRange from detached iframe

Check that iframe exists (defaultView should be not null)

* Fix draft from nowhere; fix errors in console

Author: hlomzik

src/components/Node/Node.js

@@ -18,7 +18,7 @@ const NodeViews = {
   RichTextRegionModel: NodeView({
     name: "HTML",
     icon: IconText,
-    getContent: node => <span style={{ color: "#5a5a5a" }}>{node.highlightedText}</span>,
+    getContent: node => <span style={{ color: "#5a5a5a" }}>{node.text}</span>,
   }),
 
   ParagraphsRegionModel: NodeView({

src/components/RelationsOverlay/BoundingBox.js

@@ -96,7 +96,7 @@ const _detect = region => {
     case "paragraphs":
     case "timeseriesregion": {
       const regionBbox = Geometry.getDOMBBox(region.getRegionElement());
-      const container = region.parent?.rootNodeRef?.current;
+      const container = region.parent?.visibleNodeRef?.current;
 
       if (container?.tagName === "IFRAME") {
         const iframeBbox = Geometry.getDOMBBox(container, true);

src/core/TimeTraveller.js

@@ -30,6 +30,7 @@ const TimeTraveller = types
     // A way to handle multiple simultaneous freezes from different places
     const freezingLockSet = new Set();
     let changesDuringFreeze = false;
+    let replaceNextUndoState = false;
 
     function triggerHandlers() {
       updateHandlers.forEach(handler => handler());
@@ -51,11 +52,16 @@ const TimeTraveller = types
 
       unfreeze(key) {
         self.safeUnfreeze(key);
-        if (!self.isFrozen && changesDuringFreeze) {
-          self.recordNow();
+        if (!self.isFrozen) {
+          if (changesDuringFreeze) self.recordNow();
+          self.setReplaceNextUndoState(false);
         }
       },
 
+      setReplaceNextUndoState(value = true) {
+        replaceNextUndoState = value;
+      },
+
       recordNow() {
         self.addUndoState(getSnapshot(targetStore));
       },
@@ -81,9 +87,10 @@ const TimeTraveller = types
           return;
         }
 
-        self.history.splice(self.undoIdx + 1);
+        self.history.splice(self.undoIdx + !replaceNextUndoState, self.history.length);
         self.history.push(recorder);
         self.undoIdx = self.history.length - 1;
+        replaceNextUndoState = false;
         changesDuringFreeze = false;
       },
 

src/mixins/HighlightMixin.js

@@ -7,34 +7,31 @@ import { isDefined } from "../utils/utilities";
 
 export const HighlightMixin = types
   .model()
-  .volatile(()  =>({
-    _highlightedText: "",
-  }))
   .views(self => ({
     get _hasSpans() {
+      // @todo is it possible that only some spans are connected?
       return self._spans ? (
         self._spans.every(span => span.isConnected)
       ) : false;
     },
-    get highlightedText() {
-      return self.text || self._highlightedText;
-    },
   }))
   .actions(self => ({
     /**
      * Create highlights from the stored `Range`
      */
     applyHighlight() {
       if (self.parent.isLoaded === false) return;
-      // Avoid calling this method twice
+
       // spans in iframe disappear on every annotation switch, so check for it
       // in iframe spans still isConnected, but window is missing
-      if (self._hasSpans && self._spans[0]?.ownerDocument?.defaultView) {
-        console.warn("Spans already created");
+      const isReallyConnected = Boolean(self._spans?.[0]?.ownerDocument?.defaultView);
+
+      // Avoid calling this method twice
+      if (self._hasSpans && isReallyConnected) {
         return;
       }
 
-      const range = self.rangeFromGlobalOffset();
+      const range = self.getRangeToHighlight();
       const root = self._getRootNode();
 
       // Avoid rendering before view is ready
@@ -47,6 +44,7 @@ export const HighlightMixin = types
 
       const labelColor = self.getLabelColor();
       const identifier = guidGenerator(5);
+      // @todo use label-based stylesheets created only once
       const stylesheet = createSpanStylesheet(root.ownerDocument, identifier, labelColor);
       const classNames = ["htx-highlight", stylesheet.className];
 
@@ -70,8 +68,8 @@ export const HighlightMixin = types
 
     updateHighlightedText() {
       if (!self.text) {
-        // Concatinating of spans' innerText is up to 10 times faster, but loses "\n"
-        const range = self.rangeFromGlobalOffset();
+        // Concatenating of spans' innerText is up to 10 times faster, but loses "\n"
+        const range = self.getRangeToHighlight();
         const root = self._getRootNode();
 
         if (!range || !root) {
@@ -81,9 +79,8 @@ export const HighlightMixin = types
 
         selection.removeAllRanges();
         selection.addRange(range);
-        self._highlightedText = String(selection);
+        self.text = String(selection);
         selection.removeAllRanges();
-
       }
     },
 

src/regions/RichTextRegion.js

@@ -11,12 +11,21 @@ import Registry from "../core/Registry";
 import { AreaMixin } from "../mixins/AreaMixin";
 import Utils from "../utils";
 import { isDefined } from "../utils/utilities";
-import { findRangeNative } from "../utils/selection-tools";
+import { findRangeNative, rangeToGlobalOffset } from "../utils/selection-tools";
 
 const GlobalOffsets = types.model("GlobalOffset", {
   start: types.number,
   end: types.number,
-});
+  // distinguish loaded globalOffsets from user's annotation and internally calculated one;
+  // we should rely only on calculated offsets to find ranges, see initRangeAndOffsets();
+  // it should be in the model to avoid reinit on undo/redo.
+  calculated: false,
+}).views(self => ({
+  get serialized() {
+    // should never get to serialized result
+    return { start: self.start, end: self.end };
+  },
+}));
 
 const Model = types
   .model("RichTextRegionModel", {
@@ -33,6 +42,7 @@ const Model = types
   })
   .volatile(() => ({
     hideable: true,
+    cachedRange: null,
   }))
   .views(self => ({
     get parent() {
@@ -77,7 +87,7 @@ const Model = types
 
           Object.assign(res.value, {
             ...xpathRange,
-            globalOffsets: self.globalOffsets?.toJSON(),
+            globalOffsets: self.globalOffsets.serialized,
           });
         } catch(e) {
           // regions may be broken, so they don't have globalOffsets
@@ -88,7 +98,7 @@ const Model = types
 
           if (self.globalOffsets) {
             Object.assign(res.value, {
-              globalOffsets: self.globalOffsets?.toJSON(),
+              globalOffsets: self.globalOffsets.serialized,
             });
           }
         }
@@ -101,7 +111,8 @@ const Model = types
       return res;
     },
 
-    updateOffsets(startOffset, endOffset) {
+    // text regions have only start/end, so we should update start/endOffsets with these values
+    updateTextOffsets(startOffset, endOffset) {
       Object.assign(self, { startOffset, endOffset });
     },
 
@@ -112,42 +123,103 @@ const Model = types
       });
     },
 
-    rangeFromGlobalOffset() {
+    getRangeToHighlight() {
       const root = self._getRootNode();
 
-      if (self.globalOffsets && isDefined(root)) {
-        return findRangeNative(self.globalOffsets.start, self.globalOffsets.end, root);
+      if (!root || !self.globalOffsets) return undefined;
+
+      const rangeIsMissing = !self.cachedRange
+        || self.cachedRange.collapsed
+        // if this range is in detached iframe it'll look like a good one, check this
+        || !self.cachedRange.startContainer?.ownerDocument?.defaultView;
+
+      if (rangeIsMissing) {
+        const { start, end } = self.globalOffsets;
+
+        self.cachedRange = findRangeNative(start, end, root);
       }
 
-      return self._getRange();
+      return self.cachedRange;
     },
 
-    // For external XPath updates
-    _fixXPaths() {
-      if (self.isText) return;
+    /**
+     * Main method to detect HTML range and its offsets for LSF region
+     * globalOffsets are used for:
+     * - internal use (get ranges to highlight quickly)
+     * - end users convenience
+     * - for emergencies (xpath invalid)
+     */
+    initRangeAndOffsets() {
+      if (self.globalOffsets?.calculated) return;
+
+      const root = self._getRootNode();
+      let range;
+
+      // 0. Text regions are simple — just get range by offsets
+      if (self.isText) {
+        const { startOffset: start, endOffset: end } = self;
+
+        self.globalOffsets = { start, end, calculated: true };
+        self.cachedRange = findRangeNative(start, end, root);
+        return;
+      }
+
+      // 1. first try to find range by xpath in original document
+      range = self._getRange({ useOriginalContent: true });
+
+      if (range) {
+        // we need this range in the visible document, so find it by global offsets
+        const originalRoot = self._getRootNode(true);
+        const [start, end] = rangeToGlobalOffset(range, originalRoot);
 
-      const range = self._getRange(true);
+        self.globalOffsets = { start, end, calculated: true };
+        self.cachedRange = findRangeNative(start, end, root);
+
+        return;
+      }
 
-      if (range && self.globalOffsets) {
-        const root = self._getRootNode(true);
+      // 2. then try to find range on visible document
+      // that's for old buggy annotations created over dirty document state
+      range = self._getRange({ useOriginalContent: false });
 
-        const rangeFromGlobal = findRangeNative(
-          self.globalOffsets.start,
-          self.globalOffsets.end,
-          root,
-        );
+      if (range) {
+        const [start, end] = rangeToGlobalOffset(range, root);
 
-        if (!rangeFromGlobal) return;
+        self.globalOffsets = { start, end, calculated: true };
+        self.cachedRange = range;
+
+        return;
+      }
+
+      // 3. if xpaths are broken use globalOffsets if given
+      if (self.globalOffsets && isDefined(root)) {
+        const { start, end } = self.globalOffsets;
 
-        const normedRange = xpath.fromRange(rangeFromGlobal, root);
+        self.cachedRange = findRangeNative(start, end, root);
 
-        if (!isDefined(normedRange)) return;
+        if (self.cachedRange) {
+          self._fixXPaths(self.cachedRange, root);
+          self.globalOffsets.calculated = true;
+        }
 
-        self.start = normedRange.start ?? self.start;
-        self.end = normedRange.end ?? self.end;
-        self.startOffset = normedRange.startOffset ?? self.startOffset;
-        self.endOffset = normedRange.endOffset ?? self.endOffset;
+        return;
       }
+
+      // 4. out of options — region is broken
+      // @todo show error in console and regions list
+      return undefined;
+    },
+
+    // fix XPaths when we found region by globalOffsets
+    _fixXPaths(range, root) {
+      const normedRange = xpath.fromRange(range, root);
+
+      if (!isDefined(normedRange)) return;
+
+      self.start = normedRange.start;
+      self.end = normedRange.end;
+      self.startOffset = normedRange.startOffset;
+      self.endOffset = normedRange.endOffset;
     },
 
     _getRange({ useOriginalContent = false, useCache = true } = {}) {
@@ -168,9 +240,13 @@ const Model = types
     },
 
     _getRootNode(originalContent = false) {
-      const ref = originalContent
-        ? self.parent.originalContentRef
-        : self.parent.rootNodeRef;
+      const parent = self.parent;
+      let ref;
+
+      if (originalContent) ref = parent.originalContentRef;
+      else if (parent.useWorkingNode) ref = parent.workingNodeRef;
+      else ref = parent.visibleNodeRef;
+
       const node = ref.current;
 
       return node?.contentDocument?.body ?? node;
@@ -183,29 +259,11 @@ const Model = types
 
       const { start, startOffset, end, endOffset } = self;
 
-      try {
-        if (self.isText) {
-          const { startContainer, endContainer } = Utils.Selection.findRange(startOffset, endOffset, rootNode);
-          const range = document.createRange();
-
-          if (!startContainer || !endContainer) return;
-
-          range.setStart(startContainer.node, startContainer.position);
-          range.setEnd(endContainer.node, endContainer.position);
-
-          self.text = range.toString();
-
-          return range;
-        }
-      } catch (err) {
-        console.log("can't find range", err);
-      }
-
       try {
         return xpath.toRange(start, startOffset, end, endOffset, rootNode);
       } catch (err) {
         // actually this happens when regions cannot be located by xpath for some reason
-        console.log("can't locate xpath", err);
+        console.warn("can't locate xpath", { start, end }, err);
       }
 
       return undefined;

src/stores/AnnotationStore.js

@@ -841,11 +841,12 @@ const Annotation = types
         });
       }
 
-      if (isFF(FF_DEV_1555)) {
-        self.updateObjects();
-      } else {
-        self.objects.forEach(obj => obj.needsUpdate?.());
-      }
+      const { history } = self;
+
+      history.freeze("richtext:suggestions");
+      self.objects.forEach(obj => obj.needsUpdate?.({ suggestions: true }));
+      history.setReplaceNextUndoState(true);
+      history.unfreeze("richtext:suggestions");
     },
 
     /**
@@ -954,6 +955,7 @@ const Annotation = types
           const hasStartEnd = isDefined(value.start) && isDefined(value.end);
           const lacksOffsets = !isDefined(value.startOffset) && !isDefined(value.endOffset);
 
+          // @todo move this Text regions offsets transform to RichTextRegion
           if (hasStartEnd && lacksOffsets) {
             return Object.assign({}, value, {
               start: "",