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

Override font-family-modern font weight #231

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented Jan 31, 2025

Description

Tumblr's stylesheets often use an unusual font weight of 350 on the new Favorit Modern font, specified with the --font-family-modern variable. As of #215, we override the font face on these elements, but don't override the weight, which results in unusually thin text on font faces (or, more specifically, font face/operating system/browser combinations) which support this value.

This inserts a stylesheet that sets the font weight back to normal on these elements, which requires shadowing every CSS rule which sets this font face and font weight combination. Copying the native selectors exactly ensures that this uses the same specificity as the native rules and therefore shouldn't have unintended consequences (assuming that the native rules don't rely on CSS insertion order, and it's a redpop bug if they do).

There are, unfortunately, way too many of these rules to collect by hand. This therefore uses a node script to collect the rules to override, and unlike in #216, making use of/maintaining the script isn't really optional to keep this functionality working. The script, at least, doesn't make use of puppeteer, so it's simpler. (It does use a library—https://github.com/reworkcss/css and https://github.com/brettstimmerman/mensch are essentially equivalent, and the postcss parser likely is too—because node.js doesn't have css parsing built-in and while I've done it with regex before, it's not pretty.)

Resolves #223.

Testing steps

  • Load the extension without this PR and set the font override to Helvetica Neue. Observe unusually thin text on activity items and the communities settings page. (I observed this on MacOS in Firefox with Helvetica Neue, American Typewriter, and Gill Sans.)
  • Load the extension with this PR and set the font override to Helvetica Neue. Confirm that text is no longer thin.
  • Set the font override to "default." Confirm that no change is made to the Favorit Modern font weight.
  • Set the font override to "custom" and don't type anything in the box. Confirm that no change is made to the Favorit Modern font weight.
  • Load the extension in Firefox with this PR and set the font override to Helvetica Neue. Confirm that changes to the stylesheet are instantly applied to the page (when the file is saved if using web-ext run, or when the extension is reloaded from another tab). This is a proxy for Firefox autoupdate; see Fix static styles failing to update XKit-Rewritten#1619.
  • Edit or delete the generated CSS file and confirm that npm run update-font-weight-override produces it correctly.

@marcustyphoon marcustyphoon marked this pull request as ready for review January 31, 2025 06:08
@AprilSylph
Copy link
Owner

Question: why do we need a dev script for this? Can't we do the same thing at runtime instead? It's not perfectly efficient to build the stylesheet on every pageload, I know, but it would be perfectly robust, would it not?

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Jan 31, 2025

...Hm. Hmmmmmm. Iiiiinteresting.

a) I was going to say it's an enormous number of network requests to literally do it exactly this way, which it absolutely is (close to 100 without any filtering). But as a clever reader of the dev script might note, the client is going to perform and cache those fetches anyway at some point, so it could probably be free in certain circumstances (not immediately sure how to differentiate which circumstances, though).

b) A guaranteed-free way would be to generate rules in real time as Tumblr loads styles that need to be overridden, presumably from document.stylesheets (I did reference #216 (comment) to make an earlier version of this)*. The only thing I'm not sure about offhand is how to detect the addition of new styles that'll have to be processed (we can detect style and link elements, but they load asynchronously, right?) I'm going to go look through the CSSOM APIs again to see if there's an obvious elegant way to do this that I'm not currently aware of.

Let me think about this!

*or possibly via a mutationobserver checking the computed style of newly added elements, but that would probably perform poorly and seems mostly like the naive way to do it

@AprilSylph
Copy link
Owner

*or possibly via a mutationobserver checking the computed style of newly added elements, but that would probably perform poorly and seems mostly like the naive way to do it

Yeah, I almost recommended doing this but then realised it would be a pain to undo—I'm pretty sure we don't want to bump up the font weight if Favorit Modern is actually in use? The weights are weird, and I'm pretty sure 500 is "medium", which is used as bold because browser rendering of web fonts is actually a little broken.

@marcustyphoon
Copy link
Collaborator Author

I'm pretty sure we don't want to bump up the font weight if Favorit Modern is actually in use?

We indeed do not want to do that.

But yeah, I mean, it's definitely possible to make it undoable depending on what action one actually takes when discovering an element with the target computed style. Add a classname to it, say, or simply use it as an indicator that one needs to go process document.stylesheets again. Have to be careful not to repeatedly trigger style recalculation, would be the major concern, I imagine.

@marcustyphoon
Copy link
Collaborator Author

Aside: I've just noticed that I copied the file name casing from paletteData.json in a bunch of my stuff, but we generally snake_case file names. Oops!

@marcustyphoon marcustyphoon marked this pull request as draft January 31, 2025 13:39
@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Jan 31, 2025

Just discovered that at least one target rule (.stats-FzBTa .link-GpO9n) is inside a media query that does actually change the font face and font weight. No ill effect (the non-mobile-width rule was, naturally, Favorit 400 anyway), but probably better safe than sorry.

  • consider finding a way to process these or just exclude them

Edit: Processing them is doable but probably not worth it vs just excluding them. I can't even find where these rules are applied. marcustyphoon/Palettes-for-Tumblr@font-weight-override...marcustyphoon:Palettes-for-Tumblr:font-weight-override-media

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants