Bug 216295 – [Repaint] styleWillChange may call repaint on the same renderer multiple times.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

Let's not do that.

[zalan]

Created attachment 408286 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/68538666>

[Simon Fraser (smfr)]

Comment on attachment 408286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408286&action=review

> Source/WebCore/rendering/RenderElement.cpp:426
> +    if (is<RenderLayerModelObject>(*this)) {
> +        auto& renderer = downcast<RenderLayerModelObject>(*this);
> +        if (oldStyle && parent()) {
> +            if (diff == StyleDifference::Repaint) {
> +                renderer.repaint();
> +                return true;
> +            }
> +            if (newStyle.outlineSize() < oldStyle->outlineSize()) {
> +                renderer.repaint();
> +                return true;
> +            }
> +        }
> +        if (!renderer.hasLayer() && (newStyle.hasTransform() || newStyle.opacity() < 1 || newStyle.hasFilter() || newStyle.hasBackdropFilter())) {
> +            // If we don't have a layer yet, but we are going to get one because of transform or opacity, then we need to repaint the old position of the object.
> +            renderer.repaint();
> +            return true;
> +        }
> +    }
> +
> +    if (is<RenderBox>(*this)) {
> +        auto& renderer = downcast<RenderBox>(*this);
> +        if (diff == StyleDifference::Layout && renderer.parent() && oldStyle && oldStyle->position() != newStyle.position() && oldStyle->position() == PositionType::Static) {
> +            renderer.repaint();
> +            return true;
> +        }
> +    }
> +
> +    // RenderElement repaint invalidation.
> +    if (parent() && (shouldRepaintForStyleDifference(diff) || (oldStyle && newStyle.outlineSize() < oldStyle->outlineSize()))) {
> +        repaint();
> +        return true;
> +    }
> +
> +    if (diff > StyleDifference::RepaintLayer && enclosingLayer() && oldStyle && oldStyle->visibility() != newStyle.visibility()) {
> +        auto rendererWillBeHidden = newStyle.visibility() != Visibility::Visible;
> +        auto& layer = *enclosingLayer();
> +        if (rendererWillBeHidden && layer.hasVisibleContent() && (this == &layer.renderer() || layer.renderer().style().visibility() != Visibility::Visible)) {
> +            repaint();
> +            return true;
> +        }
> +    }
> +

How about one big lambda for needsRepaint() and then just a single call to repaint()?

[zalan]

> How about one big lambda for needsRepaint() and then just a single call to
> repaint()?
yeah good idea. this patch is mostly just a mechanical copy/paste to gather layout results.

[zalan]

Created attachment 408334 [details]
Patch

[zalan]

Created attachment 408339 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 408334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408334&action=review

> Source/WebCore/rendering/RenderElement.cpp:383
> +void RenderElement::issueRepaintBeforeStyleChange(StyleDifference diff, const RenderStyle* oldStyle, const RenderStyle& newStyle)

Maybe just repaintBeforeStyleChange()?

