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

Support for object-fit css property #669

Closed
guojiaqi1027 opened this issue Aug 14, 2018 · 15 comments · Fixed by #875
Closed

Support for object-fit css property #669

guojiaqi1027 opened this issue Aug 14, 2018 · 15 comments · Fixed by #875
Labels
feature New feature that should be supported good first issue Issues that can be quite easily solved by Python developers with a good CSS background
Milestone

Comments

@guojiaqi1027
Copy link

Hi:

I'm using WeasyPrint for generate PDF and the css property object-fit is not supported yet.

Do you have a plan to support this feature as this is really common used in my project.

Thanks.

@liZe liZe added feature New feature that should be supported good first issue Issues that can be quite easily solved by Python developers with a good CSS background labels Aug 14, 2018
@liZe
Copy link
Member

liZe commented Aug 14, 2018

Hello,

It's not supported yet, but it looks pretty easy to handle (in draw.py, draw_background_image function). If anyone is interested in adding this feature, I'd be happy to help!

@mbarkhau
Copy link
Contributor

Very tempting.

@mbarkhau
Copy link
Contributor

Not quite so simple (at least for me). I think I at least have the css parsing working. There must be some other code path than draw_background_image for normal <img> elements.

@liZe
Copy link
Member

liZe commented May 31, 2019

There must be some other code path than draw_background_image for normal <img> elements.

Yes, it's in RasterImage.draw() and SVGImage.draw(), in images.py. You have to change the context.scale() with the adapted context.scale() and context.translate() according to the object-fit value.

@mbarkhau
Copy link
Contributor

I think I have object-fit working now, but I guess object-position is also not supported...

@liZe
Copy link
Member

liZe commented May 31, 2019

I think I have object-fit working now

Thanks a lot! 🎈 🎉

but I guess object-position is also not supported

No, it's not. I can do it if you don't want to!

@mbarkhau
Copy link
Contributor

I'm going to try and continue tomorrow, but if you want to give feedback I'd appreciate it. I think the layout code that is still missing the use of object-position is better to move out of draw.py, probably to replaced.py.

@mbarkhau
Copy link
Contributor

I have a file that I'm testing with and manually comparing to what browsers render. What would the appropriate approach be to write tests for this?

@liZe
Copy link
Member

liZe commented Jun 1, 2019

I'm going to try and continue tomorrow, but if you want to give feedback I'd appreciate it.

Your PR is simple and clean, I really appreciate it. Thank you.

The major thing I'd change is to put percentage in layout/percentage.py and not in a module on its own.

I think the layout code that is still missing the use of object-position is better to move out of draw.py, probably to replaced.py.

Yes, it's probably better in replaced.py.

I have a file that I'm testing with and manually comparing to what browsers render. What would the appropriate approach be to write tests for this?

You can rely on what's drawn, as it's done in test_draw/test_image.py. It's pretty painful, so if you don't want to add tests I'll write them, no problem.

@mbarkhau
Copy link
Contributor

mbarkhau commented Jun 2, 2019

The major thing I'd change is to put percentage in layout/percentage.py and not in a module on its own.

I actually started out doing that, but I ran into circular imports when changing the various modules that had their own percentage functions. I think the main culprit is from ..images import LinearGradient, RadialGradient in css/utils.py. It seems to me that the css module should have it's own representation of a gradient, rather than instantiate a class that is used to generate the actual pattern.

It's pretty painful

I created PR #876 to maybe help with that.

@mbarkhau
Copy link
Contributor

mbarkhau commented Jun 2, 2019

My test case now appears to work. In the pair of images the top are using object-fit and object-position the bottom use the equivalent background positioning.

image

The only ones that appear different are the ones to test object-position: none, but if I compare the test file to that in chrome it appears to be an issue with how the backround is positioned, so I'll leave that for now.

I think the only thing left are tests (and the percentage function issue).

@liZe
Copy link
Member

liZe commented Jun 2, 2019

I actually started out doing that, but I ran into circular imports when changing the various modules that had their own percentage functions.

OK, I'll check that.

My test case now appears to work. In the pair of images the top are using object-fit and object-position the bottom use the equivalent background positioning.

Nice! 😄

The only ones that appear different are the ones to test object-position: none, but if I compare the test file to that in chrome it appears to be an issue with how the backround is positioned, so I'll leave that for now.

No problem.

I think the only thing left are tests (and the percentage function issue).

Thank you very much. I'll merge your pull request, add tests and clean a couple of things. Thanks too for parse_pixels, it will be very useful!

@liZe liZe closed this as completed in #875 Jun 2, 2019
@liZe
Copy link
Member

liZe commented Jun 2, 2019

It works perfectly!

it appears to be an issue with how the backround is positioned, so I'll leave that for now.

Could you please provide your example? I'd like to fix this bug too 😄.

@mbarkhau
Copy link
Contributor

mbarkhau commented Jun 2, 2019

Here you go: https://codepen.io/mbarkhau/pen/NVoPBe

liZe added a commit that referenced this issue Jun 2, 2019
For now, we only avoid the repetition when we have no-repeat for x and y
axes. We could do this when the painting area is smaller than the image and
when the position doesn't require a repetition. But… Who cares?

Fix #238.

There was also a bug in the size of the surface needed to draw images that are
not repeated. Before this commit, the repeat size was the painting size * 2 to
avoid glitches at the boundaries. But we have to be sure to draw at least the
whole image, as the image may be translated before being drawn. The
multiplication is not needed as we don't repeat the image anymore.

Related to #669.
@liZe
Copy link
Member

liZe commented Jun 2, 2019

Here you go: https://codepen.io/mbarkhau/pen/NVoPBe

Thank you, it's now fixed.

@liZe liZe added this to the 48 milestone Jun 2, 2019
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 good first issue Issues that can be quite easily solved by Python developers with a good CSS background
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants