[Bug]: Clipping Group with shadows will result in the loss of the canvas background.

[zhe-he]

CheckList

• I agree to follow this project's Code of Conduct
• I have read and followed the Contributing Guide
• I have read and followed the Issue Tracker Guide
• I have searched and referenced existing issues and discussions
• I am filing a BUG report.
• I have managed to reproduce the bug after upgrading to the latest version
• I have created an accurate and minimal reproduction

Version

6.0.0-beta16

In What environments are you experiencing the problem?

No response

Node Version (if applicable)

None

Link To Reproduction

demo

Steps To Reproduce

1. "Create a frame object with clipping that inherits from a group."
2. "Draw a rectangle with shadows."
3. "Add the rectangle to the frame."

Expected Behavior

The rectangle will be clipped if it exceeds the boundaries of the frame.

Actual Behavior

rt.

I have created an online demo, which contains two files. One is 'test-clip' which behaves normally, and the other is 'test-clip-shadow' which behaves abnormally.

demo

Error Message & Stack Trace

No response

[ShaMan123]

seems related to caching

[ShaMan123]

fabric.js/src/shapes/Group.ts

Lines 418 to 429 in 82d94b4

	shouldCache() {
	const ownCache = FabricObject.prototype.shouldCache.call(this);
	if (ownCache) {
	for (let i = 0; i < this._objects.length; i++) {
	if (this._objects[i].willDrawShadow()) {
	this.ownCaching = false;
	return false;
	}
	}
	}
	return ownCache;
	}

[ShaMan123]

https://codesandbox.io/p/devbox/fabric-clip-shadow-demo-forked-yqtqyh?file=%2Fsrc%2Fframe.ts%3A71%2C13

[zhe-he]

@ShaMan123
Thank you for pinpointing the issue. The final modification is as follows, which perfectly resolves the problem.

shouldCache() {
    const ownCache = FabricObject.prototype.shouldCache.call(this);
    if (ownCache) {
      for (let i = 0; i < this._objects.length; i++) {
        if (this._objects[i].willDrawShadow()) {
          this._objects[i].dirty = true;
        }
      }
    }
    return ownCache;
  }

[ShaMan123]

In that case I would do

shouldCache() {
    return FabricObject.prototype.shouldCache.call(this);
  }

_set(key, value) {
  super._set(key, value);
  key === 'dirty' && this.forEachObject(obj => obj._set('dirty', true)) 
}

[zhe-he]

@ShaMan123
Thank you, I really appreciate the direct problem-solving approach you provided.
However, this will lead to an infinite loop: group dirty -> child dirty -> group dirty.
So I changed it to this, not sure if it will cause any other issues.

key === 'dirty' && this.forEachObject(obj => obj.dirty =  true))

https://github.com/fabricjs/fabric.js/blob/7154d143f0e34bf3e779b5b9592ca98e704ea2fd/src/shapes/Object/Object.ts#L694C2-L711C6

[ShaMan123]

Good catch, we should PR to fix

- } else if (key === 'dirty' && this.group) {
+ } else if (key === 'dirty' && value && this.group) {

[ShaMan123]

Not sure actually
Your approach looks legit but consider that if you will have more levels of nesting you will need to walk down the tree, currently it is for 1 level of nesting

[ShaMan123]

- } else if (key === 'dirty' && this.group) {
+ } else if (key === 'dirty' && this.group && isChanged) {

This should work I think

[ShaMan123]

It should occur only when try so the initial approach is correct