Closed Bug 1397122 Opened 7 years ago Closed 7 years ago

stylo: causes extreme redrawing issues on Quizlet Learn

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ljbousfield, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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.
Priority: -- → P1
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?
Flags: needinfo?(hikezoe)
Flags: needinfo?(emilio)
Yeah, transition seems to be broken there. I take this.
Assignee: nobody → hikezoe
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(hikezoe)
Attached file 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.
Flags: needinfo?(emilio)
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
Thanks. Yeah, I think so.  I am now fixing it.
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
Summary: Stylo: causes extreme redrawing issues on Quizlet Learn → stylo: causes extreme redrawing issues on Quizlet Learn
Component: Layout → CSS Parsing and Computation
Oops, I did miss resetting cssparser::Parser state and fixing serialization.
Comment on attachment 8905788 [details]
Bug 1397122 - Simplify transition-property parser.

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

LGTM
Attachment #8905788 - Flags: review?(boris.chiou) → review+
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.
Attachment #8905789 - Flags: review?(xidorn+moz) → review-
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.
(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 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.
Attachment #8905789 - Flags: review?(xidorn+moz) → review+
Attachment #8905788 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/89f84623ff97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.