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

Make default linear-gradient() direction 'to bottom', not 'to top'. #14748

Closed
wants to merge 1 commit into from

Conversation

@heycam
Copy link
Member

heycam commented Dec 28, 2016

Found this while looking at failing Firefox reftests with stylo. If a linear-gradient() value doesn't specify a direction, then it should go from top to bottom.

r? @shinglyu


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (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 28, 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 28, 2016

warning Warning warning

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

DominoTree commented Dec 28, 2016

Think this is bug #14745 and was fixed in #14746

@DominoTree
Copy link
Contributor

DominoTree commented Dec 28, 2016

@heycam What reftests were failing? Or was it not related to the incorrect direction?

@heycam
Copy link
Member Author

heycam commented Dec 28, 2016

@DominoTree Oh, I guess you just got in before me. :-) There are a bunch of tests under tests/wpt/css-tests/css-images-3_dev/ that fail, but because that directory of tests is skipped by default, and there is no expectation metadata, it's hard to see which ones were failing before but would now be passing. Closing in favour of your PR.

@heycam heycam closed this Dec 28, 2016
@heycam
Copy link
Member Author

heycam commented Dec 28, 2016

My mistake, there aren't any linear-gradient() tests under that directory.

@DominoTree
Copy link
Contributor

DominoTree commented Dec 28, 2016

Do you think those would be the right place to add tests for this? I haven't really delved into those yet, and I added a unit test.

@heycam
Copy link
Member Author

heycam commented Dec 28, 2016

I'm not sure what the process is for adding tests under tests/wpt/ that should be upstreamed, unfortunately. (In Firefox, we have a script that periodically upstreams changes to imported test suites like this, but I'm not sure how it works in Servo.)

@heycam
Copy link
Member Author

heycam commented Dec 28, 2016

(Also I realise earlier you probably were asking about which Firefox reftests were failing -- the one I noticed was a print preview reftest, where we have a dark grey gradient shown behind the pages to be printed. There would be other reftests that more directly test linear-gradient() that would be failing too, but the process for sorting out all the currently failing stylo reftests is going to be a long one so I didn't notice them yet.)

@DominoTree
Copy link
Contributor

DominoTree commented Dec 28, 2016

I was mainly asking because I was hoping you'd stumbled on some linear-gradient tests that I'd missed :D

I guess I'll dive into these ref tests and see if I can make heads or tails of how to roll my own, and go from there :)

@shinglyu
Copy link
Member

shinglyu commented Dec 28, 2016

@DominoTree IIRC, if your test is general enough to be upstreamed, please submit a PR to WPT test, after it merged, you can use the method described here to update the tests.

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.

None yet

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