Skip to content
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

Add support for border-image-source and border-image-slice. #2125

Merged
merged 31 commits into from
Apr 26, 2024

Conversation

xavidotron
Copy link
Contributor

@xavidotron xavidotron commented Apr 10, 2024

This adds basic support for border-image-source and border-image-slice.

This is sufficient for common uses of image borders in CSS.

It does not yet support non-default values for border-image-width, border-image-outset, or border-image-repeat, which I plan on addressing in future pull requests.

This is my first substantial contribution, so let me know if I should be putting this logic in a different place or otherwise approaching it differently. Thanks for your review!

First part of #2124.

@xavidotron xavidotron changed the title Add basic support for border-image-source and border-image-slice with percents. Add basic support for border-image-source and border-image-slice. Apr 11, 2024
@liZe
Copy link
Member

liZe commented Apr 24, 2024

That’s great, thanks a lot! The code seems to be almost perfect. ❤️

I’ll fix a couple of things and push small commits so that you can easily track what I’ve changed.

I think that we’ll merge this PR just after 62 is released (hopefully on Friday), because it already contains a lot of features and may cause a lot of bug reports.

@xavidotron
Copy link
Contributor Author

Great, thanks for your review!

@liZe
Copy link
Member

liZe commented Apr 25, 2024

It looks like I’ve changed a lot of lines, but the modifications are actually minor. Thanks a lot, that’s a great pull request! 🙏🏽

Here are the commits I’ve added:

  • Rename tests and remove comments related to specification (you can ignore this commit, that’s a problem in my head).
  • Simplify the SVG file used for tests (another pathological behavior).
  • Separate specified values from computed values. If you don’t know the difference between specified and computed values, this separation looks quite strange, but that’s important for many internal aspects of CSS. Blindly following what’s in the specification for this topic is always a good idea IMO, it avoids complex problems in the future. 😄
  • Clean some code, mainly to use longer variable names and follow the famous (but unwritten) WeasyPrint code style. I’ve also used the default image sizing algorithm that can handle funny cases like images with no intrinsic size.
  • Add CSS validation tests. We try to test specified values for complex cases, and generally rely on the rendering tests to check computed values. We’ll add more rendering tests when all the related properties are supported (I may have broken computed values for unsupported properties).

I also rebased the branch on top of "main", mainly to use and check the new linting rules.

If you want to support other properties, don’t hesitate to add new commits to this PR, we have some time before we merge it. There’s also the border-image shorthand to add into expanders.py.

@liZe liZe added this to the 63.0 milestone Apr 25, 2024
@xavidotron
Copy link
Contributor Author

I believe my existing change to expanders.py handles the border-image shorthand, though I may have missed something.

@xavidotron
Copy link
Contributor Author

I believe this now supports all the border-image-* properties, including some basic test coverage.

@liZe
Copy link
Member

liZe commented Apr 26, 2024

I believe my existing change to expanders.py handles the border-image shorthand, though I may have missed something.

I missed that!

I believe this now supports all the border-image-* properties, including some basic test coverage.

Thank you! If everything is OK, it may even be ready for version 62.

@liZe liZe added the feature New feature that should be supported label Apr 26, 2024
@liZe liZe modified the milestones: 63.0, 62.0 Apr 26, 2024
@liZe
Copy link
Member

liZe commented Apr 26, 2024

Thanks a lot for this pull request.

All the relevant tests of the W3C testing suite pass. 🚀

We could add more drawing tests in WeasyPrint, but the code is already in very good shape and W3C tests give a good reason to be optimistic. Let’s merge if everything’s OK for you!

@xavidotron
Copy link
Contributor Author

Sounds great! This works for my needs, so I'd be happy for it to be merged. Thanks for your work to improve this as well!

@liZe liZe merged commit df7c9b9 into Kozea:main Apr 26, 2024
13 checks passed
@liZe liZe changed the title Add basic support for border-image-source and border-image-slice. Add support for border-image-source and border-image-slice. Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature that should be supported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants