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

Fix start and end points for linear gradients with angle specified. #8094

Merged
merged 1 commit into from Oct 26, 2015

Conversation

glennw
Copy link
Member

@glennw glennw commented Oct 20, 2015

Previously, this was most noticeable with 45deg gradients, where the gradient would end too early, and the remainder was filled with a solid color.

(This also fixes gradients on webrender, which relies on the start and stop points being correct).

Review on Reviewable

Previously, this was most noticeable with 45deg gradients, where the gradient would end too early, and the remainder was filled with a solid color.

(This also fixes gradients on webrender, which relies on the start and stop points being correct).
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 20, 2015
@highfive
Copy link

warning Warning warning

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

@glennw
Copy link
Member Author

glennw commented Oct 20, 2015

r? @pcwalton

@jdm
Copy link
Member

jdm commented Oct 20, 2015

Is there any way to write a reftest for this?

@glennw
Copy link
Member Author

glennw commented Oct 20, 2015

Not that I could think of.

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 20, 2015

How about a not-ref?

@glennw
Copy link
Member Author

glennw commented Oct 22, 2015

The only ways I can think of to reftest this (with our current infrastructure) would rely on exact pixel accuracy on all platforms between two different features (such as a gradient with a css transform). I don't think we can rely on this being consistent on all platforms and renderer backends.

The best way long term would be if we could support tests that dump the display list to text and compare that to an expected reference I think.

Any thoughts @pcwalton ?

@glennw
Copy link
Member Author

glennw commented Oct 22, 2015

(And I don't know what you'd compare against in a != test that would actually make for a useful test).

@pcwalton
Copy link
Contributor

Yeah, I'm at a loss as to how to reftest this. You could try to do it with a degenerate gradient with a sharp color transition compared against a solid box with a rotation transform, but pixel accuracy issues are likely going to ruin that.

@glennw
Copy link
Member Author

glennw commented Oct 22, 2015

OK, I tried to do that, but I can't get them to match pixel for pixel. Can we land this without a test? It's a strict improvement on what's there now. Additionally, when webrender lands - it does fail reftests without this change (since it implements gradients in a different way).

@pcwalton
Copy link
Contributor

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 113778a has been approved by pcwalton

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 26, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 113778a with merge b8f196f...

@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 26, 2015
bors-servo pushed a commit that referenced this pull request Oct 26, 2015
Fix start and end points for linear gradients with angle specified.

Previously, this was most noticeable with 45deg gradients, where the gradient would end too early, and the remainder was filled with a solid color.

(This also fixes gradients on webrender, which relies on the start and stop points being correct).

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8094)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 113778a into servo:master Oct 26, 2015
@glennw glennw deleted the fix-angle-gradients branch December 12, 2016 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-needs-test S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants