1397122 - stylo: causes extreme redrawing issues on Quizlet Learn

Status: RESOLVED FIXED

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

Description

[Lee Bousfield]

Attachment: screenshot of the problem

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170905220108

Steps to reproduce:

Enable stylo with layout.css.servo.enabled;true

Go to a Quizlet list, then click Learn. No account required. E.g.: https://quizlet.com/106774752/learn

Complete some questions, or just hit some numbers 1-4 on your keyboard.


Actual results:

Previous questions get stacked on top of another, until something causes a redraw such as text being selected, keys being pressed, or just time passing. In addition, when the problem resolves itself, it is often a gradual resolution. It's possible the gradual resolution is Quizlet's doing though (it may clean up old elements on a timer).

The problem seems to also affect focus, but that may be unrelated.

I'm not sure if redraw is the right word here, because when you switch to another tab/window or move your mouse over the affected text, the problem remains. Maybe "layout caching"?


Expected results:

As seen on the same version of Firefox without stylo enabled, questions should not stack on top of each other.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Seems like some transitions aren't properly triggering? I confirmed the page is still broken with DISABLE_STYLE_SHARING_CACHE=1, fwiw.

Relevant rules:

.AssistantFeedbackViewTransitionLayout-transition-enter {
    opacity: 0;
    -webkit-transform: translateY(13%);
    -moz-transform: translateY(13%);
    -ms-transform: translateY(13%);
    transform: translateY(13%)
}

.AssistantFeedbackViewTransitionLayout-transition-enter-active {
    -webkit-transition: none 250ms ease-in-out;
    -moz-transition: none 250ms ease-in-out;
    -ms-transition: none 250ms ease-in-out;
    transition: none 250ms ease-in-out;
    -webkit-transition-property: all;
    -moz-transition-property: all;
    -ms-transition-property: all;
    transition-property: all;
    opacity: 1;
    -webkit-transform: none;
    -moz-transform: none;
    -ms-transform: none;
    transform: none;
    -webkit-transition-delay: 250ms;
    -moz-transition-delay: 250ms;
    -ms-transition-delay: 250ms;
    transition-delay: 250ms
}

.AssistantFeedbackViewTransitionLayout-transition-leave {
    opacity: 1;
    -webkit-transform: none;
    -moz-transform: none;
    -ms-transform: none;
    transform: none
}

.AssistantFeedbackViewTransitionLayout-transition-leave-active {
    -webkit-transition: none 250ms ease-in-out;
    -moz-transition: none 250ms ease-in-out;
    -ms-transition: none 250ms ease-in-out;
    transition: none 250ms ease-in-out;
    -webkit-transition-property: all;
    -moz-transition-property: all;
    -ms-transition-property: all;
    transition-property: all;
    opacity: 0;
    -webkit-transform: translateX(-100%);
    -moz-transform: translateX(-100%);
    -ms-transform: translateX(-100%);
    transform: translateX(-100%)
}

.AssistantFixedActionLayout {
    display: -webkit-box;
    display: -moz-box;
    display: -webkit-flex;
    display: -ms-flexbox;
    display: box;
    display: flex;
    -webkit-box-orient: vertical;
    -moz-box-orient: vertical;
    -webkit-flex-direction: column;
    -ms-flex-direction: column;
    flex-direction: column;
    bottom: 0;
    left: 0;
    position: absolute;
    right: 0;
    top: 0
}

I can take a look tomorrow morning, but Hiro, could you take a look in case this rings any bells?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Yeah, transition seems to be broken there. I take this.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Thanks!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Simplified test case

The problem is that servo drops transition duration value if it's specified in 'transition' shorthand and its transition-property is none.  I will check the transition spec whether it is a right thing to do or not.

Comment 5

[Lee Bousfield]

I believe the transition-duration should be kept, since otherwise there would be no point to explicitly allowing "none" as the property in the shorthand.

