Skip to content

feat: add font-family-fallbacks rule #174

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 23, 2025
Merged

feat: add font-family-fallbacks rule #174

merged 11 commits into from
Jul 23, 2025
+831 −14

Conversation

Tanujkanti4441
Copy link
Contributor

@Tanujkanti4441 Tanujkanti4441 commented Jun 12, 2025 •
edited
Loading

Prerequisites checklist

What is the purpose of this pull request?

Previous description : This is a draft PR to strategize the variable support for the rule font-family-fallbacks.

Latest description: Add a new rule called font-family-fallbacks to enforce the use of fallback fonts and a generic font last.

What changes did you make? (Give an overview)

Previous description: Added a rule called font-family-fallbacks. Right now, this PR just shows the strategy to enable variable support for this rule. So rule only contains the code to validate the use of sans-serif font in the variable (--my-font) used in font-family, and added two tests.

Latest description: Added a rule that will report following cases:

/* eslint css/font-family-fallbacks: "error" */

:root {
	--my-font: "Open Sans";
}

a {
	font-family: Arial; /* error here, as only one font is specified. */
}

b {
	font-family: Verdana, Arial, Helvetica; /* error here, as no generic font is specified in last. */
}

.foo {
	font-family: var(--my-font); /* error here as only one font is specified in variable --my-font. */
}

a {
	font: 16px Arial; /* error here, as only one font is specified. */
}

b {
	font: italic bold 2rem var(--my-font); /* error here as only one font is specified in variable --my-font. */
}

Related Issues

fixes #153

Is there anything you'd like reviewers to focus on?

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Jun 12, 2025
@Tanujkanti4441 Tanujkanti4441 mentioned this pull request Jun 12, 2025
Closed
1 task
@nzakas nzakas marked this pull request as draft June 12, 2025 18:18
@nzakas
Copy link
Member

nzakas commented Jun 12, 2025

Converting to a draft PR so we don't accidentally review and merge this.

@nzakas nzakas mentioned this pull request Jun 12, 2025
Open
1 task
@nzakas nzakas moved this from Needs Triage to Implementing in Triage Jun 18, 2025
@Tanujkanti4441 Tanujkanti4441 changed the title feat: add font-family-fallbacks rule (draft) feat: add font-family-fallbacks rule Jul 13, 2025
@Tanujkanti4441 Tanujkanti4441 marked this pull request as ready for review July 13, 2025 16:18
@lumirlumir lumirlumir added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 17, 2025
@lumirlumir lumirlumir requested a review from a team July 17, 2025 14:24
nzakas
nzakas reviewed Jul 21, 2025
View reviewed changes
docs/rules/font-family-fallbacks.md Outdated

The browser checks if the first font is available to the user's system, if the first font is not available, it tries the next font in the list. This process continues until a font is found. If none of the specified fonts are available, the browser uses the generic font family (like `serif`, `sans-serif`, etc.) at the end of the list.

It is often suggested to use a fallback font and a generic font last in the `font-family` list to ensure that text is always displayed in readable and consistent way, even if the preferred fonts are not available on the user's system or if the browser doesn't support the preferred font.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It is often suggested to use a fallback font and a generic font last in the `font-family` list to ensure that text is always displayed in readable and consistent way, even if the preferred fonts are not available on the user's system or if the browser doesn't support the preferred font.
It is a best practice to use a fallback font and a generic font last in the `font-family` list to ensure that text is always displayed in readable and consistent way, even if the preferred fonts are not available on the user's system or if the browser doesn't support the preferred font.

src/index.js Outdated
@@ -9,8 +9,18 @@

import { CSSLanguage } from "./languages/css-language.js";
import { CSSSourceCode } from "./languages/css-source-code.js";
import recommendedRules from "./build/recommended-config.js";
import rules from "./build/rules.js";
import fontFamilyFallbacks from "./rules/font-family-fallbacks.js";
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is reverting the recent work that we did to build up the rules list. Can you rebase and double-check?

src/rules/font-family-fallbacks.js Outdated
const variableMap = new Map();

return {
Rule(node) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic to find variables looks more complicated than necessary and potentially buggy. Please see how it's done in no-invalid-properties.

src/rules/font-family-fallbacks.js Outdated
}
},

Declaration(node) {
Copy link
Member

Choose a reason for hiding this comment

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

This function does a lot of traversing, which we try to avoid inside of rules. I think you can simplify this by using an ESQuery selector, like this:

[Rule > Block > Declaration[property="font-family"] > Value](node) {

src/rules/font-family-fallbacks.js Outdated
}
}

node.value.children.forEach(child => {
Copy link
Member

Choose a reason for hiding this comment

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

See if we can avoid multiple lookups like this (a.b.c.d) as the perf hit for continually looking up properties can add up over time.

src/rules/font-family-fallbacks.js Outdated

// If the value is a variable function, we need to check the variable value
if (
node.value.children[0].type === "Function" &&
Copy link
Member

Choose a reason for hiding this comment

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

Similar here, node.value.children[0] is used in a lot of places, so let's store that in a variable to avoid unnecessary lookups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

nzakas
nzakas approved these changes Jul 23, 2025
View reviewed changes
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas nzakas merged commit 5678024 into eslint:main Jul 23, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Jul 23, 2025
@github-actions github-actions bot mentioned this pull request Jul 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nzakas nzakas nzakas approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Rule: require-fallback-fonts
3 participants
@Tanujkanti4441 @nzakas @lumirlumir