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

Support the harfbuzz subsetter #56

Merged
merged 48 commits into from Jul 25, 2020
Merged

Support the harfbuzz subsetter #56

merged 48 commits into from Jul 25, 2020

Conversation

papandreou
Copy link
Collaborator

@papandreou papandreou commented Jul 27, 2019

Context: harfbuzz/harfbuzzjs#9

TODO:

  • Add woff and woff2 encoders and decoders
  • Find out why fontkit chokes on the output of the harfbuzz subsetter (fontkit.create returns undefined) Was just a silly bug in the handling of output formats in subsetLocalFontsWithHarfbuzz
  • Wait for the harfbuzz subsetter to support ligatures and possibly other missing features (see below)
  • Wait for a proper harfbuzzjs release and un-vendor hb-subset.wasm.

Rendering differences

Diff between original and subset for a text that utilizes ligatures:

11961-3996-1ip3s0e yr3x

There's also a difference when ligatures aren't involved:

11961-1918-v90nny oyha

@papandreou papandreou changed the title Support the harfbuzz subsetter WIP: Support the harfbuzz subsetter Jul 27, 2019
@papandreou papandreou self-assigned this Jul 27, 2019
@papandreou papandreou force-pushed the feature/harfbuzz branch 2 times, most recently from 8ce6081 to 06252d1 Compare August 4, 2019 09:28
@papandreou papandreou changed the base branch from feature/testReferenceImages to master August 11, 2019 08:40
@papandreou papandreou changed the title WIP: Support the harfbuzz subsetter Support the harfbuzz subsetter Aug 11, 2019
* master:
  Switch to postcss-value-parser instead
  Make a little progress with stripLocalTokens
  Convert extractReferencedCustomPropertyNames
  Add a test for extractReferencedCustomPropertyNames
  Start converting lib/stripLocalTokens.js, blocked on shellscape/postcss-values-parser#91
  WIP, carried over from assetgraph/assetgraph#989
  Update unexpected-set to ^2.0.1
  3.7.0
* master: (31 commits)
  4.0.0
  Make --no-recursive the default
  Fix path to the new main file
  Tidy up some tests a bit
  Also accept --canonical-root (not only --canonicalroot) and --dry-run (not only (--dryrun)
  inlineSubsets => inlineFonts to match the cli
  Exploit that yargs provides camelCased versions of --kebab-cased switches
  Test parseCommandLineOptions a bit more exhaustively
  Rearrange so the main export works as a programmatic api (instead of accepting an argv array)
  docs(readme): add Greenkeeper badge
  chore(package): update dependencies
  Update yargs and async-main-wrap
  Update assetgaph, httpception, mocha, nyc, sinon and unexpected
  Align linting config with Assetgraph
  3.7.1
  Fix broken main reference in package.json
  Remove no longer needed subsetFonts test (tracesByAsset)
  Also do the static trace when --dynamic is passed
  Bypass CSP in the headless browser so injecting font-tracer won't fail for that reason
  Report JavaScript and HTTP errors from the headless browser
  ...
* master: (28 commits)
  5.0.2
  Make it less messy by delaying the detaching of those relations
  Fix Munter/netlify-plugin-subfont#32
  5.0.1
  Update to font-snapper 1.0.2
  Only distribute relevant files in npm package
  5.0.0
  Update to prettier 2 and reformat all files
  Update yargs to 15.3.1
  Update sinon to 9.0.2
  Update to nyc 15
  Update to puppeteer 3.1.0
  Update engine node engine requirement to >=10
  Update mocha to 7.2.0
  Update eslint and plugins
  Update to font-tracer 2 and deprecate node 8 support. SEMVER-MAJOR
  Don't break when an unused variant points at a non-existent file
  Update asetgraph to 6.0.7
  Fix missing font size difference calculation in console output. Fixes #89
  Silence console output from testing for pyftsubset when pyftsubset is missing. Fixes #88
  ...
* master: (29 commits)
  Fix some other tests that relied on the wrong ordering
  Sort the code points numerically before converting to unicode ranges
  Fix testdata dir name
  5.0.6
  Test in node 14
  Convert mocha config to mocha 8 compatible format
  Update non-breaking development dependencies
  Always include the space character U+20 in subsets to prevent Chrome from downloading the fallback font
  Do not preload unused variants in a self-hosting scenario
  Inject unicode-range into all the @font-face declarations for the given family
  Avoid unhandled promise rejection errors
  5.0.5
  Update assetgraph to ^6.0.8
  Update font-tracer to ^2.0.1
  5.0.4
  Reword the warning slightly
  Test that we don't overwrite an existing unicode-range
  Inject unicode-range into the original @font-face declaration when it's missing glyphs that are used
  Avoid using bluebird
  5.0.3
  ...
* master: (56 commits)
  Whoops, not woff2
  Fix bad url introduced in 9a441bc
  Try with only text-size:400% to make it easier to eyeball differences
  Switch the missingGlyphs case over to use some other fonts
  Use a different font in the unusedVariants case
  Roll back all the other attempts to fix the difference
  Apply font-size:1000% in the two test cases to see if it becomes reproducible on other platforms
  Try passing --glyphs=* to pyftsubset
  Try passing --recalc-bounds to pyftsubset
  Try passing --glyph-names to pyftsubset
  Try including the replacement character as well
  Try explicitly specifying a defaultViewport with a deviceScaleFactor of 1
  Try passing --unicodes=U+0,U+D,U+20 to pyftsubset
  Try specifying pyftsubset --passthrough-tables
  Try leaving out --obfuscate_names when calling pyftsubset
  Try adding spaces to the reference image test cases so that the space character gets included in the subset
  npm run coverage: Switch to the spec reporter so it's easier to see which reference image tests fail
  Use pip instead of homebrew in travis osx tests
  Install brotli and zopfli before fonttools
  Tell homebrew to always update
  ...
* master:
  Pull in refactoring of the reference images tests from feature/harfbuzz
* master:
  Avoid readFile/unlink race
  Update gettemporaryfilepath to ^1.0.1
…ad of using the wasm build"

This reverts commit 77e833d.
…ry instead of using the wasm build""

This reverts commit d545459.
@coveralls
Copy link

coveralls commented Jul 17, 2020

Pull Request Test Coverage Report for Build 504

  • 53 of 54 (98.15%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 89.853%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/parseCommandLineOptions.js 2 3 66.67%
Totals Coverage Status
Change from base Build 502: 0.2%
Covered Lines: 1078
Relevant Lines: 1150

💛 - Coveralls

@papandreou
Copy link
Collaborator Author

@Munter, I've been maintaining this branch for a while, but the tests kept failing despite progress happening on the harfbuzz side. Today I found out that it's actually further along, but some tweaks were needed in the code that uses the wasm:

That's enough to make the simple reference image tests pass here, and if I force the harfbuzz mode on globally, the whole test suite also passes.

I think it's in a good enough state to go in (behind a clearly marked "Experimental" flag).

@papandreou papandreou requested a review from Munter July 17, 2020 18:16
@Munter Munter merged commit 1b36876 into master Jul 25, 2020
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.

None yet

3 participants