Closed Bug 1366544 Opened 7 years ago Closed 7 years ago

stylo: Implement -moz-prefixed gradient functions

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: canova)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1366544 - stylo: Implement -moz-prefixed linear gradients
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1366544 - stylo: Update test expectations for -moz-prefixed linear gradients
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1366544 - stylo: Update test expectations for -moz-prefixed radial gradients
59 bytes, text/x-review-board-request
manishearth
: review+
Details 
We will need to implement -moz-prefixed gradient functions.
Can't we wait for the unshipping ticket to settle down first?
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.
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 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 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 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.
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 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+
Attachment #8879761 - Attachment is obsolete: true
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.
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
https://hg.mozilla.org/mozilla-central/rev/a65fd17cd3db
https://hg.mozilla.org/mozilla-central/rev/5b33032b3e09
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1411119
No longer depends on: 1411119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: