Skip to content

Conversation

demo99
Copy link
Contributor

@demo99 demo99 commented Jan 12, 2018

If we don't skip the BorderStyle::None edges, the shader will render the edges as solid borders. Gecko bug is bug 1419065.
I add a wrench test. The fuzz is due to the border's corners. I also update an original wrench image because it was wrong.
I just pushed a gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e49be4dd74ae96aa2d9fac7d032e841804fb2de

r? @glennw


This change is Reviewable

@demo99
Copy link
Contributor Author

demo99 commented Jan 12, 2018

There are some unexpected failures. I think those are because of the corner. I'll see how to fix them.

@demo99
Copy link
Contributor Author

demo99 commented Jan 15, 2018

There are some problems here. One is that we should skip the borderstyle::none. Another one is that the border corner's style should be single style if the other edge is none or hidden. The last one is the double border's width should be at least one pixel or we may render nothing in some cases.

The gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e414007f54f45b94ec820d7225b4e72270c02a&selectedJob=156302140

There are 4 unexpected-pass and 3 unexpected-fail tests. I think the unexpected-fail tests might be AA problem. It was covered by the borderstyle::none problem since originally we render a solid border on the borderstyle::none edge unconditionally (all collapsed tables have this problem). I want to use fuzz for the 3 unexpected-fail tests. @staktrace, can you check the try link to see if it's okay for you?
r? @glennw

@staktrace
Copy link
Contributor

@demo99 I'd rather use fails-if for the 3 new unexpected fails, unless you're sure this is something we're not going to fix. We have a few other tests that are also marked fails-if due to slightly different coloring on table borders and that I suspect that they all have the same root cause.

Regardless I have no objection to merging this given that it fixes 4 tests and fails 3 in what appears to be trivial differences.

@glennw
Copy link
Member

glennw commented Jan 16, 2018

Looks good, thanks.

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit f1b802f has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit f1b802f with merge be99474...

bors-servo pushed a commit that referenced this pull request Jan 16, 2018
Skip the edge if the border style is none.

If we don't skip the BorderStyle::None edges, the shader will render the edges as solid borders. Gecko bug is [bug 1419065](https://bugzilla.mozilla.org/show_bug.cgi?id=1419065).
I add a wrench test. The fuzz is due to the border's corners. I also update an original wrench image because it was wrong.
I just pushed a gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e49be4dd74ae96aa2d9fac7d032e841804fb2de

r? @glennw

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

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing be99474 to master...

@bors-servo bors-servo merged commit f1b802f into servo:master Jan 16, 2018
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.

4 participants