228125 2021-07-20 13:58:02 -0700 [SVG] Attribute change triggers redundant (and out of order) setNeedsLayout call 2022-01-21 15:07:42 -0800 1 1 1 Unclassified WebKit SVG WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=230296 InRadar P2 Normal --- 1 zalan rbuis bfulgham darin dino ews-watchlist fmalita gyuyoung.kim pdr rbuis sabouhallawa schenney sergio simon.fraser svillar webkit-bug-importer zalan zimmermann oldest_to_newest 1778278 0 zalan 2021-07-20 13:58:02 -0700 <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). 1778281 1 webkit-bug-importer 2021-07-20 14:20:19 -0700 <rdar://problem/80857688> 1781217 2 svillar 2021-08-02 03:38:12 -0700 (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. 1781237 3 zalan 2021-08-02 07:51:22 -0700 (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. 1782694 4 435131 rbuis 2021-08-07 07:57:57 -0700 Created attachment 435131 Patch 1782730 5 435136 rbuis 2021-08-07 13:29:21 -0700 Created attachment 435136 Patch 1782853 6 435178 rbuis 2021-08-09 02:31:55 -0700 Created attachment 435178 Patch 1783416 7 ews-feeder 2021-08-11 00:43:49 -0700 Committed r280887 (240424@main): <https://commits.webkit.org/240424@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435178. 435131 2021-08-07 07:57:57 -0700 2021-08-07 13:29:15 -0700 Patch bug-228125-20210807165755.patch text/plain 1751 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjgwNzM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDMzNmUzMTVhNTc0MDE4 MmJjZjdiZTIwNDc2MzQyMzg2MzBiMzY1My4uN2FkNGE5NDQ3MTVkOTE5ZmMxMTI5ZDk1ZmIyZmQ1 NzQ3MWIyYmQzZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDIxLTA4LTA3ICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW1NWR10gQXR0cmlidXRlIGNoYW5n ZSB0cmlnZ2VycyByZWR1bmRhbnQgKGFuZCBvdXQgb2Ygb3JkZXIpIHNldE5lZWRzTGF5b3V0IGNh bGwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIyODEy NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdJUC4K KworICAgICAgICAqIHN2Zy9TVkdTVkdFbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNW R1NWR0VsZW1lbnQ6OnN2Z0F0dHJpYnV0ZUNoYW5nZWQpOgorCiAyMDIxLTA4LTA2ICBZb3Vlbm4g RmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAogICAgICAgICBbR1BVUF0gVGVzdCBXZWJLaXQy LkNyYXNoR1BVUHJvY2Vzc0FmdGVyQXBwbHlpbmdDb25zdHJhaW50cyBmYWlscyB3aGVuIE1lZGlh IGluIEdQVSBQcm9jZXNzIGlzIGVuYWJsZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3N2 Zy9TVkdTVkdFbGVtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3N2Zy9TVkdTVkdFbGVtZW50LmNw cAppbmRleCA5NTM1ZmE5YWIwZGNiYzFiZmJjNWZhNjc5NjY2N2M1ZmY5YTdkZDBmLi45YWFiMmQz NTNkODFkZTlkZjZjN2MyYmM1OTljOWNhOTA5YjA3Y2UzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9zdmcvU1ZHU1ZHRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvc3ZnL1NWR1NW R0VsZW1lbnQuY3BwCkBAIC0yMDksMTEgKzIwOSwxMSBAQCB2b2lkIFNWR1NWR0VsZW1lbnQ6OnN2 Z0F0dHJpYnV0ZUNoYW5nZWQoY29uc3QgUXVhbGlmaWVkTmFtZSYgYXR0ck5hbWUpCiAgICAgICAg IEluc3RhbmNlSW52YWxpZGF0aW9uR3VhcmQgZ3VhcmQoKnRoaXMpOwogICAgICAgICBpbnZhbGlk YXRlU1ZHUHJlc2VudGF0aW9uYWxIaW50U3R5bGUoKTsKIAotICAgICAgICBpZiAoYXV0byByZW5k ZXJlciA9IHRoaXMtPnJlbmRlcmVyKCkpCi0gICAgICAgICAgICBSZW5kZXJTVkdSZXNvdXJjZTo6 bWFya0ZvckxheW91dEFuZFBhcmVudFJlc291cmNlSW52YWxpZGF0aW9uKCpyZW5kZXJlcik7Cisg ICAgICAgIGlmIChpczxSZW5kZXJTVkdSb290PihyZW5kZXJlcigpKSAmJiBkb3duY2FzdDxSZW5k ZXJTVkdSb290PigqcmVuZGVyZXIoKSkuaXNFbWJlZGRlZFRocm91Z2hGcmFtZUNvbnRhaW5pbmdT VkdEb2N1bWVudCgpKQorICAgICAgICAgICAgcmVuZGVyZXIoKS0+c2V0TmVlZHNMYXlvdXQoTWFy a0NvbnRhaW5pbmdCbG9ja0NoYWluKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0KLSAgICAKKwog ICAgIGlmIChTVkdGaXRUb1ZpZXdCb3g6OmlzS25vd25BdHRyaWJ1dGUoYXR0ck5hbWUpKSB7CiAg ICAgICAgIGlmIChhdXRvKiByZW5kZXJlciA9IHRoaXMtPnJlbmRlcmVyKCkpIHsKICAgICAgICAg ICAgIHJlbmRlcmVyLT5zZXROZWVkc1RyYW5zZm9ybVVwZGF0ZSgpOwo= 435136 2021-08-07 13:29:21 -0700 2021-08-09 02:31:50 -0700 Patch bug-228125-20210807222920.patch text/plain 1898 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjgwNzM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDMzNmUzMTVhNTc0MDE4 MmJjZjdiZTIwNDc2MzQyMzg2MzBiMzY1My4uNjI4N2ZkZDU1YTE1YmI2MTM0YmU0YjA2ZmY4MjM5 OGU3ZDZiNDExMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIxLTA4LTA3ICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW1NWR10gQXR0cmlidXRlIGNoYW5n ZSB0cmlnZ2VycyByZWR1bmRhbnQgKGFuZCBvdXQgb2Ygb3JkZXIpIHNldE5lZWRzTGF5b3V0IGNh bGwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIyODEy NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFzIGV4 cGxhaW5lZCBpbiB0aGUgYnVnIHRoZSBzZXROZWVkc0xheW91dCBjYWxsIGlzIHJlZHVuZGFudCwg aG93ZXZlcgorICAgICAgICBrZWVwIHVzaW5nIGl0IGZvciBlbWJlZGRlZCBTVkcncyB0byB1cGRh dGUgd2lkdGgvaGVpZ2h0IG9mIHRoZSBlbWJlZGRlci4KKworICAgICAgICAqIHN2Zy9TVkdTVkdF bGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNWR1NWR0VsZW1lbnQ6OnN2Z0F0dHJpYnV0 ZUNoYW5nZWQpOgorCiAyMDIxLTA4LTA2ICBZb3Vlbm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNv bT4KIAogICAgICAgICBbR1BVUF0gVGVzdCBXZWJLaXQyLkNyYXNoR1BVUHJvY2Vzc0FmdGVyQXBw bHlpbmdDb25zdHJhaW50cyBmYWlscyB3aGVuIE1lZGlhIGluIEdQVSBQcm9jZXNzIGlzIGVuYWJs ZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3N2Zy9TVkdTVkdFbGVtZW50LmNwcCBiL1Nv dXJjZS9XZWJDb3JlL3N2Zy9TVkdTVkdFbGVtZW50LmNwcAppbmRleCA5NTM1ZmE5YWIwZGNiYzFi ZmJjNWZhNjc5NjY2N2M1ZmY5YTdkZDBmLi45YWFiMmQzNTNkODFkZTlkZjZjN2MyYmM1OTljOWNh OTA5YjA3Y2UzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9zdmcvU1ZHU1ZHRWxlbWVudC5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvc3ZnL1NWR1NWR0VsZW1lbnQuY3BwCkBAIC0yMDksMTEg KzIwOSwxMSBAQCB2b2lkIFNWR1NWR0VsZW1lbnQ6OnN2Z0F0dHJpYnV0ZUNoYW5nZWQoY29uc3Qg UXVhbGlmaWVkTmFtZSYgYXR0ck5hbWUpCiAgICAgICAgIEluc3RhbmNlSW52YWxpZGF0aW9uR3Vh cmQgZ3VhcmQoKnRoaXMpOwogICAgICAgICBpbnZhbGlkYXRlU1ZHUHJlc2VudGF0aW9uYWxIaW50 U3R5bGUoKTsKIAotICAgICAgICBpZiAoYXV0byByZW5kZXJlciA9IHRoaXMtPnJlbmRlcmVyKCkp Ci0gICAgICAgICAgICBSZW5kZXJTVkdSZXNvdXJjZTo6bWFya0ZvckxheW91dEFuZFBhcmVudFJl c291cmNlSW52YWxpZGF0aW9uKCpyZW5kZXJlcik7CisgICAgICAgIGlmIChpczxSZW5kZXJTVkdS b290PihyZW5kZXJlcigpKSAmJiBkb3duY2FzdDxSZW5kZXJTVkdSb290PigqcmVuZGVyZXIoKSku aXNFbWJlZGRlZFRocm91Z2hGcmFtZUNvbnRhaW5pbmdTVkdEb2N1bWVudCgpKQorICAgICAgICAg ICAgcmVuZGVyZXIoKS0+c2V0TmVlZHNMYXlvdXQoTWFya0NvbnRhaW5pbmdCbG9ja0NoYWluKTsK ICAgICAgICAgcmV0dXJuOwogICAgIH0KLSAgICAKKwogICAgIGlmIChTVkdGaXRUb1ZpZXdCb3g6 OmlzS25vd25BdHRyaWJ1dGUoYXR0ck5hbWUpKSB7CiAgICAgICAgIGlmIChhdXRvKiByZW5kZXJl ciA9IHRoaXMtPnJlbmRlcmVyKCkpIHsKICAgICAgICAgICAgIHJlbmRlcmVyLT5zZXROZWVkc1Ry YW5zZm9ybVVwZGF0ZSgpOwo= 435178 2021-08-09 02:31:55 -0700 2021-08-11 00:43:50 -0700 Patch bug-228125-20210809113153.patch text/plain 1969 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjgwNzY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmQ2ZDVmN2Y1YmViYjE1 NWQwYTE0YmJiOTQ4ZGU1ZTlmZDU1NTljOS4uNjk3MmU3OGVmNmVlNjVkZTgyMWJkZDNlYjMxYTll ZmM1ZGEzMGZmYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIxLTA4LTA5ICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW1NWR10gQXR0cmlidXRlIGNoYW5n ZSB0cmlnZ2VycyByZWR1bmRhbnQgKGFuZCBvdXQgb2Ygb3JkZXIpIHNldE5lZWRzTGF5b3V0IGNh bGwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIyODEy NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFzIGV4 cGxhaW5lZCBpbiB0aGUgYnVnIHRoZSBzZXROZWVkc0xheW91dCBjYWxsIGlzIHJlZHVuZGFudCwg aG93ZXZlcgorICAgICAgICBrZWVwIHVzaW5nIGl0IGZvciBlbWJlZGRlZCBTVkcncyB0byB1cGRh dGUgd2lkdGgvaGVpZ2h0IG9mIHRoZSBlbWJlZGRlci4KKworICAgICAgICAqIHN2Zy9TVkdTVkdF bGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNWR1NWR0VsZW1lbnQ6OnN2Z0F0dHJpYnV0 ZUNoYW5nZWQpOgorCiAyMDIxLTA4LTA4ICBKZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29t PgogCiAgICAgICAgIFJFR1JFU1NJT046IGh0dHAvdGVzdHMvcHJlbG9hZC9vbmxvYWRfZXZlbnQu aHRtbCBpcyBhIGZsYWt5IHRpbWVvdXQgb24gQmlnIFN1ciB3azEgUmVsZWFzZQpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvc3ZnL1NWR1NWR0VsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUv c3ZnL1NWR1NWR0VsZW1lbnQuY3BwCmluZGV4IDk1MzVmYTlhYjBkY2JjMWJmYmM1ZmE2Nzk2NjY3 YzVmZjlhN2RkMGYuLjkwYzFjZmY0NWQxNDMyODE2MjAxMTM2YjFjZThmNTljNWI4ZDY0OGYgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3N2Zy9TVkdTVkdFbGVtZW50LmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9zdmcvU1ZHU1ZHRWxlbWVudC5jcHAKQEAgLTIwOSwxMSArMjA5LDEzIEBAIHZv aWQgU1ZHU1ZHRWxlbWVudDo6c3ZnQXR0cmlidXRlQ2hhbmdlZChjb25zdCBRdWFsaWZpZWROYW1l JiBhdHRyTmFtZSkKICAgICAgICAgSW5zdGFuY2VJbnZhbGlkYXRpb25HdWFyZCBndWFyZCgqdGhp cyk7CiAgICAgICAgIGludmFsaWRhdGVTVkdQcmVzZW50YXRpb25hbEhpbnRTdHlsZSgpOwogCi0g ICAgICAgIGlmIChhdXRvIHJlbmRlcmVyID0gdGhpcy0+cmVuZGVyZXIoKSkKLSAgICAgICAgICAg IFJlbmRlclNWR1Jlc291cmNlOjptYXJrRm9yTGF5b3V0QW5kUGFyZW50UmVzb3VyY2VJbnZhbGlk YXRpb24oKnJlbmRlcmVyKTsKKyAgICAgICAgaWYgKGF1dG8gcmVuZGVyZXIgPSB0aGlzLT5yZW5k ZXJlcigpKSB7CisgICAgICAgICAgICBpZiAoaXM8UmVuZGVyU1ZHUm9vdD4ocmVuZGVyZXIpICYm IGRvd25jYXN0PFJlbmRlclNWR1Jvb3Q+KCpyZW5kZXJlcikuaXNFbWJlZGRlZFRocm91Z2hGcmFt ZUNvbnRhaW5pbmdTVkdEb2N1bWVudCgpKQorICAgICAgICAgICAgICAgIFJlbmRlclNWR1Jlc291 cmNlOjptYXJrRm9yTGF5b3V0QW5kUGFyZW50UmVzb3VyY2VJbnZhbGlkYXRpb24oKnJlbmRlcmVy KTsKKyAgICAgICAgfQogICAgICAgICByZXR1cm47CiAgICAgfQotICAgIAorCiAgICAgaWYgKFNW R0ZpdFRvVmlld0JveDo6aXNLbm93bkF0dHJpYnV0ZShhdHRyTmFtZSkpIHsKICAgICAgICAgaWYg KGF1dG8qIHJlbmRlcmVyID0gdGhpcy0+cmVuZGVyZXIoKSkgewogICAgICAgICAgICAgcmVuZGVy ZXItPnNldE5lZWRzVHJhbnNmb3JtVXBkYXRlKCk7Cg==