If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

stylo: causes extreme redrawing issues on Quizlet Learn

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
14 days ago
11 days ago

People

(Reporter: Lee Bousfield, Assigned: hiro)

Tracking

(Blocks: 1 bug)

57 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

14 days ago
Created attachment 8904833 [details]
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.
Blocks: 1375906
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)
(Assignee)

Comment 2

13 days ago
Yeah, transition seems to be broken there. I take this.
Assignee: nobody → hikezoe
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(hikezoe)
Thanks!
(Assignee)

Comment 4

13 days ago
Created attachment 8905319 [details]
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)
(Reporter)

Comment 5

13 days ago
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
(Assignee)

Comment 6

13 days ago
Thanks. Yeah, I think so.  I am now fixing it.
(Assignee)

Comment 7

13 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d4c856e31f4f7f9fa556fb5a7ccc2a7b75b69c7
(Assignee)

Comment 8

13 days ago
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
(Assignee)

Updated

13 days ago
Summary: Stylo: causes extreme redrawing issues on Quizlet Learn → stylo: causes extreme redrawing issues on Quizlet Learn
(Assignee)

Updated

13 days ago
Component: Layout → CSS Parsing and Computation
(Assignee)

Comment 9

12 days ago
Oops, I did miss resetting cssparser::Parser state and fixing serialization.
(Assignee)

Comment 10

12 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ccb22a8bb1acf9851a0cf6c3c65ac52a2d260a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

12 days ago
mozreview-review
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 14

12 days ago
mozreview-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 15

12 days ago
mozreview-review-reply
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.
(Assignee)

Comment 16

12 days ago
(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 hidden (mozreview-request)

Comment 18

12 days ago
mozreview-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/#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+
(Assignee)

Comment 19

11 days ago
Created attachment 8906192 [details] [review]
Servo PR

Thanks for the review, Xidorn!

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe48833b4b71d8930903922867ecdb16420395f
Comment hidden (mozreview-request)
(Assignee)

Updated

11 days ago
Attachment #8905788 - Attachment is obsolete: true

Comment 21

11 days ago
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
Last Resolved: 11 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.