servo / servo

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

Implement border-image-* longhands in stylo #13989

Merged

bors-servo merged 8 commits into servo:master from canova:border-image

Nov 6, 2016

Member

canova commented Oct 30, 2016 •

edited by larsbergstrom

Implementation of border-image-* longhands in stylo. PR covers just parsing/serialization part for now. I'll write gecko glue next.
I wanted to open a WIP pr to get early feedback about these parts. I made a SingleSpecifiedValue enums to handle {1, 4} values. I'm not sure about naming. I did like this because keyword_list helper is doing similar.

r? @emilio or @Manishearth

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive assigned emilio

highfive commented Oct 30, 2016

Heads up! This PR modifies the following files:

@bholley: components/style/values/specified/mod.rs, components/style/properties/longhand/border.mako.rs
@emilio: components/style/values/specified/mod.rs, components/style/properties/longhand/border.mako.rs

highfive commented Oct 30, 2016

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

highfive added the S-awaiting-review label

Manishearth approved these changes

Member

Manishearth left a comment

overall lgtm, minor nits.

Would be nice if you could make common utility functions for the {1,4} ones (seems tricky though). Might need a generic type "FourSidesComputed" for the computed value instead of individual tuple structs. We already have parse_four_sides but that's different iirc.

components/style/properties/longhand/border.mako.rs Outdated

  
      @@ -91,3 +91,749 @@ ${helpers.single_keyword("-moz-float-edge", "content-box margin-box",
    
                                 gecko_enum_prefix="StyleFloatEdge",

                                 products="gecko",

                                 animatable=False)}

        <%helpers:longhand name="border-image-source" products="none" animatable="False">

Manishearth Oct 31, 2016

Member

spec links for all, please

