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

Default is top-to-bottom if unset, not bottom-to-top #14746

Merged
merged 1 commit into from Dec 28, 2016

Conversation

@DominoTree
Copy link
Contributor

DominoTree commented Dec 27, 2016

Reverse linear gradient direction if not explicitly specified to match expected default behavior


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14745 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Dec 27, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/computed/image.rs
  • @emilio: components/style/values/computed/image.rs
@highfive
Copy link

highfive commented Dec 27, 2016

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@cbrewster
Copy link
Member

cbrewster commented Dec 27, 2016

Could we add a ref test for this?

@DominoTree
Copy link
Contributor Author

DominoTree commented Dec 27, 2016

Screenshot

@DominoTree
Copy link
Contributor Author

DominoTree commented Dec 28, 2016

I added a unit test, it required a bit of boilerplate, not sure if we should put it into a macro or something. Need to figure out the ref tests.

@cbrewster
Copy link
Member

cbrewster commented Dec 28, 2016

@jdm
Copy link
Member

jdm commented Dec 28, 2016

@highfive highfive assigned SimonSapin and unassigned jdm Dec 28, 2016
@@ -32,6 +38,12 @@ fn test_linear_gradient() {

// Parsing without <angle> and <side-or-corner>
assert_roundtrip_with_context!(Image::parse, "linear-gradient(red, green)");

// AngleOrCorner::None should become AngleOrCorner::Angle(Angle(PI)) when parsed

This comment has been minimized.

@shinglyu

shinglyu Dec 28, 2016

Member

Maybe add a link to the spec in the comment. Or at least make clear that PI means "top-to-bottom". :)

@shinglyu
Copy link
Member

shinglyu commented Dec 28, 2016

For the reftest, you can create two htmls, the test html could be a gradient box with the default, the reference html can be a gradient box that was explicitly set as to bottom. The two should render exactly the same.

To begin with, you can put the test in tests/wpt/mozilla/tests/css, these are servo-specific tests that are hard or impossible to upstream. But later you might want to upstream that to wpt and pull the new wpt test into servo.

@KiChjang
Copy link
Member

KiChjang commented Dec 28, 2016

Pulling from upstream WPT should really only be done by @Ms2ger or @jgraham. Please wait for them to update WPT when you have submitted and merged tests upstream.

@DominoTree
Copy link
Contributor Author

DominoTree commented Dec 28, 2016

Added reftests to existing linear-gradient parsing tests in tests/wpt/mozilla/tests/css

@emilio
Copy link
Member

emilio commented Dec 28, 2016

Looks great to me, thanks.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 28, 2016

📌 Commit 23d4f04 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Dec 28, 2016

Testing commit 23d4f04 with merge 3911fbe...

bors-servo added a commit that referenced this pull request Dec 28, 2016
Default is top-to-bottom if unset, not bottom-to-top

<!-- Please describe your changes on the following line: -->
Reverse linear gradient direction if not explicitly specified to match expected default behavior

---
<!-- 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 #14745 (github issue number if applicable).

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

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

bors-servo commented Dec 28, 2016

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Dec 28, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 28, 2016

Testing commit 23d4f04 with merge adb3e12...

bors-servo added a commit that referenced this pull request Dec 28, 2016
Default is top-to-bottom if unset, not bottom-to-top

<!-- Please describe your changes on the following line: -->
Reverse linear gradient direction if not explicitly specified to match expected default behavior

---
<!-- 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 #14745 (github issue number if applicable).

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

<!-- 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/14746)
<!-- Reviewable:end -->
@bors-servo bors-servo merged commit 23d4f04 into servo:master Dec 28, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@DominoTree DominoTree deleted the DominoTree:fix-linear-gradient branch Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.