> Source/WebCore/rendering/RenderElement.cpp:394
> +            if (!renderer.hasLayer() && (newStyle.hasTransform() || newStyle.opacity() < 1 || newStyle.hasFilter() || newStyle.hasBackdropFilter())) {

It's weird that we test for opacity, transform etc explicitly here, rather than have a "style creates layer" function.

> Source/WebCore/rendering/RenderElement.cpp:415
> +        if (is<RenderBox>(*this)) {
> +            auto& renderer = downcast<RenderBox>(*this);
> +            if (diff == StyleDifference::Layout && renderer.parent() && oldStyle && oldStyle->position() != newStyle.position() && oldStyle->position() == PositionType::Static)
> +                return true;
> +        }
> +
> +        // RenderElement repaint invalidation.
> +        if (parent() && (shouldRepaintForStyleDifference(diff) || (oldStyle && newStyle.outlineSize() < oldStyle->outlineSize())))
> +            return true;
> +
> +        if (diff > StyleDifference::RepaintLayer && enclosingLayer() && oldStyle && oldStyle->visibility() != newStyle.visibility()) {
> +            auto rendererWillBeHidden = newStyle.visibility() != Visibility::Visible;
> +            auto& layer = *enclosingLayer();
> +            if (rendererWillBeHidden && layer.hasVisibleContent() && (this == &layer.renderer() || layer.renderer().style().visibility() != Visibility::Visible))
> +                return true;
> +        }

Maybe pull all the !oldStyle code into a first condition and return. There are also several parent() checks that could maybe change to an early return.

[zalan]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 408334 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408334&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:383
> > +void RenderElement::issueRepaintBeforeStyleChange(StyleDifference diff, const RenderStyle* oldStyle, const RenderStyle& newStyle)
> 
> Maybe just repaintBeforeStyleChange()?
> 
> > Source/WebCore/rendering/RenderElement.cpp:394
> > +            if (!renderer.hasLayer() && (newStyle.hasTransform() || newStyle.opacity() < 1 || newStyle.hasFilter() || newStyle.hasBackdropFilter())) {
> 
> It's weird that we test for opacity, transform etc explicitly here, rather
> than have a "style creates layer" function.
> 
> > Source/WebCore/rendering/RenderElement.cpp:415
> > +        if (is<RenderBox>(*this)) {
> > +            auto& renderer = downcast<RenderBox>(*this);
> > +            if (diff == StyleDifference::Layout && renderer.parent() && oldStyle && oldStyle->position() != newStyle.position() && oldStyle->position() == PositionType::Static)
> > +                return true;
> > +        }
> > +
> > +        // RenderElement repaint invalidation.
> > +        if (parent() && (shouldRepaintForStyleDifference(diff) || (oldStyle && newStyle.outlineSize() < oldStyle->outlineSize())))
> > +            return true;
> > +
> > +        if (diff > StyleDifference::RepaintLayer && enclosingLayer() && oldStyle && oldStyle->visibility() != newStyle.visibility()) {
> > +            auto rendererWillBeHidden = newStyle.visibility() != Visibility::Visible;
> > +            auto& layer = *enclosingLayer();
> > +            if (rendererWillBeHidden && layer.hasVisibleContent() && (this == &layer.renderer() || layer.renderer().style().visibility() != Visibility::Visible))
> > +                return true;
> > +        }
> 
> Maybe pull all the !oldStyle code into a first condition and return. There
> are also several parent() checks that could maybe change to an early return.
My thinking here was to do it in 2 steps.
1. preserve the logic as it was (copy&paste mostly) so that in case of regression it's easy to figure out what went wrong.
2. refactor the code

[zalan]

Created attachment 408347 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 408347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408347&action=review

> Source/WebCore/rendering/RenderElement.cpp:383
> +void RenderElement::issueRepaintBeforeStyleChange(StyleDifference diff, const RenderStyle& oldStyle, const RenderStyle& newStyle)

s/issue//

> Source/WebCore/rendering/RenderElement.cpp:387
> +            // Can't resolve absolute coordinates.

Or more accurately, we're disconnected so don't need to repaint anyway.

> Source/WebCore/rendering/RenderElement.cpp:392
> +            if (diff == StyleDifference::Repaint)
> +                return true;

No need to change in this patch, but shouldRepaintForStyleDifference() will handle this below.

> Source/WebCore/rendering/RenderElement.cpp:400
> +                auto& layer = *enclosingLayer();

I think you've introduced a behavior change here. The old code lived on RenderElement and enclosingLayer() could return the layer for an ancestor. So this renders hasLayer could be false, but we would have found an enclosing layer. That no longer happens.

[zalan]

Created attachment 408351 [details]
Patch

[zalan]

RenderStyle& oldStyle, const RenderStyle& newStyle)
> 
> s/issue//
Done.

> 
> > Source/WebCore/rendering/RenderElement.cpp:387
> > +            // Can't resolve absolute coordinates.
> 
> Or more accurately, we're disconnected so don't need to repaint anyway.
At this point this is about not being able to resolve absolute coordinates. In general disconnected renderers need repaints.

> 
> > Source/WebCore/rendering/RenderElement.cpp:392
> > +            if (diff == StyleDifference::Repaint)
> > +                return true;
> 
> No need to change in this patch, but shouldRepaintForStyleDifference() will
> handle this below.
Good point. Fixed.

> 
> > Source/WebCore/rendering/RenderElement.cpp:400
> > +                auto& layer = *enclosingLayer();
> 
> I think you've introduced a behavior change here. The old code lived on
> RenderElement and enclosingLayer() could return the layer for an ancestor.
> So this renders hasLayer could be false, but we would have found an
> enclosing layer. That no longer happens.
:(

[Simon Fraser (smfr)]

Comment on attachment 408351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408351&action=review

> Source/WebCore/rendering/RenderElement.cpp:407
> +        if (diff > StyleDifference::RepaintLayer && enclosingLayer() && oldStyle.visibility() != newStyle.visibility()) {
> +            auto rendererWillBeHidden = newStyle.visibility() != Visibility::Visible;
> +            auto& layer = *enclosingLayer();

enclosingLayer() is a tree walk so I'd store the result instead of calling it twice. Also testing oldStyle.visibility() != newStyle.visibility() is much faster than checking enclosingLayer() so do it first (it's also much rarer).

[zalan]

Created attachment 408353 [details]
Patch

[EWS]

Committed r266803: <https://trac.webkit.org/changeset/266803>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408353 [details].