Bug 228125 – [SVG] Attribute change triggers redundant (and out of order) setNeedsLayout call

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[zalan]

<svg id=changeThis></svg>
<script>
  document.body.offsetHeight;
  changeThis.setAttribute("width","100px");
</script>

The RenderObject::setNeedsLayout(MarkContainingBlockChain) triggered by the SVGElement::attributeChanged (SVGElement::attributeChanged -> SVGSVGElement::svgAttributeChanged -> RenderSVGResource::markForLayoutAndParentResourceInvalidation()) is incorrect and redundant.
Incorrect in the context of how the dirty bits on the render tree should be populated. It is out of order i.e we have to have all the ancestors resolved first when marking descendant renderers dirty.
Redundant because this attribute change on the SVG element also triggers style invalidation as part of the "presentational hints" flow (which applies to pretty much all the valid SVG attributes see SVGElement::hasPresentationalHintsForAttribute) and this style invalidation path would properly update the associated renderer's dirty bit through the normal RenderTreeUpdater process (as if it was just yet another non-svg style update e.g. <div dir=ltr <-changing it to rtl).

[Radar WebKit Bug Importer]

<rdar://problem/80857688>

[Sergio Villar Senin]

(In reply to zalan from comment #0)
> <svg id=changeThis></svg>
> <script>
>   document.body.offsetHeight;
>   changeThis.setAttribute("width","100px");
> </script>
> 
> The RenderObject::setNeedsLayout(MarkContainingBlockChain) triggered by the
> SVGElement::attributeChanged (SVGElement::attributeChanged ->
> SVGSVGElement::svgAttributeChanged ->
> RenderSVGResource::markForLayoutAndParentResourceInvalidation()) is
> incorrect and redundant.
> Incorrect in the context of how the dirty bits on the render tree should be
> populated. It is out of order i.e we have to have all the ancestors resolved
> first when marking descendant renderers dirty.
> Redundant because this attribute change on the SVG element also triggers
> style invalidation as part of the "presentational hints" flow (which applies
> to pretty much all the valid SVG attributes see
> SVGElement::hasPresentationalHintsForAttribute) and this style invalidation
> path would properly update the associated renderer's dirty bit through the
> normal RenderTreeUpdater process (as if it was just yet another non-svg
> style update e.g. <div dir=ltr <-changing it to rtl).

Removing the RenderSVGResource::markForLayoutAndParentResourceInvalidation call in SVGSVGElement::svgAttributeChanged is not harmless though as it makes the test LayoutTests/svg/as-object/embedded-svg-size-changes.html fail.

[zalan]

(In reply to Sergio Villar Senin from comment #2)
> (In reply to zalan from comment #0)
> > <svg id=changeThis></svg>
> > <script>
> >   document.body.offsetHeight;
> >   changeThis.setAttribute("width","100px");
> > </script>
> > 
> > The RenderObject::setNeedsLayout(MarkContainingBlockChain) triggered by the
> > SVGElement::attributeChanged (SVGElement::attributeChanged ->
> > SVGSVGElement::svgAttributeChanged ->
> > RenderSVGResource::markForLayoutAndParentResourceInvalidation()) is
> > incorrect and redundant.
> > Incorrect in the context of how the dirty bits on the render tree should be
> > populated. It is out of order i.e we have to have all the ancestors resolved
> > first when marking descendant renderers dirty.
> > Redundant because this attribute change on the SVG element also triggers
> > style invalidation as part of the "presentational hints" flow (which applies
> > to pretty much all the valid SVG attributes see
> > SVGElement::hasPresentationalHintsForAttribute) and this style invalidation
> > path would properly update the associated renderer's dirty bit through the
> > normal RenderTreeUpdater process (as if it was just yet another non-svg
> > style update e.g. <div dir=ltr <-changing it to rtl).
> 
> Removing the RenderSVGResource::markForLayoutAndParentResourceInvalidation
> call in SVGSVGElement::svgAttributeChanged is not harmless though as it
> makes the test LayoutTests/svg/as-object/embedded-svg-size-changes.html fail.
Yup and that's what someone (preferably with WebKit SVG background) needs to figure out and adjust some other part of the code accordingly.

[Rob Buis]

Created attachment 435131 [details]
Patch

[Rob Buis]

Created attachment 435136 [details]
Patch

[Rob Buis]

Created attachment 435178 [details]
Patch

[EWS]

Committed r280887 (240424@main): <https://commits.webkit.org/240424@main>

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