Transition shorthand spec: http://www.w3.org/TR/css3-transitions/#transition-shorthand-property

Additionally, this behaviour is similar to the background shorthand dealing with multiple images, which is specified here: https://www.w3.org/TR/css3-background/#the-background

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thanks. Yeah, I think so.  I am now fixing it.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d4c856e31f4f7f9fa556fb5a7ccc2a7b75b69c7

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I should have run all tests.  (Though I did specify invalid test name in the previous try.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ffb76ec944af86871b6dbbc90cef3cc81cddcd4

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Oops, I did miss resetting cssparser::Parser state and fixing serialization.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ccb22a8bb1acf9851a0cf6c3c65ac52a2d260a

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1397122 - Simplify transition-property parser.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

A single transition value that includes 'none' for transition-property is
valid, so we should store whole specified values (if it's valid).  Whereas,
the spec [1] clearly says for the case where there are multiple entries;
  If there is more than one <single-transition> in the shorthand, and any of
  the transitions has none as the <single-transition-property>, then the
  declaration is invalid.

'none 2s linear 2s' is a test case for the former, '1s width, 2s none' is a
test case for the latter.

Note that with this patch, 'transition: none' is serialized as 'none 0s ease 0s'
instead of 'none' but it matches Gecko.

[1] https://drafts.csswg.org/css-transitions/#single-transition

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

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8905788 [details]
Bug 1397122 - Simplify transition-property parser.

https://reviewboard.mozilla.org/r/177594/#review182634

LGTM

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8905789 [details]
Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

https://reviewboard.mozilla.org/r/177596/#review182650

::: servo/components/style/properties/shorthand/box.mako.rs:125
(Diff revision 1)
>                  // 'transition-property' accepts any keyword.
> -                try_parse_one!(input, property, transition_property);
> +                if !has_none && property.is_none() {
> +                    // The parser for transition-property fails even if 'none' is specified.
> +                    // So, we do call the parser directly instead of using Parser::try().
> +                    let start = input.state();
> +                    match transition_property::SingleSpecifiedValue::parse(input) {

Probably using `input.try` is better than manipulate input state manually?

I would expect something like
```rust
if let Ok(value) = input.try(|i| Value::parse(i)) {
    // ...
} else if input.try(|i| i.expect_ident_matching("none")).is_ok() {
    // ...
}
```

::: servo/components/style/properties/shorthand/box.mako.rs:133
(Diff revision 1)
> +                            continue;
> +                        },
> +                        Err(CssParseError::Custom(err)) => {
> +                            // 'none' is a valid case.
> +                            if err == SelectorParseError::UnexpectedIdent("none".into()) {
> +                                has_none = true;

Would it be better to have `property` variable here having type `Option<Option<Value>>` so that we don't need a separate `has_none` to track it?

We would then assign `Some(Some(value))` for successfully parsed `transition-property` value, and `Some(None)` for `none` keyword, and have `property.unwrap_or_else(|| Some(...))` at the end.

Not sure whether it is better, though. I have a feeling that using a separate variable could be more error-prone, but that's probably not the case.

::: servo/components/style/properties/shorthand/box.mako.rs:170
(Diff revision 1)
> -                % for prop in "property duration timing_function delay".split():
> -                ${prop}s.push(result.transition_${prop});
> -                % endfor
> +            match result.transition_property {
> +                Some(transition_property) => propertys.push(transition_property),
> +                None => {
> +                    // If there is more than one item, and any of transitions has 'none',
> +                    // then it's invalid.
> +                    if multiple_items {
> +                        return Err(StyleParseError::UnspecifiedError.into());
> -            }
> +                    }
> -        } else {
> -            // `transition: none` is a valid syntax, and we keep transition_property empty because |none| is not
> -            // a valid TransitionProperty.
> +                    // Empty vector means transition-property is 'none'.
> +                },
> +            }

Probably a little more concise with `if let`:
```rust
if let Some(value) = result.transition_property {
    propertys.push(value);
} else if results.len() > 1 {
    // If there is ... Otherwise empty vector means transition-property is 'none'.
    return Err(...);
}
```

::: servo/components/style/properties/shorthand/box.mako.rs:205
(Diff revision 1)
> +            let mut len = 0;
> +            % for name in "duration delay timing_function".split():
> +                len = max(len, self.transition_${name}.0.len());
> +            % endfor
> +
> +            if len == 0 && property_len == 0 {

Is this possible at all? Transition properties other than `transition-property` should always at least have one item, shouldn't they?

::: servo/components/style/properties/shorthand/box.mako.rs:208
(Diff revision 1)
>              // If any value list length is differs then we don't do a shorthand serialization
>              // either.
> -            % for name in "property duration delay timing_function".split():
> -                if len != self.transition_${name}.0.len() {
> +            // Note that 0-length transition_property means 'transition-property: none', also
> +            // in such cases, we don't have more than one longhand property.

I guess this comment should be updated as a whole, otherwise the first paragraph seems to conflict with the second.

Probably something like
> There are two cases that we can do shorthand serialization:
> * when all value lists have the same length, or
> * when transition-property is none, and other value lists have exactly one item.

::: servo/components/style/properties/shorthand/box.mako.rs:212
(Diff revision 1)
> -
>              // If any value list length is differs then we don't do a shorthand serialization
>              // either.
> -            % for name in "property duration delay timing_function".split():
> -                if len != self.transition_${name}.0.len() {
> +            // Note that 0-length transition_property means 'transition-property: none', also
> +            // in such cases, we don't have more than one longhand property.
> +            if (property_len != 0 && property_len != len) ||

So this check doesn't actually match the comment above. It only checks that the length of `transition-property` and the maximum length among all other transition properties match.

It means that with something like
```css
transition-property: width, height;
transition-duration: 1s, 2s;
transition-delay: 0s;
transition-timing-function: linear;
```
you would assert with "index out of bounds" in the loop below.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8905789 [details]
Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

https://reviewboard.mozilla.org/r/177596/#review182650

> So this check doesn't actually match the comment above. It only checks that the length of `transition-property` and the maximum length among all other transition properties match.
> 
> It means that with something like
> ```css
> transition-property: width, height;
> transition-duration: 1s, 2s;
> transition-delay: 0s;
> transition-timing-function: linear;
> ```
> you would assert with "index out of bounds" in the loop below.

Maybe you can do something like
```rust
if property_len == 0 {
    % for name in "duration delay timing_function".split():
        if self.transition_${name}.0.len() != 1 {
            return Ok(());
        }
    % endfor
} else {
    % for name in "duration delay timing_function".split():
        if self.transition_${name}.0.len() != property_len {
            return Ok(());
        }
    % endfor
}
```

Then you have a single variable to rely on.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> Comment on attachment 8905789 [details]
> Bug 1397122 - Handle values as valid in single transition shorthand even if
> the transition-property is 'none'.
> 
> https://reviewboard.mozilla.org/r/177596/#review182650
> 
> ::: servo/components/style/properties/shorthand/box.mako.rs:125
> (Diff revision 1)
> >                  // 'transition-property' accepts any keyword.
> > -                try_parse_one!(input, property, transition_property);
> > +                if !has_none && property.is_none() {
> > +                    // The parser for transition-property fails even if 'none' is specified.
> > +                    // So, we do call the parser directly instead of using Parser::try().
> > +                    let start = input.state();
> > +                    match transition_property::SingleSpecifiedValue::parse(input) {
> 
> Probably using `input.try` is better than manipulate input state manually?
> 
> I would expect something like
> ```rust
> if let Ok(value) = input.try(|i| Value::parse(i)) {
>     // ...
> } else if input.try(|i| i.expect_ident_matching("none")).is_ok() {
>     // ...
> }
> ```

Oh nice. I did miss the Parser rests the state in error cases. 

> ::: servo/components/style/properties/shorthand/box.mako.rs:133
> (Diff revision 1)
> > +                            continue;
> > +                        },
> > +                        Err(CssParseError::Custom(err)) => {
> > +                            // 'none' is a valid case.
> > +                            if err == SelectorParseError::UnexpectedIdent("none".into()) {
> > +                                has_none = true;
> 
> Would it be better to have `property` variable here having type
> `Option<Option<Value>>` so that we don't need a separate `has_none` to track
> it?
> 
> We would then assign `Some(Some(value))` for successfully parsed
> `transition-property` value, and `Some(None)` for `none` keyword, and have
> `property.unwrap_or_else(|| Some(...))` at the end.
> 
> Not sure whether it is better, though. I have a feeling that using a
> separate variable could be more error-prone, but that's probably not the
> case.

Though I am not a big fan of nested Option<>, I will do it. Thanks!

> ::: servo/components/style/properties/shorthand/box.mako.rs:205
> (Diff revision 1)
> > +            let mut len = 0;
> > +            % for name in "duration delay timing_function".split():
> > +                len = max(len, self.transition_${name}.0.len());
> > +            % endfor
> > +
> > +            if len == 0 && property_len == 0 {
> 
> Is this possible at all? Transition properties other than
> `transition-property` should always at least have one item, shouldn't they?

Oh right. I thought just setting transition-property is the case, it isn't the case actually.

> ::: servo/components/style/properties/shorthand/box.mako.rs:208
> (Diff revision 1)
> >              // If any value list length is differs then we don't do a shorthand serialization
> >              // either.
> > -            % for name in "property duration delay timing_function".split():
> > -                if len != self.transition_${name}.0.len() {
> > +            // Note that 0-length transition_property means 'transition-property: none', also
> > +            // in such cases, we don't have more than one longhand property.
> 
> I guess this comment should be updated as a whole, otherwise the first
> paragraph seems to conflict with the second.
> 
> Probably something like
> > There are two cases that we can do shorthand serialization:
> > * when all value lists have the same length, or
> > * when transition-property is none, and other value lists have exactly one item.

Thanks for the nice sentence. I will use this.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8905789 [details]
Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8905789 [details]
Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

https://reviewboard.mozilla.org/r/177596/#review182716

Looks better now.

If your old patch doesn't break any test, probably consider adding a testcase with serializing a rule like
```css
div {
  transition: width, height;
  transition-delay: 0s;
}
```
and check that it doesn't end up serializing a shorthand or crash for out-of-bound access.

::: servo/components/style/properties/shorthand/box.mako.rs:123
(Diff revision 2)
> -                try_parse_one!(input, property, transition_property);
> +                if property.is_none() {
> +                    if let Ok(value) = input.try(|i| transition_property::SingleSpecifiedValue::parse(i)) {
> +                        property = Some(Some(value));
> +                        continue;
> +                    } else if input.try(|i| i.expect_ident_matching("none")).is_ok() {
> +                        // The parser for transition-property fails even if 'none' is specified.

This comment sounds a little weird. Probably something like
> 'none' is not a valid value for <single-transition-property>, so it's not accepted in the function above.

::: servo/components/style/properties/shorthand/box.mako.rs:157
(Diff revision 2)
> +                // If there is more than one item, and any of transitions has 'none',
> +                // then it's invalid.

Probably still worth mentioning
> Otherwise, leave propertys to be empty (which means "transition-property: none").

either here or after the block.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Thanks for the review, Xidorn!

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe48833b4b71d8930903922867ecdb16420395f

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8905789 [details]
Bug 1397122 - Handle values as valid in single transition shorthand even if the transition-property is 'none'.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89f84623ff97
Handle values as valid in single transition shorthand even if the transition-property is 'none'. r=xidorn

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/89f84623ff97