-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(PatternBrush): getPatternSrc
, rm getPatternSrcFunction
#8468
Conversation
Build Stats
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
READY and TESTED
fabric.Object.prototype.objectCaching = true; | ||
}); | ||
hooks.after(() => { | ||
// fabric.config.restoreDefaults(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really bad.
Other suites depend on some configuration made in this test that leaks.
getPatternSrcFunction
getPatternSrc
, rm getPatternSrcFunction
The patternBrush is more likely a demo / blueprint rather than something useful. |
on top of that getPatternSrcFunction imho was a possible security flaw. |
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
Motivation
#8455 (comment)
Description
getPatternSrcFunction
was weird, mutating the function itself by converting it to string. Bad practice and no need for it.It seemed that in v4/v5 Pattern stopped accepting strings to execute as functions, breaking PatternBrush. It was forgotten since (I am guessing) it had no tests.
So I have removed the method to align with Pattern and added tests that cover these cases.
Refactored
PatternBrush.getPatternSrc
signature to accept color as an arg. Am not sure it is needed to say the truth. We can usethis.color
because the function runs in context always, seems so.Changes
Non breaking since
this.color
will still work as expected ingetPatternSrc
Gist
Added visual tests.
Found a leak in test config.
In Action