html-to-image icon indicating copy to clipboard operation
html-to-image copied to clipboard

fix(embedded-webfonts): retrieving font family via getPropertyValue

Open petrmiko opened this issue 10 months ago • 5 comments

Description

Motivation and Context

Current solution attempts to read fontFamily property from style, which is a https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration instance, and results in an undefined font that later crashes in normalizeFontFamily. This fix leverages CSSStyleDeclaration.getPropertyValue() and provides empty string in case of a node not having defined font-family.

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Enhancement (changes that improvement of current feature or performance)
  • [ ] Refactoring (changes that neither fixes a bug nor adds a feature)
  • [ ] Test Case (changes that add missing tests or correct existing tests)
  • [ ] Code style optimization (changes that do not affect the meaning of the code)
  • [ ] Docs (changes that only update documentation)
  • [ ] Chore (changes that don't modify src or test files)

Self Check before Merge

  • [ ] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

petrmiko avatar Feb 25 '25 18:02 petrmiko

👋 @petrmiko

💖 Thanks for opening this pull request! 💖

Please follow the contributing guidelines. And we use semantic commit messages to streamline the release process.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add graph.scale() method
  • docs: graph.getShortestPath is now available

Things that will help get your PR across the finish line:

  • Follow the TypeScript coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

biiibooo[bot] avatar Feb 25 '25 18:02 biiibooo[bot]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.50%. Comparing base (d9b2fcf) to head (746b875). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/embed-webfonts.ts 66.66% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #507   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files          10       10           
  Lines         612      612           
  Branches      150      150           
=======================================
  Hits          407      407           
  Misses        144      144           
  Partials       61       61           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 25 '25 18:02 codecov[bot]

Since v1.11.12, the problem causes an error when using Firefox to save html to image when a css webfont is used. For Firefox, the solution is to pin to 1.11.11.

I believe this PR fixes the problem. Somehow the github action failed to label the PR.

refs https://github.com/bubkoo/html-to-image/pull/476

cheungpat avatar Feb 27 '25 10:02 cheungpat

So apparently in Firefox we must use getPropertyValue to access property values of CSSStyleRule.

I apologize for causing the issue.

Btw, I think normalizeFontFamily doesn't need to handle undefined because getPropertyValue always returns a string (empty string if not set).

This will fix: https://github.com/bubkoo/html-to-image/issues/508

tuhm1 avatar Mar 02 '25 07:03 tuhm1

Hello @bubkoo, sorry to bother, could you please look into reviewing this MR? It is affecting all Firefox users :pray:

petrmiko avatar May 08 '25 10:05 petrmiko