fix(Object): Fix detection of falsy shadows in Object.needsItsOwnCache method

[gloriousjob]

Motivation

@closes #9390

Description

Fix blur that can happen when zooming in on text. This is due to incorrect caching that should refresh instead.

Changes

Update shadow on Objects to have undefined instead of null. Also fix cache check to check for not undefined rather than typeof.
Changing to undefined required having to change several tests which were expecting null and also to use an actual JSON object rather than a parsed string (string parse does not support undefined).
Utilized playwright to have an image compare for the test. I tried reverting the code and saw that it had a 0.02 diff so I used 0.01 to ensure it failed if the old code was present.

[ShaMan123]

Just gave a quick glance
Good catch!

Regarding the fix, I suggest extracting all the shadow undefined to a standalone PR, and rebasing this PR ontop of that or the other way around.
I am speaking on behalf of @asturur I am sure.
The fix itself can be simplified regardless to accomadate both undefined/null

Regarding the test, I don't see it runs in node , follow the template e2e/tests/template-with-node

[ShaMan123]

regardless I agree shadow should be undefined and not null

[asturur]

yes shadow null is weird, let's fix it counting that one day the shadow type will be just an optional, so let's use the !!shadow that today covers both null and undefined.

But i can't see why this PR balooned up to 30 changed files, let me read everything slowly

[asturur]

There is a reason why i don't want the visual test and i ll try to be exhaustive.

There are testing related reasons first.
Being blurry is not a bug in general, and being blurry is the side effect of the bug you experienced in your particular case.
The bug here is that you are asking for objectCaching false and you are forced to use the cache without a valid reason.

The bug is that the check on shadow was done on typeof shadow === 'object' and null is an object, so shadow or not shadow was always true.

The bug is that the function

  needsItsOwnCache() {
    if (
      this.paintFirst === 'stroke' &&
      this.hasFill() &&
      this.hasStroke() &&
      typeof this.shadow === 'object'
    ) {
      return true;
    }
    if (this.clipPath) {
      return true;
    }
    return false;
  }

returns true when it shouldn't and so i would like to test this.

so we need to make an object, any, it doesn't have to be text, and test the output of this function, possibly with a description.

describe('needsItsOwnCache', () => {
  it('return false for default values', () => {
    const rect = new Rect({ width: 100, height: 100 }):
    epxect(rect.needsItsOwnCache()).toBe(false);
  });
  it('return true when a clipPath is present', () => {
     ...
  });
  it('return true when paintFirst is stroke and there is a shadow', () => {
     ...
  });
  it('return false when paintFirst is stroke and there is no shadow', () => {
     ...
  });
  it('any other relevant cases', () => {
     ...
  });
})

On top of this, there are other reasons why i don't think an e2e test is a good idea for this:

• being blurry is hard to detect, font rendering differences between OS and machine config are possibly stronger than the blurriness
• being blurry depends on caching implementation, if we do a better cache that is not blurry the test is not covering the bug anymore
• start a browser window to run this test is no joke for a ci, we need to use the e2e tests for flows and the fact that we use it also for rendering tests is just because we want to get away from qunit at some point, but i would rather not add them unless we are testing a rendering property itself.

I m sorry you made a 30 files PR and i m asking you to go back to a one line PR + a jest test.

Then if you want to open a following PR for the type change across files that is fine by me,

[asturur]

i left in a comment the reason for requesting changes

[gloriousjob]

@asturur Understood about the reasons and I appreciate the insight.
Thanks for providing some test cases, though I found they were pretty much in the QUnit test so I just moved them over into jest, except using Rect like your example instead of Object. Oddly enough, the no shadow test passed in QUnit but failed in jest with the original code... I don't really understand that but sounds like jest correctly tested.

[ShaMan123]

After reading comments by @asturur I also agree
Also I would add a test case of paintFirst = fill
You could/should use it.each since all cases do the same and vary in params

[ShaMan123]

@gloriousjob good job! thanks