218211 2020-10-26 15:55:02 -0700 RenderStyle::resetPadding sets incorrect computed value (auto) 2020-10-27 10:02:11 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 zalan zalan bfulgham changseok darin esprehn+autocc ews-watchlist glenn koivisto kondapallykalyan pdr simon.fraser webkit-bug-importer zalan oldest_to_newest 1701662 0 zalan 2020-10-26 15:55:02 -0700 should be 0 1701664 1 412361 zalan 2020-10-26 16:01:33 -0700 Created attachment 412361 Patch 1701665 2 zalan 2020-10-26 16:02:17 -0700 Alternatively we could just set it to LengthBox(Fixed). Not sure what the preferred way is in RenderStyle. 1701675 3 412361 simon.fraser 2020-10-26 16:33:35 -0700 Comment on attachment 412361 Patch Is this testable with CSS reset? 1701692 4 412361 darin 2020-10-26 17:09:40 -0700 Comment on attachment 412361 Patch Does this have any effect? 1701714 5 zalan 2020-10-26 18:49:18 -0700 (In reply to Darin Adler from comment #4) > Comment on attachment 412361 [details] > Patch > > Does this have any effect? Absolutely. This is how in IFC we normally resolve the computed padding value (e.g. padding left): valueForLength(style.paddingLeft(), containingBlockWidth); Now for the (incorrect)auto specified value, valueForLength returns the containingBlockWidth as opposed to the expected, initial value of 0. (I noticed this after enabling <select> element in IFC. RenderTheme calls style.resetPadding() on the select renderer and when IFC resolves the computed value, it sees a non-zero padding left/right/top/bottom). 1701717 6 zalan 2020-10-26 18:59:26 -0700 (In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 412361 [details] > Patch > > Is this testable with CSS reset? the unset? It does not seem to call resetPadding. 1701846 7 412361 koivisto 2020-10-27 06:14:13 -0700 Comment on attachment 412361 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412361&action=review > Source/WebCore/rendering/style/RenderStyle.h:1027 > + void resetPadding() { SET_VAR(m_surroundData, padding, LengthBox(initialPadding().intValue())); } Just initialPadding()? 1701848 8 zalan 2020-10-27 06:19:07 -0700 (In reply to Antti Koivisto from comment #7) > Comment on attachment 412361 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412361&action=review > > > Source/WebCore/rendering/style/RenderStyle.h:1027 > > + void resetPadding() { SET_VAR(m_surroundData, padding, LengthBox(initialPadding().intValue())); } > > Just initialPadding()? That was my initial attempt, but LengthBox does not take Length. 1701853 9 ews-feeder 2020-10-27 06:28:45 -0700 Committed r269037: <https://trac.webkit.org/changeset/269037> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412361. 1701854 10 webkit-bug-importer 2020-10-27 06:29:21 -0700 <rdar://problem/70719975> 1701906 11 darin 2020-10-27 09:21:54 -0700 If this has an effect why isn’t a test case going from fail to pass? 1701923 12 zalan 2020-10-27 09:55:21 -0700 (In reply to Darin Adler from comment #11) > If this has an effect why isn’t a test case going from fail to pass? I should have been more specific. There's no effect on trunk yet because the rendering code is written in a way that the incorrect 'auto' value gets resolved to 0. LayoutUnit RenderBoxModelObject::computedCSSPadding(const Length& padding) const { LayoutUnit w; if (padding.isPercentOrCalculated()) w = containingBlockLogicalWidthForContent(); return minimumValueForLength(padding, w); } minimumValueForLength() returns 0 for auto values. The "from fail to pass" progression is behind a flag which is currently turned off on trunk (where we start exercising some more LFC code for inline content, specifically where LFC calls valueForLength(style.paddingLeft(), containingBlockWidth) to resolve the computed value.) I could have waited with this fix until after we enabled the flag to show the progression but it looked like a valid correctness fix in itself (since 'auto' is an incorrect value for padding). 1701931 13 darin 2020-10-27 10:02:11 -0700 (In reply to zalan from comment #12) > There's no effect on trunk yet because the > rendering code is written in a way that the incorrect 'auto' value gets > resolved to 0. OK. 412361 2020-10-26 16:01:33 -0700 2020-10-27 06:28:46 -0700 Patch bug-218211-20201026160132.patch text/plain 1604 zalan U3VidmVyc2lvbiBSZXZpc2lvbjogMjY4OTc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNWIzY2M2MGQyNzA3ZGMw ZDY4ZTAyMTc1Yjk5N2ZkMjhhZDE4ZTJhNC4uNWU0NGQ3MjdhOTMwZGI4N2FjYzQ5MDZlNDE0OWU1 MDk4ZDFlMmFjZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDIwLTEwLTI2ICBaYWxh biBCdWp0YXMgIDx6YWxhbkBhcHBsZS5jb20+CisKKyAgICAgICAgUmVuZGVyU3R5bGU6OnJlc2V0 UGFkZGluZyBzZXRzIGluY29ycmVjdCBjb21wdXRlZCB2YWx1ZSAoYXV0bykKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxODIxMQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcmVuZGVyaW5nL3N0eWxlL1Jl bmRlclN0eWxlLmg6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJTdHlsZTo6cmVzZXRQYWRkaW5n KToKKwogMjAyMC0xMC0yNiAgWGFiaWVyIFJvZHJpZ3VleiBDYWx2YXIgIDxjYWx2YXJpc0BpZ2Fs aWEuY29tPgogCiAgICAgICAgIFtHU3RyZWFtZXJdW0VNRV0gUmVtb3ZlIHVudXNlZCBwcm90ZWN0 aW9uIGV2ZW4gaW4gY29tbW9uIGRlY3J5cHRvcgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL3N0eWxlL1JlbmRlclN0eWxlLmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv c3R5bGUvUmVuZGVyU3R5bGUuaAppbmRleCA1NTc2NDljMWI5ZjlkYTY0NGRmMDJjY2E5MmJhYzY0 NzkwZGI0Y2E1Li40YTI3NWJiMTBiM2VlMjRiMGEzNmI1OTEyNTE1ZTExYTg5MGJkOTNhIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvUmVuZGVyU3R5bGUuaAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvUmVuZGVyU3R5bGUuaApAQCAtMTAyNCw3 ICsxMDI0LDcgQEAgcHVibGljOgogICAgIHZvaWQgc2V0TWFyZ2luU3RhcnQoTGVuZ3RoJiYpOwog ICAgIHZvaWQgc2V0TWFyZ2luRW5kKExlbmd0aCYmKTsKIAotICAgIHZvaWQgcmVzZXRQYWRkaW5n KCkgeyBTRVRfVkFSKG1fc3Vycm91bmREYXRhLCBwYWRkaW5nLCBMZW5ndGhCb3goQXV0bykpOyB9 CisgICAgdm9pZCByZXNldFBhZGRpbmcoKSB7IFNFVF9WQVIobV9zdXJyb3VuZERhdGEsIHBhZGRp bmcsIExlbmd0aEJveChpbml0aWFsUGFkZGluZygpLmludFZhbHVlKCkpKTsgfQogICAgIHZvaWQg c2V0UGFkZGluZ0JveChMZW5ndGhCb3gmJiBib3gpIHsgU0VUX1ZBUihtX3N1cnJvdW5kRGF0YSwg cGFkZGluZywgV1RGTW92ZShib3gpKTsgfQogICAgIHZvaWQgc2V0UGFkZGluZ1RvcChMZW5ndGgm JiBsZW5ndGgpIHsgU0VUX1ZBUihtX3N1cnJvdW5kRGF0YSwgcGFkZGluZy50b3AoKSwgV1RGTW92 ZShsZW5ndGgpKTsgfQogICAgIHZvaWQgc2V0UGFkZGluZ0JvdHRvbShMZW5ndGgmJiBsZW5ndGgp IHsgU0VUX1ZBUihtX3N1cnJvdW5kRGF0YSwgcGFkZGluZy5ib3R0b20oKSwgV1RGTW92ZShsZW5n dGgpKTsgfQo=