Fix 19570 tree chart not compliant strict csp styles

[Manviel]

Brief Information

This pull request is in the type of:

• bug fixing.
• refactoring.

What does this PR do?

This pull request addresses a specific limitation concerning Content Security Policy (CSP). When CSP is enabled, direct assignments to an element's style property using a string are disallowed. However it is possible to use className instead.

Basically, innerHTML causes the violation because it inserts html that contains inline styles.

Fixed issues

Tree chart with tooltips is not compliant with strict CSP directives for styles

Details

Before: What was the problem?

CSP violation errors are thrown into browser console:

After: How does it behave after the fixing?

It no longer uses inline styles.

Document Info

One of the following should be checked.

• This PR doesn't relate to document changes

ZRender Changes

• No.

Others

Tried to apply a similar solution as in https://github.com/ecomfe/zrender/pull/1030

[echarts-bot[bot]]

Thanks for your contribution! The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: PR: doc ready, PR: awaiting doc, PR: doc unchanged

This message is shown because the PR description doesn't contain the document related template.

[admsev]

@gooroodev review

[gooroodev]

Summary of Changes

1. ESLint Configuration:

   • Updated tsconfig.json path to include all subdirectories.
   • Added browser environment.

2. Tooltip Component:

   • Refactored assembleArrow function to use CSS classes instead of inline styles.
   • Introduced helper functions calculateArrowOffset and getColorClassName.
   • Refactored setContent method to use helper functions clearContent, setTextContent, and appendArrow.

3. Tooltip Markup:

   • Removed inline styles and replaced them with CSS classes.
   • Refactored getTooltipTextStyle to return CSS class names instead of inline styles.
   • Refactored wrapBlockHTML, wrapInlineNameHTML, and wrapInlineValueHTML to use CSS classes.

4. Format Utility:

   • Updated getTooltipMarker to use CSS classes instead of inline styles.

5. Test HTML Files:

   • Added basic.css file to include necessary styles.
   • Added Content-Security-Policy meta tag to enforce strict CSP.
   • Removed inline styles and replaced them with styles from basic.css.

6. CSS File:

   • Created basic.css with necessary styles for tooltips, markers, and layout.

Issues, Bugs, and Typos

1. Code Duplication:

   • calculateArrowOffset and getColorClassName functions are defined inside assembleArrow. They can be moved outside for better readability and reusability.

2. Inconsistent CSS Class Naming:

   • The class names for colors and other properties are inconsistent. Consider using a consistent naming convention for better maintainability.

3. Unused Imports:

   • Ensure no unused imports or variables are present in the updated files.

Proposed Code Improvements

1. Move Helper Functions Outside:

   function calculateArrowOffset(rotatedWH: number, borderWidth: number, arrowWH: number) {
       return Math.round(
           (
               ((rotatedWH - Math.SQRT2 * borderWidth) / 2
                   + Math.SQRT2 * borderWidth
                   - (rotatedWH - arrowWH) / 2)
               * 100
           ) / 100
       );
   }
   
   function getColorClassName(color: ZRColor) {
       const colorValue = convertToColorString(color);
       return colorValue.replace(/[^a-zA-Z0-9]/g, '');
   }
   

2. Consistent CSS Class Naming:

   • Ensure that the class names follow a consistent pattern, e.g., tooltip-arrow-color-fff instead of tooltip-arrow-background-color-fff.

General Review of Code Quality and Style

1. Code Quality:

   • The refactoring improves the separation of concerns by moving styles to CSS classes.
   • The use of helper functions enhances code readability and maintainability.
   • The use of CSS classes instead of inline styles makes the code more compliant with CSP.

2. Code Style:

   • The code follows good practices such as modularization and reusability.
   • The naming conventions for variables and functions are clear and descriptive.
   • The indentation and formatting are consistent and adhere to standard coding guidelines.

3. Testing:

   • Ensure that all changes are covered by existing or new tests.
   • Verify that the changes do not introduce any regressions or new issues.

Overall, the pull request significantly improves the compliance with strict CSP by removing inline styles and using CSS classes. The proposed changes enhance the maintainability and readability of the code.

Yours, Gooroo.dev. To receive reviews automatically, install Github App

