fix!: shim an implementation of getSubStringLength (#6663) (CP: 24.4)
#6670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Shim an implementation of getSubStringLength Estimate the rendered length of a substring of text in an SVG element. Supports wrapping/truncation of long labels in charts. * Improve shimmed measurement routines Improve shims of getBBox and getSubStringLength: The prior implementations were relying on specific DOM structures that Highcharts used to create: if the measured element had children, all text was expected to be within those children, and none in the top-level element. Also, it did not handle arbitrary element nesting. The current version of Highcharts does not always follow these rules, and there were cases of structures like: <text>Some text here<tspan>and some in a nested element</tspan></text> In that example, 'Some text here' would be omitted from measurement. Replace that strategy with one that processes each text node separately, providing the context of the containing element, and recursing into all nested elements. Fix some bugs around new line management in getBBox that became apparent after that change. The previous method of computing string width was a rough approximation based on average character width. This method produced suboptimal results in many situtations, generating extra space or overlapping text. Instead, use the string-pixel-width library to provide a better estimate. This library has per-character widths for a number of font families and variants, and results in widths much closer to the actual rendered style. As Lucida is used heavily in at least the test charts, and it is not natively supported by the library, add a custom mapping file including metrics for it. Also, improve detection of font family and font size, walking up the element parent chain (even potentially outside of the measured element) to find settings of these attributes. * Review cleanup * Test getSubStringLength Add test for getSubStringLength to ensure proper handling of sub-strings crossing text nodes (and other cases). To support this, change the way methods are added to jsdom SVG elements: rather than adding them only for elements instantiated via createElementNS, add them to the SVGElement prototype. This makes them available for elements created by any method of instantiation (including by setting innerHTML, as used in the test). Use rewire to access private elements of the exporter during tests. * spotless
DiegoCardoso
changed the title
getSubStringLength (#6663) (CP: 24.4)getSubStringLength (#6663) (CP: 24.4)
vaadin-charts-flow-parent/vaadin-charts-flow-svg-generator/package.json
Outdated
Show resolved
Hide resolved
vaadin-charts-flow-parent/vaadin-charts-flow-svg-generator/package.json
Outdated
Show resolved
Hide resolved
DiegoCardoso
approved these changes
Sep 26, 2024
|
|
Hi @vaadin-bot and @DiegoCardoso, when i performed cherry-pick to this commit to 23.5, i have encountered the following issue. Can you take a look and pick it manually? |
DiegoCardoso
added a commit
that referenced
this pull request
Sep 27, 2024
…4) (#6670) * Shim an implementation of `getSubStringLength` (#6663) * Shim an implementation of getSubStringLength Estimate the rendered length of a substring of text in an SVG element. Supports wrapping/truncation of long labels in charts. * Improve shimmed measurement routines Improve shims of getBBox and getSubStringLength: The prior implementations were relying on specific DOM structures that Highcharts used to create: if the measured element had children, all text was expected to be within those children, and none in the top-level element. Also, it did not handle arbitrary element nesting. The current version of Highcharts does not always follow these rules, and there were cases of structures like: <text>Some text here<tspan>and some in a nested element</tspan></text> In that example, 'Some text here' would be omitted from measurement. Replace that strategy with one that processes each text node separately, providing the context of the containing element, and recursing into all nested elements. Fix some bugs around new line management in getBBox that became apparent after that change. The previous method of computing string width was a rough approximation based on average character width. This method produced suboptimal results in many situtations, generating extra space or overlapping text. Instead, use the string-pixel-width library to provide a better estimate. This library has per-character widths for a number of font families and variants, and results in widths much closer to the actual rendered style. As Lucida is used heavily in at least the test charts, and it is not natively supported by the library, add a custom mapping file including metrics for it. Also, improve detection of font family and font size, walking up the element parent chain (even potentially outside of the measured element) to find settings of these attributes. * Review cleanup * Test getSubStringLength Add test for getSubStringLength to ensure proper handling of sub-strings crossing text nodes (and other cases). To support this, change the way methods are added to jsdom SVG elements: rather than adding them only for elements instantiated via createElementNS, add them to the SVGElement prototype. This makes them available for elements created by any method of instantiation (including by setting innerHTML, as used in the test). Use rewire to access private elements of the exporter during tests. * spotless * chore: pin npm dependencies --------- Co-authored-by: Aron Nopanen <[email protected]> Co-authored-by: Diego Cardoso <[email protected]>
DiegoCardoso
added a commit
that referenced
this pull request
Sep 27, 2024
…5) (#6673) * Shim an implementation of `getSubStringLength` (#6663) * Shim an implementation of getSubStringLength Estimate the rendered length of a substring of text in an SVG element. Supports wrapping/truncation of long labels in charts. * Improve shimmed measurement routines Improve shims of getBBox and getSubStringLength: The prior implementations were relying on specific DOM structures that Highcharts used to create: if the measured element had children, all text was expected to be within those children, and none in the top-level element. Also, it did not handle arbitrary element nesting. The current version of Highcharts does not always follow these rules, and there were cases of structures like: <text>Some text here<tspan>and some in a nested element</tspan></text> In that example, 'Some text here' would be omitted from measurement. Replace that strategy with one that processes each text node separately, providing the context of the containing element, and recursing into all nested elements. Fix some bugs around new line management in getBBox that became apparent after that change. The previous method of computing string width was a rough approximation based on average character width. This method produced suboptimal results in many situtations, generating extra space or overlapping text. Instead, use the string-pixel-width library to provide a better estimate. This library has per-character widths for a number of font families and variants, and results in widths much closer to the actual rendered style. As Lucida is used heavily in at least the test charts, and it is not natively supported by the library, add a custom mapping file including metrics for it. Also, improve detection of font family and font size, walking up the element parent chain (even potentially outside of the measured element) to find settings of these attributes. * Review cleanup * Test getSubStringLength Add test for getSubStringLength to ensure proper handling of sub-strings crossing text nodes (and other cases). To support this, change the way methods are added to jsdom SVG elements: rather than adding them only for elements instantiated via createElementNS, add them to the SVGElement prototype. This makes them available for elements created by any method of instantiation (including by setting innerHTML, as used in the test). Use rewire to access private elements of the exporter during tests. * spotless * chore: pin npm dependencies --------- Co-authored-by: Vaadin Bot <[email protected]> Co-authored-by: Aron Nopanen <[email protected]>
|
This ticket/PR has been released with Vaadin 24.4.13. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.