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

Remove light weight font rendering #7880

Closed
wants to merge 2 commits into from
Closed

Remove light weight font rendering #7880

wants to merge 2 commits into from

Conversation

globau
Copy link

@globau globau commented Feb 23, 2022

Specifying the font-weight here is harmful when using fonts that support
Light or variable weights.

Signed-off-by: glob byron@glob.com.au

Notes: Remove light weight font rendering


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://pr7880--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Specifying the font-weight here is harmful when using fonts that support
Light or variable weights.

Fixes element-hq/element-web#21171
@globau globau requested a review from a team as a code owner February 23, 2022 07:16
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

It appears that there a couple more occurence of font-weight: 300 in the codebase that would probably be suceptible to the same issue you raised. Could you update those ones too?

I looked at @font-face declaration and it appears that we're loading the following font weights: 400, 500, 600, 700 for Inter. And 400 and 700 for Inconsolata

Will also loop in the design team to confirm this is a change we're comfortable making

@germain-gg germain-gg requested a review from a team February 23, 2022 08:57
@germain-gg germain-gg changed the title Remove harmfull .mx_Dialog font-weight definition Remove light weight font rendering Feb 23, 2022
@globau globau requested review from germain-gg and removed request for a team February 23, 2022 10:31
@jryans jryans requested a review from a team February 23, 2022 14:12
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Code looks good to me.
Please keep the design reviewers on. Without their approval we will not be abe to move this pull request forward

@janogarcia
Copy link
Contributor

Unsure where those lighter fonts are being used, but, yeah, the current minimum weight should be 400.

Copy link
Contributor

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

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

Are the affected components now inheriting the base font-weight of 400?

@globau
Copy link
Author

globau commented Feb 24, 2022

Are the affected components now inheriting the base font-weight of 400?

Excellent question!

As far as I can tell mx_Dialog always cascades to 400.

I can't find where mx_CreateRoom is used (no references to it under the src directory).

I'm not sure what a GroupView is so I can't verify manually in the Element client, but as far as I can tell (with my very limited knowledge here) the default weight for mx_GroupView_header_shortDesc should be 400.

@t3chguy
Copy link
Member

t3chguy commented Feb 24, 2022

I can't find where mx_CreateRoom is used (no references to it under the src directory).

It is no longer used, can we kill that entire file as part of this PR please.

@globau
Copy link
Author

globau commented Feb 24, 2022

I can't find where mx_CreateRoom is used (no references to it under the src directory).

It is no longer used, can we kill that entire file as part of this PR please.

Removing an potentially unused class seems to be unrelated to this issue, and I'd prefer if it were fixed in a separate PR.

@t3chguy
Copy link
Member

t3chguy commented Feb 24, 2022

Editing a class which does not cause your bug due to being unused is also unrelated to this issue though?

@janogarcia
Copy link
Contributor

@globau Thanks for checking!

@t3chguy @gsouquet Can you please check/confirm if removing the declaration for 300 will inherit 400 for the cases where @globau is unsure how to test for?

@globau
Copy link
Author

globau commented Mar 6, 2022

@janogarcia To move this along how about instead of removing the font-weight: 300 lines I replace them with font-weight: 400?

That will guarantee correct rendering regardless of context, however it might mean clean up is required in a different PR due to possible redundancy.

@janogarcia
Copy link
Contributor

@globau Sounds reasonable (ideally, if we could avoid those extra declarations, the better), but I'm afraid that would be up to @t3chguy and @gsouquet.

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.

4 participants