stylo: Implement -moz-prefixed gradient functions

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: canaltinova)

Tracking

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
We will need to implement -moz-prefixed gradient functions.
Can't we wait for the unshipping ticket to settle down first?
Reporter

Comment 2

2 years ago
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?
Flags: needinfo?(nox)
I would prefer that someone else does.
Flags: needinfo?(nox)
Looks like unshipping in bug 1337655 didn't work. Nazim, can you knock this out?
Assignee: nobody → canaltinova
Priority: -- → P1
Sure, I'll work on this.
Comment hidden (mozreview-request)
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

2 years ago
mozreview-review
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?
Attachment #8879033 - Flags: review?(manishearth) → review+

Comment 10

2 years ago
mozreview-review
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
Attachment #8879032 - Flags: review?(manishearth) → review+

Comment 11

2 years ago
mozreview-review
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.
Attachment #8879032 - Flags: review?(nox)
Please make a PR and I'll review it from there.
Addressed the comments and opened a PR here: https://github.com/servo/servo/pull/17434
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

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

https://reviewboard.mozilla.org/r/151126/#review155962
Attachment #8879762 - Flags: review?(manishearth) → review+

Comment 19

2 years ago
mozreview-review
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
Attachment #8879761 - Flags: review?(manishearth) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8879761 - Attachment is obsolete: true

Comment 25

2 years ago
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)
Looks like this busted. I'll pull the patch and push.

Comment 27

2 years ago
Pushed by bholley@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a65fd17cd3db
https://hg.mozilla.org/mozilla-central/rev/5b33032b3e09
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1411119
Reporter

Updated

2 years ago
No longer depends on: 1411119
You need to log in before you can comment on or make changes to this bug.