Skip to content

/ servo

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

Jump to bottom

Servo doesn't properly render img border-radius #17510

Closed
atouchet opened this issue Jun 24, 2017 · 8 comments
Closed

Servo doesn't properly render img border-radius #17510

atouchet opened this issue Jun 24, 2017 · 8 comments
Labels
A-content/css A-layout/inline C-has-manual-testcase

Comments

@atouchet
Copy link
Contributor

@atouchet atouchet commented Jun 24, 2017 •
edited

Example on: https://github.com/business

The face pictures are supposed to be circles but in Servo they are squares.

Image (Servo on top, Firefox on bottom):

border-radius

The offending line appears to be: <img src="https://assets-cdn.github.com/images/modules/site/avatars/hubspot-quote.jpg" alt="" style="border-radius:25px;" width="50" height="50">

Tested in Windows 10.
@jdm
Copy link
Member

@jdm jdm commented Jun 24, 2017 •
edited

All of our tests for border-radius and images appear to use background-image rather than image elements.
@streichgeorg
Copy link
Contributor

@streichgeorg streichgeorg commented Jun 28, 2017 •
edited

I looked a bit into this and found that at the moment the clipping region only gets adjusted for border radius in build_display_list_for_background_if_applicable, when I copy the code from there to build_fragment_type_specific_display_items, the the test gets rendered correctly.
@jdm
Copy link
Member

@jdm jdm commented Jun 28, 2017

Sounds like a reasonable solution to me (possibly sharing the code in a clip_for_border_radius function).
@streichgeorg
Copy link
Contributor

@streichgeorg streichgeorg commented Jun 28, 2017

Should the border-radius property also affect tags like canvasand iframe, or only img ones?
@jdm
Copy link
Member

@jdm jdm commented Jun 28, 2017

Yes, it should affect every element.
@streichgeorg
Copy link
Contributor

@streichgeorg streichgeorg commented Jun 29, 2017

I have another one last question. How should I test this PR, and where do I put the tests?
@jdm
Copy link
Member

@jdm jdm commented Jun 29, 2017

I recommend adding a reference test. You can create a new one in tests/wpt/mozilla/css/ as described in the documentation.
@streichgeorg streichgeorg mentioned this issue Jul 2, 2017
Closed
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 2, 2017
@bors-servo
Auto merge of #17589 - streichgeorg:border-radius, r=<try>
Loading status checks…
36fa2ee 
Apply border-radius to img, canvas, iframe

<!-- Please describe your changes on the following line: -->
Added some code that adjusts the clipping region of ```img```, ```canvas```, ```iframe``` elements for the ```border-radius``` property.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17510 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17589)
<!-- Reviewable:end -->
@atouchet
Copy link
Contributor Author

@atouchet atouchet commented Jul 19, 2017

This is fixed now.
@atouchet atouchet closed this Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/css A-layout/inline C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Apply border-radius to img, canvas, iframe
3 participants
@jdm @streichgeorg @atouchet
You can’t perform that action at this time.