Skip to content
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 the issue where TextNode does not render when it is an empty string and the line-height issue in non-Firefox browsers; also, fix the misalignment of textDecorationLine at high resolutions. #3182

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

xweiba
Copy link

@xweiba xweiba commented Jul 2, 2024

Summary

This PR fixes/implements the following bugs/features

  • Bug Fix the issue of TextNode content being overlooked in rendering due to being perceived as blank by trim().
  • Resolving the line-height issue of TextNode in non-Firefox browsers by utilizing element heights.
  • Correcting the issue of textDecorationLine displaying offsets on high-resolution devices by utilizing element heights.

Closing issues

Fixes #2238 #1624 #2107

… being overlooked in rendering due to being perceived as blank by trim().
…x-based browsers by modifying the x-axis calculation method. This update addresses line-height issues caused by differences in x-axis calculation methods in browsers that do not use the Firefox engine. By refining the x-axis calculation method, it ensures uniform line-height behavior across all major browsers.
…n-Firefox-based browsers by modifying the x-axis calculation method. This update addresses line-height issues caused by differences in x-axis calculation methods in browsers that do not use the Firefox engine. By refining the x-axis calculation method, it ensures uniform line-height behavior across all major browsers."

This reverts commit 806f466.
…x-based browsers by modifying the x-axis calculation method. This update addresses line-height issues caused by differences in x-axis calculation methods in browsers that do not use the Firefox engine. By refining the x-axis calculation method, it ensures uniform line-height behavior across all major browsers.
…rors on high-resolution devices due to varying devicePixelRatio, corrected by using relative values of element heights.
@Zefling
Copy link

Zefling commented Jul 4, 2024

Is it possible to push this on:
https://github.com/html2canvas/html2canvas

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

The changes to the CanvasRenderer.js file include updates to how text decorations are drawn on a canvas. These changes introduce a variable fillHeight to adjust the height of the text decoration lines dynamically and aim to fix issues related to high-resolution devices. The code appears to address previous comments and TODOs by streamlining the drawing process.

Key Changes

  1. Variable Introduction:

    • Added var fillHeight = 1; to standardize the fill height of the text decorations.
  2. Updated Drawing Logic:

    • Simplified the drawing logic for TEXT_DECORATION_LINE_UNDERLINE, TEXT_DECORATION_LINE_OVERLINE, and TEXT_DECORATION_LINE_LINE_THROUGH to use the new fillHeight variable.
  3. Removed Redundant Code:

    • Removed old calculations and TODO comments related to handling font size variations and browser inconsistencies.

Recommendations

The changes improve the readability and maintainability of the code. However, here are a few recommendations:

  1. Variable Naming: Consider using a more descriptive name for fillHeight, such as decorationLineHeight, to clarify its purpose.
  2. Code Comments: Although some TODO comments were removed, it might be beneficial to add comments explaining why fillHeight is used and how it resolves the previous issues.
  3. Testing: Ensure thorough testing across different browsers and devices to confirm that the new approach consistently resolves the high-resolution device issues and maintains correct text decoration rendering.

Example with Recommendations Applied

Here's a slightly modified version with a more descriptive variable name and an added comment:

export class CanvasRenderer extends Renderer {
    ...
    if (styles.textDecorationLine.length) {
        this.ctx.fillStyle = asString(styles.textDecorationColor || styles.color);
        styles.textDecorationLine.forEach((textDecorationLine) => {
            var decorationLineHeight = 1; // Fix for high-resolution device issues
            switch (textDecorationLine) {
                case TEXT_DECORATION_LINE_UNDERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + text.bounds.height - decorationLineHeight, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_OVERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_LINE_THROUGH:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + (text.bounds.height / 2) - (decorationLineHeight / 2), text.bounds.width, decorationLineHeight);
                    break;
            }
        });
    }
    ...
}

@xweiba
Copy link
Author

xweiba commented Jul 17, 2024

Is it possible to push this on:是否有可能推动这个: https://github.com/html2canvas/html2canvas

Already PR

@xweiba
Copy link
Author

xweiba commented Jul 17, 2024

Review Summary 评测总结

