1366544 - stylo: Implement -moz-prefixed gradient functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

We will need to implement -moz-prefixed gradient functions.

Comment 1

[Anthony Ramine [:nox]]

Can't we wait for the unshipping ticket to settle down first?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Now we've entered our soft code freeze for Firefox 55, and bug 1182775 hasn't get resolved (as far as I can tell, Gmail is still using broken syntax there).

I suggest we would definitely need this for stylo now, because we cannot switch it off in Firefox 55 (the "soft code freeze"), however Firefox 55 is the last release before Firefox 57 becomes Beta, which means we probably cannot remove -moz-prefixed gradient support before 57 (otherwise we cannot re-enable it when we hit regression in release).

nox, given this, could you take this bug?

Comment 3

[Anthony Ramine [:nox]]

I would prefer that someone else does.

Comment 4

[Bobby Holley (:bholley)]

Looks like unshipping in bug 1337655 didn't work. Nazim, can you knock this out?

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Sure, I'll work on this.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1366544 - stylo: Implement -moz-prefixed linear gradients

Review commit: https://reviewboard.mozilla.org/r/150342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150342/

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients

Review commit: https://reviewboard.mozilla.org/r/150344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150344/

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

The `position` on -moz-linear gradient is a little different from normal positions. Normal position accepts values like `right 20px bottom` but prefixed gradient's position doesn't accept that. If I remember correctly, this was a legacy position syntax. I could create another struct instead of manupilating the current position but that could cause code duplication. So I decided to go with current position struct and add another parsing method for it.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8879033 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients

https://reviewboard.mozilla.org/r/150344/#review155198

What kind of failures do we have left?

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8879032 [details]
Bug 1366544 - stylo: Implement -moz-prefixed linear gradients

https://reviewboard.mozilla.org/r/150342/#review155200

::: servo/components/style/values/specified/position.rs:199
(Diff revision 1)
>  impl<S: Parse> PositionComponent<S> {
>      /// Parses a component of a CSS position, with quirks.
>      pub fn parse_quirky<'i, 't>(context: &ParserContext,
>                                  input: &mut Parser<'i, 't>,
> -                                allow_quirks: AllowQuirks)
> +                                allow_quirks: AllowQuirks,
> +                                legacy_syntax: bool)

documentation for this param

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8879032 [details]
Bug 1366544 - stylo: Implement -moz-prefixed linear gradients

https://reviewboard.mozilla.org/r/150342/#review155220

I really really want nox to look at this. He took a fair amount of time to cleanup the gradient code.

My comments are mostly nit-picking around. Thanks for fixing this! :)

::: servo/components/style/values/specified/image.rs:74
(Diff revision 1)
>      Corner(X, Y),
> +    /// A Position and an Angle for legacy `-moz-` prefixed gradient.
> +    /// `-moz-` prefixed linear gradient can contain both a position and an angle but it
> +    /// uses legacy syntax for position. That means we can't specify both keyword and
> +    /// length for each horizontal/vertical components.
> +    PositionAngle(Option<Position>, Option<Angle>),

A position doesn't sound a lot like a line direction... Perhaps this should be renamed?

::: servo/components/style/values/specified/image.rs:578
(Diff revision 1)
>  }
>  
>  impl LineDirection {
>      fn parse<'i, 't>(context: &ParserContext,
>                       input: &mut Parser<'i, 't>,
> -                     compat_mode: CompatMode)
> +                     compat_mode: &mut CompatMode)

