Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

load twemoji dynamically as colr or sbix; fix monospace#3008

Merged
bwindels merged 9 commits into
developfrom
matthew/fontmanager
May 22, 2019
Merged

load twemoji dynamically as colr or sbix; fix monospace#3008
bwindels merged 9 commits into
developfrom
matthew/fontmanager

Conversation

@ara4n
Copy link
Copy Markdown
Member

@ara4n ara4n commented May 22, 2019

This uses a stripped down fork of https://github.com/RoelN/ChromaCheck/blob/master/chromacheck.js to determine whether your browser can render COLR fonts or not. If it can, it dynamically loads twemoji as COLR, failing that sbix.

The sbix version of the font was created by converting the colr one using TransType.

This also makes monospace fonts consistent as Inconsolata, to avoid inconsistencies from depending on system fonts.

If Twemoji fails entierly, we explicitly fall back to Apple or Noto emoji if we have them installed to avoid half-assed emoji in Arial, Helvetica, Courier etc taking priority.

should fix element-hq/element-web#9651 and element-hq/element-web#9765

@ara4n ara4n requested a review from a team May 22, 2019 01:41
@bwindels bwindels requested review from bwindels and removed request for a team May 22, 2019 07:46
Comment thread res/themes/light/css/_fonts.scss
Comment thread src/utils/FontManager.js Outdated
// we programatically add the right fontface.
let font;
if (await isColrFontSupported()) {
font = new FontFace("Twemoji", "url('../../fonts/Twemoji_Mozilla/TwemojiMozilla-colr.woff2')", {});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should be able to require the font path like we do for images in JS, and that will allow you to keep the cache busting.

Copy link
Copy Markdown
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

looks good, would just consider excluding FF from the check.

Comment thread src/utils/FontManager.js

// we programatically add the right fontface.
let font;
if (await isColrFontSupported()) {
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.

This check doesn't work on FF with content blocking enabled (enabled by default in Tor browser IIRC), even when giving the permission to extract canvas data. So it's going for the SBIX font without really needing.

We could sniff the user agent to exclude FF from the check, as it has supported COLR fonts since version 26, according to colorfonts.wtf?

@bwindels bwindels requested a review from jryans May 22, 2019 09:15
@bwindels
Copy link
Copy Markdown
Contributor

addressed the comments so we can get this in the RC asap. Can you have another look @jryans ?

@bwindels
Copy link
Copy Markdown
Contributor

bwindels commented May 22, 2019

The big emojis appear a bit clipped atm in Chrome on Linux:
image
Don't think this is a blocking issue though. Changing the line-height from 52px to 66px fixes it, but then it looks a bit tall in other browsers.

@bwindels
Copy link
Copy Markdown
Contributor

Tested on FF and Chrome with added changes and without riot-web webpack config PR, working fine.

Copy link
Copy Markdown
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Great, thanks for working on this! 😁 I have also tested with Firefox, Chrome, and Safari on macOS.

I suppose Windows and Electron on various platforms would be nice to know about, but can be tested after merging I think.

Comment thread res/css/_common.scss Outdated
Comment thread src/utils/FontManager.js
}

// Firefox has supported COLR fonts since version 26
// but doesn't support the check below with content blocking enabled.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am curious what actually goes wrong with the content blocking, but it doesn't seem worth digging into right now...

Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reactions: Mix of color, non-color emojis and boxes in reaction bar

3 participants