Bug 216295

Summary: [Repaint] styleWillChange may call repaint on the same renderer multiple times.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review-
Patch
none
Patch none

Description zalan 2020-09-08 17:05:55 PDT
Let's not do that.
Comment 1 zalan 2020-09-08 17:07:13 PDT
Created attachment 408286 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-09-08 17:07:23 PDT
<rdar://problem/68538666>
Comment 3 Simon Fraser (smfr) 2020-09-08 17:49:57 PDT
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()?
Comment 4 zalan 2020-09-08 18:36:03 PDT
> 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.
Comment 5 zalan 2020-09-09 10:15:05 PDT
Created attachment 408334 [details]
Patch
Comment 6 zalan 2020-09-09 10:29:31 PDT
Created attachment 408339 [details]
Patch
Comment 7 Simon Fraser (smfr) 2020-09-09 10:29:51 PDT
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.
Comment 8 zalan 2020-09-09 10:34:51 PDT
(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
Comment 9 zalan 2020-09-09 11:04:20 PDT
Created attachment 408347 [details]
Patch
Comment 10 Simon Fraser (smfr) 2020-09-09 11:14:35 PDT
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.
Comment 11 zalan 2020-09-09 11:27:55 PDT
Created attachment 408351 [details]
Patch
Comment 12 zalan 2020-09-09 11:30:08 PDT
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.
:(
Comment 13 Simon Fraser (smfr) 2020-09-09 11:30:36 PDT
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).
Comment 14 zalan 2020-09-09 11:36:06 PDT
Created attachment 408353 [details]
Patch
Comment 15 EWS 2020-09-09 15:45:50 PDT
Committed r266803: <https://trac.webkit.org/changeset/266803>

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