components/style/properties/longhand/border.mako.rs Outdated

  
                    let length = self.0.len();

                    try!(self.0[0].to_css(dest));

                    if length > 1 {

Manishearth Oct 31, 2016

Member

Just loop through self.0.iter().skip(1) emitting a space and the element each iteration.

components/style/properties/longhand/border.mako.rs Outdated

  
            pub struct SpecifiedValue(pub SingleSpecifiedValue,

                                      pub Option<SingleSpecifiedValue>);

            define_css_keyword_enum!(SingleSpecifiedValue:

Manishearth Oct 31, 2016

Member

nit: probably call it RepeatKeyword?

components/style/properties/longhand/border.mako.rs Outdated

  
                    let length = self.0.len();

                    try!(self.0[0].to_css(dest));

                    if length > 1 {

Manishearth Oct 31, 2016

Member

same comment about looping

components/style/properties/longhand/border.mako.rs Outdated

  
                }

            }

            impl SingleSpecifiedValue {

Manishearth Oct 31, 2016

Member

nit: Make this an impl of Parse since you're not using the context anyway

components/style/properties/longhand/border.mako.rs Outdated

  
                    }

                    let length = input.try(|input| LengthOrPercentage::parse(input));

                    if let Ok(len) = length {

Manishearth Oct 31, 2016

Member

nit: Collapse this into a single if let? Unless there are line size issues.

components/style/properties/longhand/border.mako.rs Outdated

  
                fn from_computed_value(computed: &computed_value::SingleComputedValue) -> Self {

                    match *computed {

                        computed_value::SingleComputedValue::LengthOrPercentage(len) =>

                            SingleSpecifiedValue::LengthOrPercentage(ToComputedValue::from_computed_value(&len)),

Manishearth Oct 31, 2016

Member

nit: wrap this somehow.

components/style/properties/longhand/border.mako.rs Outdated

  
                fn to_computed_value(&self, context: &Context) -> computed_value::SingleComputedValue {

                    match *self {

                        SingleSpecifiedValue::LengthOrPercentage(len) =>

                            computed_value::SingleComputedValue::LengthOrPercentage(len.to_computed_value(context)),

Manishearth Oct 31, 2016

Member

nit: wrap this somehow

components/style/properties/longhand/border.mako.rs Outdated

  
                    let length = self.corners.len();

                    try!(self.corners[0].to_css(dest));

                    if length > 1 {

Manishearth Oct 31, 2016

Member

ditto about looping

components/style/values/specified/mod.rs Outdated

  
            fn parse(input: &mut Parser) -> Result<Self, ()> {

                let context = AllowedNumericType::All;

                match try!(input.next()) {

                    Token::Percentage(ref value) if context.is_ok(value.unit_value) =>

Manishearth Oct 31, 2016

Member

surprised we didn't have this already

Manishearth unassigned emilio

canova force-pushed the canova:border-image branch from 0b298c5 to 03eddf3

Member Author

canova commented Nov 2, 2016

@Manishearth I pushed a commit about refactoring image code and implementing border-image-source glue. I couldn't test it with a stylo build yet. But it builds with build-geckolib.

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                }

                if let Some(image) = v.0 {

                    set_image(image, &mut self.gecko.mBorderImageSource)

Manishearth Nov 2, 2016

Member

fwiw this could be a method on nsStyleImage in gecko_conversions.rs. But not necessary, this is fine too.

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                Image::Gradient(gradient) => {

                    set_gradient(gradient, &mut geckoimage)

                },

                Image::Url(..) => {

Manishearth Nov 2, 2016

Member

There should be a third argument, with_url, that turns on this branch. (Image::Url if with_url => ..). The if shorthand == "background" above needs to set with_url.

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                                           background-image: url() not yet implemented");

                                }

                            }

                            set_image(image, &mut geckoimage.mImage)

Manishearth Nov 2, 2016

Member

Both branches of the if shorthand do the same thing now. We need to pass a boolean arg down.

canova Nov 2, 2016

Author Member

@Manishearth It looks like else branch has only one more match pattern than if branch. And I kept that match in else part. Moved the inner match pattern which looks pretty same. In if branch, Image::Url prints a warning about url and in else branch, it does nothing except gradient(with this line _ => () // we need to support image values) I thought we can give a warning in else branch too since url's not implemented in both. Should I check this still?

canova Nov 2, 2016 •

edited

Author Member

Oh, actually I have a merge conflict in these lines. The new changes forces to do what you said, changing as you said.

Manishearth Nov 2, 2016

Member

Yeah, the ongoing background-image: url work makes that if necessary

Member

Manishearth commented Nov 2, 2016

Great! Let me know when you have it tested!

canova force-pushed the canova:border-image branch from 03eddf3 to f88008c

canova reviewed

components/style/properties/gecko.mako.rs Outdated

  
      @@ -1466,6 +1296,195 @@ fn static_assert() {
    
            }

        </%def>

        fn set_image(image: Image, mut geckoimage: &mut structs::nsStyleImage, with_url: bool, cacheable: &mut bool) {

canova Nov 2, 2016

Author Member

I needed to add another reference parameter to pass cacheable variable after resolving merge conflicts.

Manishearth Nov 2, 2016

Member

Good. Ultimately we need to make the border-image-source glue match background-image, but for now set with_url to false for border-image-source and leave a todo comment

Manishearth Nov 4, 2016

Member

I would make this a method on nsStyleImage btw.

canova Nov 4, 2016

Author Member

I'm moving it. But I'm also moving set_gradient into impl nsStyleImage is it ok?

Manishearth Nov 4, 2016

Member

Yep. Make it private.

canova force-pushed the canova:border-image branch from f88008c to aa6d303

Member Author

canova commented Nov 3, 2016

Implemented their stylo glue and tested them in stylo. They're working currently. But in a special case, Firefox 51 acts different from stylo and chrome. It might be a gecko bug. I'll post a screenshot soon.

From now on, I want to move image code to gecko_conversions.rs as you mentioned.
Also maybe we can write a helper for handling 4 sided attributes?

Member Author

canova commented Nov 3, 2016 •

edited

Test case:

<!DOCTYPE html>
<html>
<head>
    <title></title>
    <style type="text/css">
        #box {
            border: 100px solid transparent;
            border-image-source: linear-gradient(red, blue);
            border-image-slice: 30 30% 45 fill;
            border-image-width: 20px 40px;
            border-image-repeat: round;
        }
    </style>
</head>
<body>
<div id="box"></div>
</body>
</html>

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                for (i, corner) in v.corners.iter().enumerate() {

                    match *corner {

                        PercentageOrNumber::Percentage(p) =>

Manishearth Nov 3, 2016

Member

You can impl ToGeckoStyleCoordConvertible or whatever that trait is in style::gecko::values for PercentageorNumber to make this cleaner.

canova Nov 4, 2016

Author Member

Oh the LengthOrNumber is actually single value of border_image_outset. It's not actually stored in style/values/.., it's stored in border_image_outset longhand. Should I implement it still?

Manishearth Nov 4, 2016

Member

Yes. You can also move it into style::values::* :)

wafflespeanut assigned Manishearth

canova force-pushed the canova:border-image branch from aa6d303 to c5b90ac

Member Author

canova commented Nov 4, 2016

@Manishearth Made the changes :)

canova changed the title ~~[WIP] Implement border-image-* longhands in stylo~~ Implement border-image-* longhands in stylo

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                use gecko_bindings::structs::{NS_STYLE_BORDER_IMAGE_REPEAT_ROUND, NS_STYLE_BORDER_IMAGE_REPEAT_SPACE};

                let mut keywords = vec![];

                    % for side in range(2):

Manishearth Nov 5, 2016

Member

Don't create a vector here, just set directly. for i,side in enumerate(["H", "V"]) will help.

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                % for side in SIDES:

                match v.${side.index} {

                    SingleComputedValue::Auto =>

                        self.gecko.mBorderImageWidth.data_at_mut(${side.index}).set_value(CoordDataValue::Auto),

Manishearth Nov 5, 2016

Member

Use braces here (and below)

Manishearth reviewed

components/style/properties/gecko.mako.rs Outdated

  
                        PercentageOrNumber::Percentage(p) =>

                            self.gecko.mBorderImageSlice.data_at_mut(i).set_value(CoordDataValue::Percent(p.0)),

                        PercentageOrNumber::Number(n) =>

                            self.gecko.mBorderImageSlice.data_at_mut(i).set_value(CoordDataValue::Factor(n)),

Manishearth Nov 5, 2016

Member

braces in this function too

Member

Manishearth commented Nov 5, 2016

Minor issues, r=me with them fixed

@bors-servo delegate+

Contributor

bors-servo commented Nov 5, 2016

✌️ @canaltinova can now approve this pull request

emilio reviewed

components/style/values/specified/mod.rs Outdated

  
        impl Parse for LengthOrNumber {

            fn parse(input: &mut Parser) -> Result<Self, ()> {

                let length = input.try(|input| Length::parse(input));

emilio Nov 5, 2016

Member

nit: This can be written input.try(Length::parse)

canova force-pushed the canova:border-image branch from c5b90ac to 570aa24

Contributor

bors-servo commented Nov 6, 2016

☔ The latest upstream changes (presumably #14077) made this pull request unmergeable. Please resolve the merge conflicts.

highfive added the S-needs-rebase label

Member

Manishearth commented Nov 6, 2016

The other PR merged, you probably can rebase and land this now.

canova added 8 commits


        Implement parsing/serialization for border-image-* longhands

da27b61


        Refactor image code and implement gecko glue for border-image-source

132b368


        Implement gecko glue for border-image-outset

dc55e80


        Implement gecko glue for border-image-repeat

cbd43f7


        Implement gecko glue for border-image-width

8387d7e


        Implement gecko glue for border-image-slice

6014c0c


        Move set_image and set_gradient into nsStyleImage impl

9a2ef2e


        Move LengthOrNumber to style/values and implement GeckoStyleCoordConv…

7720fe4

…ertible

canova force-pushed the canova:border-image branch from 570aa24 to 7720fe4

Member Author

canova commented Nov 6, 2016

@bors-servo r=Manishearth
Rebased thanks.

Contributor

bors-servo commented Nov 6, 2016

📌 Commit 7720fe4 has been approved by Manishearth

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels

Contributor

bors-servo commented Nov 6, 2016

⌛ Testing commit 7720fe4 with merge 60e09ad...

bors-servo added a commit that referenced this pull request


        Auto merge of #13989 - canaltinova:border-image, r=Manishearth

60e09ad

Implement border-image-* longhands in stylo

Implementation of border-image-\* longhands in stylo. PR covers just parsing/serialization part for now. I'll write gecko glue next.
I wanted to open a WIP pr to get early feedback about these parts. I made a `SingleSpecifiedValue` enums to handle {1, 4} values. I'm not sure about naming. I did like this because `keyword_list` helper is doing similar.

r? @emilio or @Manishearth
---

<!-- 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

<!-- Either: -->
- [ ] 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/13989)

<!-- Reviewable:end -->

Contributor

bors-servo commented Nov 6, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

bors-servo merged commit 7720fe4 into servo:master

2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed

Details

continuous-integration/travis-ci/pr The Travis CI build passed

Details

homu Test successful

Details

highfive removed the S-awaiting-merge label

This was referenced Nov 6, 2016

Make use of Servo-specific ToCss everywhere! #14089

Merged

support cursor property url() values in stylo #14055

Merged

canova deleted the canova:border-image branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.