[players] Fix persisted eraser path for annotations

[ivancea]

Problem

While erasing annotations, apparently erased lines will reappear later, because the code that stores the eraser mask uses a "limited" Fabric intersection calculation:

I could reproduce it consistently by doing:

1. Added a line
2. Erased the line
3. Added a circle
4. Erased the circle -> The line reappeared partially
5. Erased the line -> The circle reappeared partially
6. Erased the circle -> The circle reappeared again

(Note that to see the "reappearance", I have to go out of the preview/page and return)

Example:

[frankrousseau]

Note: this analysis was done with Claude Code, reviewed and endorsed by me.

Thanks for the PR! The premise doesn't hold on fabric 7.4.0 though: calcACoords() builds the coords from _getTransformedDimensions(), which already adds strokeWidth — e.g. new Path('M 0 0 L 100 0', { strokeWidth: 20 }).getBoundingRect() → { left: -10, top: -10, width: 120, height: 20 }. So intersectsWithObject() already accounts for the eraser radius, and the patch would count it twice. The test only fails without the fix because the FakePath mock returns stroke-less bounds.

Could you share reproduction steps? If lines really reappear, the cause is elsewhere.

[ivancea]

Uhm, sorry for that. This did indeed not fix it, I was overconfident on the fix. In fact, this didn't solve it (Just tested it again).

I'll keep the fix in case it makes sense later (I'm not sure it will, I'll clean it up later).

For a repro example, in an image preview, I:

1. Added a line
2. Erased the line
3. Added a circle
4. Erased the circle -> The line reappeared partially
5. Erased the line -> The circle reappeared partially
6. Erased the circle -> The circle reappeared again

(Note that to see the "reappearance", I have to go out of the preview/page and return)

Example:

I'll iterate it, I have a better dev workflow now 😅

[ivancea]

Fixed and checked

[frankrousseau]

Note: this analysis was done with Claude Code, reviewed and endorsed by me.

Good news: this time the diagnosis is right, and I could verify it against fabric 7.4.0. groupInit always runs an initialization layout, even with FixedLayout (that strategy only overrides getInitialSize, not shouldPerformLayout). commitLayout then shifts every child by (0,0) - childrenBboxCenter while the group itself keeps its serialized left/top, so the whole mask drifts by the bbox center on each save/reload cycle. That matches your repro exactly, and it means the comment in our code claiming "FixedLayout preserves them" was simply wrong. Thanks for tracking this down!

Since the root cause is "the layout strategy runs at all", there is a smaller fix: neutralize the strategy once, instead of bypassing the constructor and add() at each call site:

// Initialization layout recenters children around their bbox center, which
// corrupts mask paths stored in the erased object's local frame. Never layout.
class NoLayout extends FixedLayout {
  shouldPerformLayout() {
    return false
  }
}

and in the Eraser constructor: layoutManager: new LayoutManager(new NoLayout()). With that, fromObject and _addPathToObjectEraser can stay exactly as they are on main (new Eraser(children, options) and eraser.add(clone) both become safe), and addWithoutLayout is no longer needed.

I checked this variant against real fabric 7.4.0: children keep their coordinates on revival, the group keeps its serialized left/top/width/height, add() does not shift anything (the added trigger is gated by the same shouldPerformLayout), and the serialize/revive round-trip is stable. Bonus: children go through the public enterGroup, so they also get their parent reference set, which the _enterGroup shortcut skips.

Your regression tests are the valuable part, please keep them. To make them still fail without the fix, the FakeGroup mock can recenter children in its constructor, gated on the strategy:

constructor(objects = [], opts = {}) {
  this._objects = [...objects]
  Object.assign(this, opts)
  const strategy = opts.layoutManager?.strategy
  if (objects.length && strategy?.shouldPerformLayout?.({ type: 'initialization' }) !== false) {
    // mimic fabric: initialization layout recenters children around their bbox
    this._objects.forEach(o => { o.left = 0; o.top = 0 })
  }
}

and FakeGroup.add can go back to a plain push: in real fabric, add() on a FixedLayout group does not shift children, only the initialization layout does.

While you are at it, could you also update the stale comment above the constructor ("Children passed to fromObject() are already in group-local coords; FixedLayout preserves them.") to describe the new invariant? Something like: mask paths live in the erased object's local frame and must never be repositioned, hence NoLayout.

[ivancea]

Fixed with NoLayout, thanks for the guidance on this!

Btw, also added a related-but-parallel issue fix (In _getCacheCanvasDimensions()), that made after erasing, the fullscreen and assets view rendered differently erased annotations (Basically, a few corners that were erased could be seen while in assets page, but not in fullscreen).

[frankrousseau]

Thank you