[Manviel]

@gooroodev review

[gooroodev]

Appreciate the mention, @Manviel!

🐞Mistake	🤪Typo	🚨Security	🚀Performance	💪Best Practices	📖Readability	❓Others
1	0	1	1	2	2	0

Summary of Incoming Changes

1. ➕ Added functions calculateArrowOffset and getColorClassName to improve modularity and readability.
2. ➕ Introduced CSS class-based styling for tooltips and markers to comply with strict Content Security Policy (CSP) styles.
3. ➕ Added browser environment to ESLint configuration.
4. ➕ Created basic.css file to house common styles.
5. ➕ Introduced utility functions clearContent, setTextContent, and appendArrow to manage tooltip content.
6. ➖ Removed inline styles and replaced them with CSS classes.
7. 📖 Improved readability by refactoring inline styles into CSS classes.
8. 🚨 Added CSP meta tag to test HTML files.
9. 🚀 Optimized some functions by reducing inline style manipulations.

Identified Issues

ID	Type	Details	Severity	Confidence
1	🚨Security	Inline styles removed but some classes like tooltip-arrow-offset-* can lead to dynamic class injection	🟠Medium	🟠Medium
2	🚀Performance	The function createElementFromHTML uses DOMParser which can be replaced with innerHTML for better performance	🟡Low	🟡Low
3	💪Best Practices	The clearContent function could be optimized by setting innerHTML to an empty string.	🟡Low	🟡Low
4	💪Best Practices	The function appendArrow creates a new div element every time, which might be unnecessary.	🟡Low	🟡Low
5	📖Readability	The calculateArrowOffset function's formula is complex and could use inline comments.	🟡Low	🟡Low
6	📖Readability	The getColorClassName function could have a more descriptive name.	🟡Low	🟡Low

Code Snippets for Fixes

1. Security Issue Fix (Dynamic Class Injection):

   // src/component/tooltip/TooltipHTMLContent.ts
   function getColorClassName(color: ZRColor) {
       const colorValue = convertToColorString(color);
       return btoa(colorValue); // Base64 encoding to avoid injection
   }
   

2. Performance Optimization (DOMParser):

   // src/component/tooltip/tooltipMarkup.ts
   function createElementFromHTML(htmlString: string) {
       const tempDiv = document.createElement('div');
       tempDiv.innerHTML = htmlString.trim();
       return tempDiv.firstChild;
   }
   

3. Best Practices (clearContent Optimization):

   // src/component/tooltip/TooltipHTMLContent.ts
   function clearContent(el: HTMLElement) {
       el.innerHTML = '';
   }
   

4. Best Practices (appendArrow Optimization):

   // src/component/tooltip/TooltipHTMLContent.ts
   function appendArrow(el: HTMLElement, arrow: string) {
       if (!arrow) {
           return;
       }
       el.innerHTML += arrow;
   }
   

5. Readability Improvement (Inline Comments):

   // src/component/tooltip/TooltipHTMLContent.ts
   function calculateArrowOffset(rotatedWH: number, borderWidth: number, arrowWH: number) {
       // Calculate the offset of the arrow based on the rotated width and border width
       return Math.round(
           (
               ((rotatedWH - Math.SQRT2 * borderWidth) / 2
                   + Math.SQRT2 * borderWidth
                   - (rotatedWH - arrowWH) / 2)
               * 100
           ) / 100
       );
   }
   

6. Readability Improvement (Function Naming):

   // src/component/tooltip/TooltipHTMLContent.ts
   function generateColorClassName(color: ZRColor) {
       const colorValue = convertToColorString(color);
       return colorValue.replace(/[^a-zA-Z0-9]/g, '');
   }
   

General Review

The incoming changes significantly improve the codebase by introducing modular functions and replacing inline styles with CSS classes, which enhances compliance with CSP. However, there are minor issues related to security, performance, and readability that need attention. The code quality is generally good, but a few optimizations and better naming conventions can further improve it.

-- I only arrive when I am mentioned and asked to review the pull request. React or reply to keep the discussion going!

[github-actions[bot]]

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19717@8221d35

[freaky126]

@plainheart Could you reivew this issue ASAP, please.