ladybird icon indicating copy to clipboard operation
ladybird copied to clipboard

LibWeb/CSS: Use default font metrics when computing `:root` font size

Open dzfrias opened this issue 1 year ago • 5 comments

Prevents repeatedly building font size when using rem in the :root element.

Fixes #139 and #339. Resetera font is still larger than it should be, but it no longer continuously grows until crash.

dzfrias avatar Jul 15 '24 23:07 dzfrias

Not sure if https://github.com/dzfrias/ladybird/blob/568de76fd26c1f5349132834e236a43f3cd5cfbc/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp#L2218 is also relevant here. I'm not sure why absolutize_values doesn't mess everything up since it's also using root font metrics... regardless, it didn't cause any issues.

dzfrias avatar Jul 15 '24 23:07 dzfrias

Not sure if dzfrias/ladybird@568de76/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp#L2218 is also relevant here. I'm not sure why absolutize_values doesn't mess everything up since it's also using root font metrics... regardless, it didn't cause any issues.

The way it works is a bit messy unfortunately. I think this also needs updating, and might be why the font is still larger than it should be, though I don't know why it's not continuously growing.

AtkinsSJ avatar Jul 16 '24 10:07 AtkinsSJ

I'm having a little bit of trouble since create_document_style also uses absolutize_values, and I'm not sure what the correct behavior should be. I'm not fully sure how the font selection stuffs works.

Is there a certain reference or algorithm that these methods of StyleComputer are following? Is it all from the CSS spec?

dzfrias avatar Jul 17 '24 02:07 dzfrias

I don't think there's a spec algorithm. Most of the CSS specs just tell you what the end result should be and you have to figure it out yourself.

The relevant spec would be https://www.w3.org/TR/css-fonts-4/ - I don't think any others will affect this.

AtkinsSJ avatar Jul 17 '24 06:07 AtkinsSJ

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

github-actions[bot] avatar Dec 07 '24 21:12 github-actions[bot]

The original issue is still there on master, so it should be cool to continue with this PR. @dzfrias is this PR still blocked by "the way we do fonts right now"?

shlyakpavel avatar Dec 15 '24 15:12 shlyakpavel

It's mostly blocked because I'm hesitant to try to merge something that I don't think is a particularly good solution to this problem. My solution is more of a patch over the problem, and I don't understand the CSS computation code well enough to know if the changes I'm making are appropriate.

That said, I haven't read the relevant code in a while now, so things might've changed.

dzfrias avatar Dec 15 '24 15:12 dzfrias

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

github-actions[bot] avatar Jan 08 '25 01:01 github-actions[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

github-actions[bot] avatar Jan 16 '25 01:01 github-actions[bot]