-
-
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(Text): _getFontDeclaration
#9082
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -785,9 +785,10 @@ export class Text< | |
) { | ||
const fontCache = cache.getFontCache(charStyle), | ||
fontDeclaration = this._getFontDeclaration(charStyle), | ||
previousFontDeclaration = this._getFontDeclaration(prevCharStyle), | ||
couple = previousChar + _char, | ||
stylesAreEqual = fontDeclaration === previousFontDeclaration, | ||
stylesAreEqual = | ||
previousChar && | ||
fontDeclaration === this._getFontDeclaration(prevCharStyle), | ||
fontMultiplier = charStyle.fontSize / this.CACHE_FONT_SIZE; | ||
let width: number | undefined, | ||
coupleWidth: number | undefined, | ||
|
@@ -1640,25 +1641,31 @@ export class Text< | |
* @returns {String} font declaration formatted for canvas context. | ||
*/ | ||
_getFontDeclaration( | ||
styleObject?: TextStyleDeclaration, | ||
{ | ||
fontFamily = this.fontFamily, | ||
fontStyle = this.fontStyle, | ||
fontWeight = this.fontWeight, | ||
fontSize = this.fontSize, | ||
}: Partial< | ||
Pick< | ||
TextStyleDeclaration, | ||
'fontFamily' | 'fontStyle' | 'fontWeight' | 'fontSize' | ||
> | ||
> = {}, | ||
forMeasuring?: boolean | ||
): string { | ||
const style = styleObject || this, | ||
family = this.fontFamily, | ||
fontIsGeneric = Text.genericFonts.indexOf(family.toLowerCase()) > -1; | ||
const fontFamily = | ||
family === undefined || | ||
family.indexOf("'") > -1 || | ||
family.indexOf(',') > -1 || | ||
family.indexOf('"') > -1 || | ||
fontIsGeneric | ||
? style.fontFamily | ||
: `"${style.fontFamily}"`; | ||
const parsedFontFamily = | ||
fontFamily.includes("'") || | ||
fontFamily.includes('"') || | ||
fontFamily.includes(',') || | ||
Text.genericFonts.includes(fontFamily.toLowerCase()) | ||
? fontFamily | ||
: `"${fontFamily}"`; | ||
return [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a hot path method? (e.g. called when rendering) In that case you should check if a string interpolation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we should do that. It is called more than once in rendering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should recheck our code. We changed so much without measuring, and we don't even have a way of properly measuring. |
||
style.fontStyle, | ||
style.fontWeight, | ||
forMeasuring ? this.CACHE_FONT_SIZE + 'px' : style.fontSize + 'px', | ||
fontFamily, | ||
fontStyle, | ||
fontWeight, | ||
`${forMeasuring ? this.CACHE_FONT_SIZE : fontSize}px`, | ||
parsedFontFamily, | ||
].join(' '); | ||
} | ||
|
||
|
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 changelog doesn't tell much about the change. It should be more
_getFontDeclaration: passed style should take precedence
as the test case.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.
point noted
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.
Yeah i also changed the pr description and the merge commit, but forgot to update the changelog.