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 linear gradient's specified form #13892

Closed
canova opened this issue Oct 22, 2016 · 7 comments
Closed

Fix linear gradient's specified form #13892

canova opened this issue Oct 22, 2016 · 7 comments

Comments

@canova
Copy link
Member

@canova canova commented Oct 22, 2016

Currently linear gradient is not preserving the current corner directions if just one corner is specified. We should fix thix. The specified form of the linear gradient should preserve the corners and it should convert to angle in ToComputedValue implementation.

To give a more details, you need to convert this enum's HorizontalDirection and VerticalDirection to Option<HorizontalDirection> and Option<HorizontalDirection> and set it here directly without converting to Angle.

This corner to angle conversion should be moved to ToComputedValue implementation, to do this you should remove this line and write the implementation in components/style/values/computed/image.rs GradientKind implemetation should give a hint to how to do it.

Also you can see this part of the Property hacking guide to see how to implement a ToComputedValue conversion.

It may require some additional changes but compile errors will give you hint about that. And of course you can ask here if you have any questions.

@canova
Copy link
Member Author

@canova canova commented Nov 7, 2016

@AVGP Are you still working on this? You can ask here if you have any questions.

@AVGP
Copy link

@AVGP AVGP commented Nov 7, 2016

Absolutely, sorry that it takes me so long, just swamped with work but
should get a chance later this week

Am 07.11.2016 10:04 vorm. schrieb "Nazım Can Altınova" <
notifications@github.com>:

@AVGP https://github.com/AVGP Are you still working on this? You can
ask here if you have any questions.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#13892 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAWmRkrZ_o5Ij5pwhk7VwJ4-kK7oHJqeks5q7um2gaJpZM4Kd2-4
.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Dec 11, 2016

@AVGP next week never came...

@KiChjang KiChjang removed the C-assigned label Dec 11, 2016
@DominoTree
Copy link
Contributor

@DominoTree DominoTree commented Dec 11, 2016

I'd like to give this one a shot

@canova
Copy link
Member Author

@canova canova commented Dec 11, 2016

@DominoTree Sure, go ahead! Btw, the links were showing wrong lines because of code changes over time. Updated them. You can ask here if something is unclear.

@canova canova added the C-assigned label Dec 11, 2016
@DominoTree
Copy link
Contributor

@DominoTree DominoTree commented Dec 14, 2016

@canaltinova I've been hacking on this some but I'm having trouble wrapping my head around it as a whole. Could you take a look at master...DominoTree:master and see if it looks like I'm moving in the right direction or if anything jumps out at you? Right now, I'm trying to figure out the parse method.

@canova
Copy link
Member Author

@canova canova commented Dec 14, 2016

@DominoTree Looks like you're in the right path! You need to also remove AngleOrCorner from here and create an AngleOrCorner enum in computed part like this one(Sorry I forgot to mention that):

pub enum AngleOrCorner {
    Angle(Angle),
    Corner(HorizontalDirection, VerticalDirection),
}

In this way we have 2 AngleOrCorner enums, one is computed and one is specified. Computed AngleOrCorner should be like the old specified one as above. You can undo your changes in components/layout/display_list_builder.rs after you changed that. We don't change the behavior of the computed AngleOrCorner. You need to convert specified::AngleOrCorner to computed::AngleOrCorner in ToComputedValue implementation.

+            //TODO: do I need to worry about this?
+            AngleOrCorner::Corner(horizontal, vertical) => {
+                specified::AngleOrCorner::Corner(horizontal, vertical)
+            }

This is mostly true, you need to change the line to specified::AngleOrCorner::Corner(Some(horizontal), Some(vertical)) because we changed the horizontal and vertical direction to Option in specified.

You can open a WIP PR if you like and we can help you more. It's a bit hard to point a line in this way :)

bors-servo added a commit that referenced this issue Dec 16, 2016
Fix linear gradient's specified form #13892

<!-- Please describe your changes on the following line: -->
WIP for #13892

---
<!-- 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 #13892  (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/14598)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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