Skip to content

Support CSS font size keywords #3242

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

peschwartz
Copy link

@peschwartz peschwartz commented Mar 7, 2025

Added backend support for CSS font-size keywords to Android, Cocoa, iOS, GTK, and Windows and updated the core and travertino testing suites. Includes updated documentation.

Fixes #1814

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

sasoder and others added 2 commits March 7, 2025 10:46
Add: CSS keyword constants to Travertino and update core style component #17
Update: testbed to handle CSS keywords and improve coverage #24
Add: CSS font size keywords and tests for iOS #5 #10 (#18)
Add: windows CSS keyword support and tests #6 #11 (#22)
Add: Cocoa CSS keywords support and tests #3 #8 (#23)
Add: Gtk css keywords support and tests #4 #9 (#27)
Fix: remove xxxl keywords #29
Docs: add css keywords to style reference (#33)
Add: Android support for CSS font keywords and tests #2 #7 (#28)

----------
Co-authored-by: Phoebe Schwartz <[email protected]>
Co-authored-by: KlaraLindemalm <[email protected]>
Co-authored-by: Jacmol <[email protected]>
Co-authored-by: carltestar <[email protected]>
fix relative size tests to include parent size if one exists
Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

Thanks, this looks very thorough.

fix: remove all relative_font_size instances and update documentation

---------

Co-authored-by: phoebe <[email protected]>
@peschwartz
Copy link
Author

Hello! We've made the changes to remove the xxxl size and the relative sizes. It passes all of the CI checks but for some reason, it is not passing the docs build. Any ideas as to why?

@freakboy3742
Copy link
Member

Hello! We've made the changes to remove the xxxl size and the relative sizes. It passes all of the CI checks but for some reason, it is not passing the docs build. Any ideas as to why?

It looks like it might be related to RTD's recent switchover to their new management UI. Your original docs builds seems to have fallen into a crack somewhere; I've manually restarted the build, and it seems to have worked. Apologies for the confusion!

@peschwartz peschwartz requested a review from mhsmith March 20, 2025 12:46
Updated code from review.
@peschwartz
Copy link
Author

@mhsmith I've implemented the changes you suggested! Let me know if there is more we can do for this PR.

@freakboy3742
Copy link
Member

@peschwartz Thanks for the update - @mhsmith and I are both travelling for work at the moment, so we're a little busy; we'll take a look as soon as we get a chance.

Comment on lines +100 to +101
font_size = DEFAULT_FONT.Size
font_size *= FONT_SIZE_SCALE[self.interface.size]
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
font_size = DEFAULT_FONT.Size
font_size *= FONT_SIZE_SCALE[self.interface.size]
font_size = DEFAULT_FONT.Size * FONT_SIZE_SCALE[self.interface.size]

Comment on lines +100 to +101
base_size = NSFont.systemFontSize
font_size = base_size * FONT_SIZE_SCALE[self.interface.size]
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
base_size = NSFont.systemFontSize
font_size = base_size * FONT_SIZE_SCALE[self.interface.size]
font_size = NSFont.systemFontSize * FONT_SIZE_SCALE[self.interface.size]

Comment on lines +112 to +113
default = base_size * FONT_SIZE_SCALE[self.interface.size]
return default
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
default = base_size * FONT_SIZE_SCALE[self.interface.size]
return default
return base_size * FONT_SIZE_SCALE[self.interface.size]

@mhsmith
Copy link
Member

mhsmith commented Apr 14, 2025

I'm not able to apply these changes because you didn't enable the "allow edits by maintainers" option. Please do that in future PRs.

Meanwhile, you don't need to do anything; I'll merge this branch by creating a separate PR.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

OK, actually there are a few more issues, so I'll leave you to deal with them. Please enable the "allow edits by maintainers" option on this PR anyway – it's at the bottom of the right hand column.

And one more workflow point – please don't click the "resolve" button on review comments – leave that to the person who posted it. This will make it easier to keep track of what's left to be reviewed. (If you accept a suggested change, then it will resolve automatically – this is fine.)

Thanks for your updates, and sorry this is taking so long, but I think we're almost finished now.

@@ -0,0 +1 @@
Added support for absolute CSS font size keywords in Android, Cocoa, GTK, iOS, and Windows.
Copy link
Member

Choose a reason for hiding this comment

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

misc change notes are hidden in the release notes. This is a publicly-visible feature, so the file should be renamed to 1814.feature.rst.

@@ -96,6 +99,7 @@ def typeface(self, *, default=Typeface.DEFAULT):
def size(self, *, default=None):
"""Return the font size in physical pixels."""
context = MainActivity.singletonThis
base_size = 14
Copy link
Member

@mhsmith mhsmith Apr 14, 2025

Choose a reason for hiding this comment

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

This size will need to be scaled in a similar way to the TypedValue.applyDimension code below. Otherwise, the text will be too small on high DPI devices (which is virtually all of them). For example, if I edit examples/font_size to use the font size keywords:

Screenshot_20250414_215333

As mentioned at #1814 (comment), on Android, as far as I know, "medium" should be the same as the default for every widget except the TextInput, where the default should be larger.

Please make a similar visual check on as many other platforms as you can, and let me know which ones you've checked. Windows in high DPI mode is the most likely one to have a problem.

@mhsmith
Copy link
Member

mhsmith commented Apr 14, 2025

When testing on Android, I got this error:

E/AndroidRuntime: Caused by: com.chaquo.python.PyException: ModuleNotFoundError: No module named 'travertino.properties.shorthand'; 'travertino.properties' is not a package
E/AndroidRuntime:   at <python>.toga.style.pack.<module>(pack.py:40)
E/AndroidRuntime:   at <python>.toga.style.<module>(__init__.py:2)
E/AndroidRuntime:   at <python>.font_size.app.<module>(app.py:2)

And sure enough, travertino/properties/__init__.py does not exist. This is an oversight, and I'm not sure why it hasn't caused problems before, but you might as well fix it in this PR if you encounter it.

@freakboy3742
Copy link
Member

And sure enough, travertino/properties/__init__.py does not exist. This is an oversight, and I'm not sure why it hasn't caused problems before, but you might as well fix it in this PR if you encounter it.

That is... shocking... how has that not caused problems? I can only presume it's because all the existing usage is from .properties.foo import ...?

@mhsmith
Copy link
Member

mhsmith commented Apr 15, 2025

Maybe it's been treated as an implicit namespace package, in which case Chaquopy's custom importer may be the reason why it fails on that platform. Still not sure why it isn't breaking the testbed as well, but it's not worth looking into any further.

@peschwartz
Copy link
Author

Hey @mhsmith,

We've looked at this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork and we aren't seeing this option to allow you maintainer access. How do you think we should proceed? I can give you direct access to our repository if that works.

@mhsmith
Copy link
Member

mhsmith commented Apr 15, 2025

It should be at the bottom of the right hand column, as shown in the screenshot on that page. If you can't see it, never mind, I can continue suggesting the changes to you, as we have been doing so far.

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.

Support CSS font size keywords
4 participants