Bug 228125 - [SVG] Attribute change triggers redundant (and out of order) setNeedsLayout call
Summary: [SVG] Attribute change triggers redundant (and out of order) setNeedsLayout call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-20 13:58 PDT by zalan
Modified: 2021-08-11 00:43 PDT (History)
16 users (show)

See Also:


Attachments
Patch (1.71 KB, patch)
2021-08-07 07:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2021-08-07 13:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2021-08-09 02:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2021-07-20 13:58:02 PDT
<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).
Comment 1 Radar WebKit Bug Importer 2021-07-20 14:20:19 PDT
<rdar://problem/80857688>
Comment 2 Sergio Villar Senin 2021-08-02 03:38:12 PDT
(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.
Comment 3 zalan 2021-08-02 07:51:22 PDT
(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.
Comment 4 Rob Buis 2021-08-07 07:57:57 PDT
Created attachment 435131 [details]
Patch
Comment 5 Rob Buis 2021-08-07 13:29:21 PDT
Created attachment 435136 [details]
Patch
Comment 6 Rob Buis 2021-08-09 02:31:55 PDT
Created attachment 435178 [details]
Patch
Comment 7 EWS 2021-08-11 00:43:49 PDT
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].