The changes to the CanvasRenderer.js file include updates to how text decorations are drawn on a canvas. These changes introduce a variable fillHeight to adjust the height of the text decoration lines dynamically and aim to fix issues related to high-resolution devices. The code appears to address previous comments and TODOs by streamlining the drawing process.对 CanvasRenderer.js 文件的更改包括对如何在画布上绘制文本装饰的更新。这些更改引入了变量 fillHeight 来动态调整文本装饰线的高度,旨在解决与高分辨率设备相关的问题。该代码似乎通过简化绘图过程来解决以前的注释和 TODO。

Key Changes 主要变化

  1. Variable Introduction: 变量介绍:

    • Added var fillHeight = 1; to standardize the fill height of the text decorations.添加了 var fillHeight = 1; 以标准化文本装饰的填充高度。
  2. Updated Drawing Logic: 更新绘图逻辑:

    • Simplified the drawing logic for TEXT_DECORATION_LINE_UNDERLINE, TEXT_DECORATION_LINE_OVERLINE, and TEXT_DECORATION_LINE_LINE_THROUGH to use the new fillHeight variable.简化了 TEXT_DECORATION_LINE_UNDERLINETEXT_DECORATION_LINE_OVERLINETEXT_DECORATION_LINE_LINE_THROUGH 的绘制逻辑以使用新的 fillHeight 变量。
  3. Removed Redundant Code: 删除冗余代码:

    • Removed old calculations and TODO comments related to handling font size variations and browser inconsistencies.删除了与处理字体大小变化和浏览器不一致相关的旧计算和 TODO 注释。

Recommendations 建议

The changes improve the readability and maintainability of the code. However, here are a few recommendations:这些更改提高了代码的可读性和可维护性。不过,这里有一些建议:

  1. Variable Naming: Consider using a more descriptive name for fillHeight, such as decorationLineHeight, to clarify its purpose.变量命名:考虑为 fillHeight 使用更具描述性的名称,例如 decorationLineHeight ,以阐明其用途。
  2. Code Comments: Although some TODO comments were removed, it might be beneficial to add comments explaining why fillHeight is used and how it resolves the previous issues.代码注释:虽然删除了一些 TODO 注释,但添加注释来解释为什么使用 fillHeight 以及它如何解决以前的问题可能会有所帮助。
  3. Testing: Ensure thorough testing across different browsers and devices to confirm that the new approach consistently resolves the high-resolution device issues and maintains correct text decoration rendering.测试:确保在不同的浏览器和设备上进行彻底的测试,以确认新方法能够一致地解决高分辨率设备问题并保持正确的文本装饰渲染。

Example with Recommendations Applied

应用建议的示例
Here's a slightly modified version with a more descriptive variable name and an added comment:这是一个稍作修改的版本,具有更具描述性的变量名称和添加的注释:

export class CanvasRenderer extends Renderer {
    ...
    if (styles.textDecorationLine.length) {
        this.ctx.fillStyle = asString(styles.textDecorationColor || styles.color);
        styles.textDecorationLine.forEach((textDecorationLine) => {
            var decorationLineHeight = 1; // Fix for high-resolution device issues
            switch (textDecorationLine) {
                case TEXT_DECORATION_LINE_UNDERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + text.bounds.height - decorationLineHeight, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_OVERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_LINE_THROUGH:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + (text.bounds.height / 2) - (decorationLineHeight / 2), text.bounds.width, decorationLineHeight);
                    break;
            }
        });
    }
    ...
}

Review Summary 评测总结

The changes to the CanvasRenderer.js file include updates to how text decorations are drawn on a canvas. These changes introduce a variable fillHeight to adjust the height of the text decoration lines dynamically and aim to fix issues related to high-resolution devices. The code appears to address previous comments and TODOs by streamlining the drawing process.对 CanvasRenderer.js 文件的更改包括对如何在画布上绘制文本装饰的更新。这些更改引入了变量 fillHeight 来动态调整文本装饰线的高度,旨在解决与高分辨率设备相关的问题。该代码似乎通过简化绘图过程来解决以前的注释和 TODO。

