Skip to content

check/colorfont_tables: update conditions#3889

Merged
felipesanches merged 12 commits intofonttools:mainfrom
m4rc1e:max-color
Oct 13, 2022
Merged

check/colorfont_tables: update conditions#3889
felipesanches merged 12 commits intofonttools:mainfrom
m4rc1e:max-color

Conversation

@m4rc1e
Copy link
Copy Markdown
Collaborator

@m4rc1e m4rc1e commented Aug 31, 2022

Description

Our current implementation for com.google.fonts/check/colorfont_tables isn't strict enough.

Fixes #3888

@anthrotype Have I covered all the cases?

To Do

  • update CHANGELOG.md
  • wait for all checks to pass
  • request a review

Comment thread Lib/fontbakery/profiles/googlefonts.py
Comment thread Lib/fontbakery/profiles/googlefonts.py Outdated
Comment thread Lib/fontbakery/profiles/googlefonts.py Outdated
Comment thread Lib/fontbakery/profiles/googlefonts.py Outdated
Comment thread Lib/fontbakery/profiles/googlefonts.py Outdated
Comment thread Lib/fontbakery/profiles/googlefonts.py Outdated
@felipesanches
Copy link
Copy Markdown
Collaborator

Rationale text needs to be updated to reflect the reasoning of these updated check criteria. It currently reads:

No one color font format supports all major user agents, but the combination
of COLR & SVG tables is pretty good.
A smart server could prune away one or the other based on user agent,
and a dumb server will at least have something that works.
Fonts that do not pass this check can be fixed with the maximum_color tool
available at https://github.com/googlefonts/nanoemoji

@felipesanches
Copy link
Copy Markdown
Collaborator

Also, code-tests need to be updated since they're enforcing the incorrect tag "SVG", while it should be "SVG " instead.

@felipesanches
Copy link
Copy Markdown
Collaborator

Docstring on the code-test must also be updated. It still reads as Fonts must have neither or both the tables 'COLR' and 'SVG'."

@felipesanches
Copy link
Copy Markdown
Collaborator

And please, also add a CHANGELOG.md entry.

@m4rc1e
Copy link
Copy Markdown
Collaborator Author

m4rc1e commented Sep 16, 2022

Done and apologies for the delay. I forgot about this one.

@m4rc1e
Copy link
Copy Markdown
Collaborator Author

m4rc1e commented Sep 16, 2022

CI errors seem to involve the whole timeout fiasco which #3892 solves.

@m4rc1e
Copy link
Copy Markdown
Collaborator Author

m4rc1e commented Sep 20, 2022

Merge conflict fixed.

yield FAIL, Message(
"drop-svg",
"Font has a COLR v0 table, which is already widely supported, "
"so the SVG table isn't needed."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adobe CC does not support COLR table (and probably will not do any time soon, given the Adobe’s history with SVG table), so if a font for GF want to also support Adobe CC, do you want to force it to ship to versions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm fair enough.. I would demote that check to a WARNING, saying that COLRv0-only is OK for browser use, unless you really need SVG support for desktop apps that don't support COLR. They probably want to keep things simple and not allow too many options when onboarding new color fonts. /cc @rsheeter

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@khaledhosny maybe file a separate issue so we can discuss there

@m4rc1e m4rc1e force-pushed the max-color branch 2 times, most recently from 577f2fa to 2bf75ba Compare October 12, 2022 08:52
@m4rc1e
Copy link
Copy Markdown
Collaborator Author

m4rc1e commented Oct 12, 2022

@felipesanches PTAL. I keep having to rebase this PR so it'll be good to get this in.

@felipesanches felipesanches merged commit b27edd3 into fonttools:main Oct 13, 2022
@m4rc1e
Copy link
Copy Markdown
Collaborator Author

m4rc1e commented Oct 13, 2022

Thank you!

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.

update check/colorfont_tables criteria

4 participants