Skip to content

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Jul 7, 2025

Fixes a regression from dropping the SCSS compiler that broke font themes like OpenDyslexic. The old code relied on the SCSS compiler to automatically correct the order of the CSS rules, ensuring the @font-face declaration was always valid. The server now correctly generates the @font-face rule at the top level of the stylesheet, fixing the previously invalid nested CSS.

Introduced in : f1448fc via #52249

@nfebe nfebe requested a review from a team as a code owner July 7, 2025 19:45
@nfebe nfebe requested review from yemkareems and removed request for a team July 7, 2025 19:45
@nfebe nfebe enabled auto-merge July 7, 2025 19:48
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

That looks wrong 🤔
It means the style will be applied everywhere and not just within the data attribute?

@nfebe
Copy link
Contributor Author

nfebe commented Jul 8, 2025

That looks wrong 🤔
It means the style will be applied everywhere and not just within the data attribute?

You're right that custom style is global, but that only defines the styles; it's only applied by the variables scoped within the data-theme attribute.

And by the way, the Dyslexic font does not work any more when you to to "Personal settings > Appearance and accessibility"


EDIT

Thought about it, in terms of @-rules only, so it appears it needs a more dynamic approach. Updating.

@nfebe nfebe force-pushed the fix/dyslexia-font-not-loading branch from ccebc7f to b301f14 Compare July 8, 2025 11:00
@nfebe nfebe requested a review from skjnldsv July 8, 2025 11:01
@susnux
Copy link
Contributor

susnux commented Jul 8, 2025

Alternative would be to add a new field like atRules or globalRules that would be also hoisted for scoped themes.
As mentioned the at-rules like @font-face or @media cannot be nested.

@skjnldsv
Copy link
Member

skjnldsv commented Jul 8, 2025

Also, keep in mind not every theme will have font in the custom CSS entry.
The API is designed to be flexible and support themes at a wider level when we'll want to open it.
E.g, apps in the AppStore that provides custom themes

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Fixes dyslexia at least

@nickvergessen
Copy link
Member

The API is designed to be flexible and support themes at a wider level when we'll want to open it.

We need to backport this to 31, so a simple solution that works for designed use cases (dark, contrast and dyslexia) is good enough for now I'd say.

@susnux
Copy link
Contributor

susnux commented Jul 9, 2025

We need to backport this to 31, so a simple solution that works for designed use cases (dark, contrast and dyslexia) is good enough for now I'd say.

Yes at least for the moment as there are no external themes

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Solves the issue, but very fragile code.
As soon as e.g. there is a @ in a comment it breaks.

But for the moment it works as we only have our themes and there is no way for external themes.

Fixes a regression from dropping the SCSS compiler that broke
font themes like OpenDyslexic. The old code relied on the SCSS
compiler to automatically correct the order of the CSS rules,
ensuring the @font-face declaration was always valid.
The server now correctly generates the `@font-face` rule at
the top level of the stylesheet, fixing the previously invalid nested CSS.

Introduced in : f1448fc

Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the fix/dyslexia-font-not-loading branch from b301f14 to 625c126 Compare July 11, 2025 11:27
@nfebe
Copy link
Contributor Author

nfebe commented Jul 11, 2025

As soon as e.g. there is a @ in a comment it breaks.

Added a two lines to strip comments, but yeah we definitely need a proper way to do this eventually.

@nfebe
Copy link
Contributor Author

nfebe commented Jul 11, 2025

/backport to stable31

@nfebe nfebe merged commit 9a8e74d into master Jul 11, 2025
264 of 287 checks passed
@nfebe nfebe deleted the fix/dyslexia-font-not-loading branch July 11, 2025 19:49
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
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.

4 participants