Key Changes 主要变化

  1. Variable Introduction: 变量介绍:

    • Added var fillHeight = 1; to standardize the fill height of the text decorations.添加了 var fillHeight = 1; 以标准化文本装饰的填充高度。
  2. Updated Drawing Logic: 更新绘图逻辑:

    • Simplified the drawing logic for TEXT_DECORATION_LINE_UNDERLINE, TEXT_DECORATION_LINE_OVERLINE, and TEXT_DECORATION_LINE_LINE_THROUGH to use the new fillHeight variable.简化了 TEXT_DECORATION_LINE_UNDERLINETEXT_DECORATION_LINE_OVERLINETEXT_DECORATION_LINE_LINE_THROUGH 的绘制逻辑以使用新的 fillHeight 变量。
  3. Removed Redundant Code: 删除冗余代码:

    • Removed old calculations and TODO comments related to handling font size variations and browser inconsistencies.删除了与处理字体大小变化和浏览器不一致相关的旧计算和 TODO 注释。

Recommendations 建议

The changes improve the readability and maintainability of the code. However, here are a few recommendations:这些更改提高了代码的可读性和可维护性。不过,这里有一些建议:

  1. Variable Naming: Consider using a more descriptive name for fillHeight, such as decorationLineHeight, to clarify its purpose.变量命名:考虑为 fillHeight 使用更具描述性的名称,例如 decorationLineHeight ,以阐明其用途。
  2. Code Comments: Although some TODO comments were removed, it might be beneficial to add comments explaining why fillHeight is used and how it resolves the previous issues.代码注释:虽然删除了一些 TODO 注释,但添加注释来解释为什么使用 fillHeight 以及它如何解决以前的问题可能会有所帮助。
  3. Testing: Ensure thorough testing across different browsers and devices to confirm that the new approach consistently resolves the high-resolution device issues and maintains correct text decoration rendering.测试:确保在不同的浏览器和设备上进行彻底的测试,以确认新方法能够一致地解决高分辨率设备问题并保持正确的文本装饰渲染。

Example with Recommendations Applied

应用建议的示例
Here's a slightly modified version with a more descriptive variable name and an added comment:这是一个稍作修改的版本,具有更具描述性的变量名称和添加的注释:

export class CanvasRenderer extends Renderer {
    ...
    if (styles.textDecorationLine.length) {
        this.ctx.fillStyle = asString(styles.textDecorationColor || styles.color);
        styles.textDecorationLine.forEach((textDecorationLine) => {
            var decorationLineHeight = 1; // Fix for high-resolution device issues
            switch (textDecorationLine) {
                case TEXT_DECORATION_LINE_UNDERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + text.bounds.height - decorationLineHeight, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_OVERLINE:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top, text.bounds.width, decorationLineHeight);
                    break;
                case TEXT_DECORATION_LINE_LINE_THROUGH:
                    this.ctx.fillRect(text.bounds.left, text.bounds.top + (text.bounds.height / 2) - (decorationLineHeight / 2), text.bounds.width, decorationLineHeight);
                    break;
            }
        });
    }
    ...
}

Thank you for your suggestion. I have made the modifications accordingly.

@yorickshan
Copy link

build failure: 'middle' is declared but its value is never read.
191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);

@xweiba
Copy link
Author

xweiba commented Jul 18, 2024

build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);

Okay, I have made the changes.

@yorickshan
Copy link

yorickshan commented Jul 18, 2024

build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);

Okay, I have made the changes.

The original author is not active on github, can you push this on: https://github.com/yorickshan/html2canvas-pro, thx ❤️ (ps: 有什么问题也可以用中文交流 😄 )

@xweiba xweiba changed the title Fix the issue of TextNode content being overlooked in rendering due to being perceived as blank by trim(). Fix the issue where TextNode does not render when it is an empty string and the line-height issue in non-Firefox browsers; also, fix the misalignment of textDecorationLine at high resolutions. Jul 18, 2024
@xweiba
Copy link
Author

xweiba commented Jul 18, 2024

build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);

Okay, I have made the changes.

The original author is not active on github, can you push this on: https://github.com/yorickshan/html2canvas-pro, thx ❤️ (ps: 有什么问题也可以用中文交流 😄 )

好的 PR了 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

html2canvas text with underline does't work
4 participants