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

Improve border images #21608

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Improve border images #21608

merged 2 commits into from
Sep 28, 2018

Conversation

pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Sep 4, 2018

Respect CSS border-image-width.
Properly support gradients as a border-image-source.

Add a new test and mark two more as passing.


This change is Reviewable

@highfive
Copy link

highfive commented Sep 4, 2018

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list/builder.rs, components/layout/display_list/background.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 4, 2018
@highfive
Copy link

highfive commented Sep 4, 2018

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 4, 2018

@bors-servo try=wpt

bors-servo pushed a commit that referenced this pull request Sep 4, 2018
WIP Improve border gradients and images

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] 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/21608)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Trying commit fc17b38 with merge 777e001...

@bors-servo
Copy link
Contributor

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@jdm
Copy link
Member

jdm commented Sep 10, 2018

@pyfisch Is this still WIP or is it waiting on review?

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 11, 2018 via email

@pyfisch pyfisch changed the title WIP Improve border gradients and images Improve border images Sep 11, 2018
@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#83.

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 11, 2018

@bors-servo try=wpt

@jdm this is ready for review. Added a test for the first two points. The third is covered already by border-image-outset-003.

Note that I just removed the check border-width == 0 and early return. This could be considered to be a performance regression but I did not want to bother right now with writing a correct early return statement.

@jdm
Copy link
Member

jdm commented Sep 11, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit 9a18f33 with merge 3484a7e...

bors-servo pushed a commit that referenced this pull request Sep 11, 2018
Improve border images

Respect CSS border-image-width.
Properly support gradients as a border-image-source.

Add a new test and mark two more as passing.

<!-- 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/21608)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 11, 2018
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 12, 2018
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#83.

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 12, 2018

Update. Mark two paint worklet tests as passing.

#17860 was referenced by the tests. Property border-image-outset is now implemented but css-paint-api/geometry-border-image-004.html still fails.

I deemed {"status": "CRASH", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/syntax/parsing/html5lib_tests9.html?run_type=uri", "line": 98391, "action": "test_result", "expected": "OK"}to be intermittent.

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 12, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit ab34be1 with merge 22e1679...

bors-servo pushed a commit that referenced this pull request Sep 12, 2018
Improve border images

Respect CSS border-image-width.
Properly support gradients as a border-image-source.

Add a new test and mark two more as passing.

<!-- 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/21608)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 12, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 27, 2018

Error syncing changes upstream. Logs saved in error-snapshot-1538082026475.

@jdm This may interest you.

@jdm
Copy link
Member

jdm commented Sep 27, 2018

Yep. Could you do another force push with no meaningful changes? I want to see if it will open a PR in the proper place now.

@ghost
Copy link

ghost commented Sep 28, 2018

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1538117948500.

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 28, 2018

Force pushed.

@jdm
Copy link
Member

jdm commented Sep 28, 2018

Ok, force pushing again should work.

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks great with or without the nit addressed.

let mut height = border_image_area.height.to_px() as u32;
let source = match image {
Image::Url(ref image_url) => {
let image = image_url.url().and_then(|url| {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Given you're doing ? at the end you may as well:

let url = image_url.url()?;
let image = state.layout_context.get_webrender_image_for_url(
    self.node,
    url.clone(),
    UsePlaceholder::No,
)?;

// ...

@emilio
Copy link
Member

emilio commented Sep 28, 2018

@bors-servo delegate+

@bors-servo
Copy link
Contributor

✌️ @pyfisch can now approve this pull request

Additionally if an image border can't be displayed show solid border.
Introduce build_display_list_for_border_image to display border images.
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1538166914642.

@pyfisch
Copy link
Contributor Author

pyfisch commented Sep 28, 2018

@bors-servo r=emilio

@jdm servo-wpt-sync failed again...

@bors-servo
Copy link
Contributor

📌 Commit 60d0c8c has been approved by emilio

@highfive highfive assigned emilio and unassigned ferjm Sep 28, 2018
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 28, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 60d0c8c with merge 97e3c5f...

bors-servo pushed a commit that referenced this pull request Sep 28, 2018
Improve border images

Respect CSS border-image-width.
Properly support gradients as a border-image-source.

Add a new test and mark two more as passing.

<!-- 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/21608)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit 60d0c8c into servo:master Sep 28, 2018
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 28, 2018
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1538170094369.

@jdm
Copy link
Member

jdm commented Sep 28, 2018

Ok. I know what went wrong, and I'll perform a manual sync.

jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2018
Respect CSS border-image-width.
Properly support gradients as a border-image-source.
Only emit a border item if the border-width is non-zero
for simple borders but still emit one if the item is
an image as paint worklet that are not drawn cause servo
to hang and fail tests.

Add a new test and mark 4 more as passing.

Upstreamed from servo/servo#21608 [ci skip]
jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2018
Additionally if an image border can't be displayed show solid border.
Introduce build_display_list_for_border_image to display border images.

Upstreamed from servo/servo#21608 [ci skip]
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.

None yet

7 participants