Ick :(. I guess it's fine, but I don't like in-out params, please document when it can change.

::: servo/components/style/values/specified/image.rs:591
(Diff revision 1)
> -        }
> +            }
> +            None
> +        };
> +
>          input.try(|i| {
> -            if compat_mode == CompatMode::Modern {
> +            let to_ident = i.try(|i| i.expect_ident_matching("to"));

Seems like we shouldn't even try to tokenize the `to` keyword if this is a `WebKit` gradient. I find this relatively hard to read.

::: servo/components/style/values/specified/position.rs:63
(Diff revision 1)
>  impl Position {
>      /// Parses a `<position>`, with quirks.
>      pub fn parse_quirky<'i, 't>(context: &ParserContext,
>                                  input: &mut Parser<'i, 't>,
> -                                allow_quirks: AllowQuirks)
> +                                allow_quirks: AllowQuirks,
> +                                legacy_syntax: bool)

Yeah, this definitely needs more documentation, and please make it a binary enum like `AllowQuirks`.

::: servo/components/style/values/specified/position.rs:84
(Diff revision 1)
>                      let x_pos = PositionComponent::Side(x_keyword, lop);
>                      let y_pos = PositionComponent::Center;
>                      return Ok(Self::new(x_pos, y_pos));
>                  }
>                  if let Ok(y_keyword) = input.try(Y::parse) {
> -                    let y_lop = input.try(|i| LengthOrPercentage::parse_quirky(context, i, allow_quirks)).ok();
> +                    let y_lop = if !legacy_syntax {

That way you can match here, and reduce the weird indentation.

::: servo/components/style/values/specified/position.rs:142
(Diff revision 1)
>          Ok(Self::new(x_pos, y_pos))
>      }
>  
> +    /// Parses legacy Position syntax.
> +    pub fn parse_legacy<'i, 't>(context: &ParserContext,
> +                                input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {

indent this to be consistent with the rest of the code.

Comment 12

[Anthony Ramine [:nox]]

Please make a PR and I'll review it from there.

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Addressed the comments and opened a PR here: https://github.com/servo/servo/pull/17434

Comment 14

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1366544 - stylo: Implement -moz-prefixed radial gradients

Review commit: https://reviewboard.mozilla.org/r/151124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151124/

Comment 15

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients

Review commit: https://reviewboard.mozilla.org/r/151126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151126/

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879032 [details]
Bug 1366544 - stylo: Implement -moz-prefixed linear gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150342/diff/1-2/

Comment 17

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879033 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150344/diff/1-2/

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8879762 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients

https://reviewboard.mozilla.org/r/151126/#review155962

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8879761 [details]
Bug 1366544 - stylo: Implement -moz-prefixed radial gradients

https://reviewboard.mozilla.org/r/151124/#review156028

::: servo/components/style/values/specified/image.rs:521
(Diff revision 1)
> -                }
> +                    }
> -                EndingShape::parse(context, i, compat_mode)
> +                    EndingShape::parse(context, i, *compat_mode)
> -            });
> +                });
> -            (shape, position)
> +                (shape, position, None)
> +            },
> +            CompatMode::Moz => {

nit: should probably write the syntax in a comment

::: servo/components/style/values/specified/image.rs:707
(Diff revision 1)
>                      return Ok(GenericEndingShape::Ellipse(Ellipse::Radii(x, y)));
>                  }
>              }
>              return Ok(GenericEndingShape::Ellipse(Ellipse::Extent(ShapeExtent::FarthestCorner)));
>          }
> +        if compat_mode != CompatMode::Moz {

nit: needs a comment

Comment 20

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879761 [details]
Bug 1366544 - stylo: Implement -moz-prefixed radial gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151124/diff/1-2/

Comment 21

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879762 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151126/diff/1-2/

Comment 22

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879032 [details]
Bug 1366544 - stylo: Implement -moz-prefixed linear gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150342/diff/2-3/

Comment 23

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879033 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150344/diff/2-3/

Comment 24

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8879762 [details]
Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151126/diff/2-3/

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 93881b30f420 -d b26128086f5b: rebasing 404957:93881b30f420 "Bug 1366544 - stylo: Implement -moz-prefixed linear gradients r=manishearth"
merging layout/style/ServoBindings.cpp
merging layout/style/ServoBindings.h
rebasing 404958:55b6144c7711 "Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients r=manishearth"
merging layout/style/test/stylo-failures.md
rebasing 404959:8ab24199c0e0 "Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients r=manishearth" (tip)
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Bobby Holley (:bholley)]

Looks like this busted. I'll pull the patch and push.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a65fd17cd3db
stylo: Implement -moz-prefixed linear gradients. r=Manishearth
https://hg.mozilla.org/integration/autoland/rev/5b33032b3e09
Update expectations. r=me

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a65fd17cd3db
https://hg.mozilla.org/mozilla-central/rev/5b33032b3e09