Closed Bug 1376073 Opened 6 years ago Closed 6 years ago

stylo: We reframe too eagerly for ::before/::after

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Bug 1376073. Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
The layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html mochitest catches this.

Simple testcase:

  <style>
    .x::before { color: green; }
  </style>
  <span onclick="this.className = 'x'">Some text; click me to reframe</span>

What happens is that in match_pseudos (matching.rs) we discover we have rules for the pseudo and do:

                matches_different_pseudos |= !data.styles.pseudos.has(&pseudo);
...
        if matches_different_pseudos && data.restyle.is_restyle() {
            // Any changes to the matched pseudo-elements trigger
            // reconstruction.
            data.restyle.damage |= RestyleDamage::reconstruct();

which is not needed in this case, because the ::before has "content: none" and hence doesn't actually render in any way.

I wonder whether we can just stop reframing here for ::before/::after and rely on compute_style_difference() triggering reframes as needed or something.
Hmm... I guess so, yeah. We only arrive there when there's an old pseudo-style right now, but we can probably massage a little the logic there to make it more general.
Assignee: nobody → bzbarsky
Comment on attachment 8891421 [details]
Bug 1376073.  Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after.

https://reviewboard.mozilla.org/r/162582/#review167988

Mostly semi-opinionated nits, feel free to address as you wish. It'd be nice to extend `layout/style/test/test_reframe_pseudo_element.html` to exercise this case, and shouldn't be too hard.

::: servo/components/style/matching.rs:550
(Diff revision 3)
> +                    // It's possible that we're switching from not having
> +                    // ::before/::after at all to having styles for them but not
> +                    // actually having a useful pseudo-element.  Check for that
> +                    // case.
> +                    let pseudo = PseudoElement::from_eager_index(i);
> +                    if old.as_ref().map_or(false,

nit: do you think this would benefit from splitting it in two different variables?

```
let new_pseudo_should_exist = ...;
let old_pseudo_should_exist = ...;

if new_pseudo_should_exist != new_pseudo_should_exist {
    // ...
```

::: servo/components/style/matching.rs:802
(Diff revision 3)
>  
>          if pseudo.map_or(false, |p| p.is_before_or_after()) {
> +            // FIXME(bz) This duplicates some of the logic in
> +            // PseudoElement::should_exist, but it's not clear how best to share
> +            // that logic without redoing the "get the display" work.
>              let old_style_generates_no_pseudo =

(I think getting the display value again is not a big deal, but your call)

::: servo/components/style/servo/selector_parser.rs:197
(Diff revision 3)
>          None
>      }
> +
> +    /// Whether this pseudo-element should actually exist if it has
> +    /// the given styles.
> +    pub fn should_exist(&self,

nit: This fits in the same line:

```
pub fn should_exist(&self, style: &ComputedValues) -> bool {
```

::: servo/components/style/servo/selector_parser.rs:201
(Diff revision 3)
> +    /// the given styles.
> +    pub fn should_exist(&self,
> +                        style: &ComputedValues)
> +    {
> +        let display = style.get_box().clone_display();
> +        display != display::T::none &&

Do you think this would be clearer (even if slightly longer) written like:

```
if display == display::T::none {
    return false;
}

if pseudo.is_before_or_after() && style.ineffective_content_property() {
    return false;
}

true
```

Seems slightly easier to extend if needed. But not a big deal anyway.
Attachment #8891421 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891421 [details]
Bug 1376073.  Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after.

https://reviewboard.mozilla.org/r/162582/#review167988

Yeah, I'll give extending that a shot.

> nit: do you think this would benefit from splitting it in two different variables?
> 
> ```
> let new_pseudo_should_exist = ...;
> let old_pseudo_should_exist = ...;
> 
> if new_pseudo_should_exist != new_pseudo_should_exist {
>     // ...
> ```

Yeah, probably a good idea.  Done.

> (I think getting the display value again is not a big deal, but your call)

I'm really not sure how hot this various code is and how slow the readback from the Gecko struct is...

> nit: This fits in the same line:
> 
> ```
> pub fn should_exist(&self, style: &ComputedValues) -> bool {
> ```

Done.

> Do you think this would be clearer (even if slightly longer) written like:
> 
> ```
> if display == display::T::none {
>     return false;
> }
> 
> if pseudo.is_before_or_after() && style.ineffective_content_property() {
>     return false;
> }
> 
> true
> ```
> 
> Seems slightly easier to extend if needed. But not a big deal anyway.

I went back and forth a few times here.  I guess it probably is clearer like that, will change it to that.
Comment on attachment 8891421 [details]
Bug 1376073.  Don't reframe when going from no pseudo to pseudo with no content for ::before and ::after.

https://reviewboard.mozilla.org/r/162582/#review167988

Oh, actually, this case is already exercised by layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html which we're failing without this patch.  So not going to worry about the test bit.
https://hg.mozilla.org/mozilla-central/rev/a2cb823abab5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.