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

Push MathJax font styles to document head to allow them to download #2078

Merged
merged 7 commits into from
Feb 8, 2022

Conversation

grant-cleary
Copy link
Contributor

So... It turns out that MathJax fonts aren't loading properly within the html-block control when rendering is deferred; specifically because MathJax sprouts its font at-rules in a style element in whatever document context you give it. It turns out that within a component's shadowDOM, these fonts are just never downloaded.

My proposed solution is to mimic what MathJax does with its styles when rendering isn't deferred: sprout the font at-rules in the document head. This allows them to download as one would expect.

As usual, I'm entirely open to other ideas here. :)

@grant-cleary grant-cleary requested a review from a team as a code owner February 3, 2022 19:51
@@ -158,6 +183,23 @@ export function loadMathJax(mathJaxConfig) {
}
};

if (mathJaxConfig && mathJaxConfig.deferTypeset && !document.head.querySelector('#d2l-mathjax-fonts') && !document.head.querySelector('#MJX-CHTML-styles')) {
Copy link
Contributor Author

@grant-cleary grant-cleary Feb 3, 2022

Choose a reason for hiding this comment

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

MJX-CHTML-styles is the id of the style element that MathJax applies, which includes the font at-rules. If it exists in the document head already, we don't need to add our own. Notably, we also don't want to use this id because MathJax does a lot more than just the at-rules with those styles, so we also don't want to clobber MathJax's styles with these.

@@ -1,4 +1,29 @@
const mathjaxContextAttribute = 'data-mathjax-context';
const mathjaxBaseUrl = 'https://s.brightspace.com/lib/mathjax/3.1.2';

const mathjaxFontMappings = new Map([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These mappings are pulled from here: https://github.com/mathjax/MathJax-src/blob/master/ts/output/chtml/fonts/tex.ts

Of course, this can certainly change in the future, so we'll need to be careful when we update MathJax to update these fonts as well. It's not ideal, but MathJax doesn't expose these in any kind of convenient API so we can't really scrape them.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

Visual diff tests failed - pull request #2079 has been opened with the updated goldens.

Copy link
Contributor

@dbatiste dbatiste left a comment

Choose a reason for hiding this comment

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

I really wish MathJax would provide an out of the box solution that handles all this so we would not have to be duct-taping this stuff together. Any thought on raising this with them in response to the thread that talks about how use MathJax in shadowDOM, or starting a new thread?

It would be much worthwhile to include a comment at the top of this file indicating the details of what needs to be checked when upgrading MathJax.

Aside from that, looks ok.

@grant-cleary
Copy link
Contributor Author

I really wish MathJax would provide an out of the box solution that handles all this so we would not have to be duct-taping this stuff together. Any thought on raising this with them in response to the thread that talks about how use MathJax in shadowDOM, or starting a new thread?

Yep, I can do that for sure.

It would be much worthwhile to include a comment at the top of this file indicating the details of what needs to be checked when upgrading MathJax.

Good point, yeah. I also find myself wondering if this would be a good candidate for automating deployment to the CDN, much like TinyMCE. That wouldn't change needing to add a comment here, but it's food for thought I think.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

Visual diff tests failed - pull request #2088 has been opened with the updated goldens.

@grant-cleary
Copy link
Contributor Author

I'm very certain my addition of a single comment to the top of this file didn't start setting d2l-dialog visual diff tests on fire...

@grant-cleary
Copy link
Contributor Author

Ack, accidentally deleted my previous comment... Any objections if I merge in the new goldens? These reports also failed across other PRs.

@dbatiste
Copy link
Contributor

dbatiste commented Feb 7, 2022

Ack, accidentally deleted my previous comment... Any objections if I merge in the new goldens? These reports also failed across other PRs.

Seems like the browser is doing something differently with shadows. Is this happening consistently?

@grant-cleary
Copy link
Contributor Author

grant-cleary commented Feb 7, 2022

Seems like the browser is doing something differently with shadows. Is this happening consistently?

Failures have been consistent across three runs, and I noticed the same set of failures here: #2083, #2086

@dbatiste
Copy link
Contributor

dbatiste commented Feb 7, 2022

So yeah, if they are consistent I think it's fine.

…-2078

Updating Visual Diff Goldens for PR 2078
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

Visual diff tests failed - pull request #2090 has been opened with the updated goldens.

github-actions and others added 2 commits February 8, 2022 15:34
@annabelsegalld2l
Copy link
Contributor

Hey Grant, you can merge your changes in now. After your PR is merged, I am going to remove some of the flaky tests that are causing us trouble so hopefully we won't need to re-run 5 times the next time we run into an issue like this 😄

@grant-cleary
Copy link
Contributor Author

grant-cleary commented Feb 8, 2022

Hey Grant, you can merge your changes in now. After your PR is merged, I am going to remove some of the flaky tests that are causing us trouble so hopefully we won't need to re-run 5 times the next time we run into an issue like this 😄

Oh, haha, fair enough. Unfortunately I don't have permission to skip required steps in this repo so I think someone else would need to push it through for me. 😬

EDIT: I say as tests finally finish and the run goes green...

@grant-cleary grant-cleary merged commit fb6e62c into main Feb 8, 2022
@grant-cleary grant-cleary deleted the gcleary/RenderMathJaxFonts branch February 8, 2022 16:00
@ghost
Copy link

ghost commented Feb 8, 2022

🎉 This PR is included in version 1.230.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants