60609 2011-05-10 22:20:21 -0700 Implement CSSPropertyMargin and CSSPropertyPadding in CSSStyleApplyProperty. 2011-05-11 23:59:59 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 macpherson webkit-unassigned abarth commit-queue dglazkov eric macpherson webkit.review.bot oldest_to_newest 401762 0 macpherson 2011-05-10 22:20:21 -0700 Implement CSSPropertyMargin and CSSPropertyPadding in CSSStyleApplyProperty. 401766 1 93072 macpherson 2011-05-10 22:22:19 -0700 Created attachment 93072 Patch 401773 2 93072 eric 2011-05-10 22:29:45 -0700 Comment on attachment 93072 Patch OK. How does this do the resetPadding stuff? Or does it call each of the reset funtions individually (thus obsoleting the resetPadidng function?) 401795 3 macpherson 2011-05-10 23:30:11 -0700 resetMargin is equivalent to setting the top, left, bottom and right margins to RenderStyle::initialMargin() however, resetPadding sets top, left, bottom, right to AUTO, while the individual cases are setting them to FIXED. This smells a lot like a bug in the existing code - in every case I've seen so far initializing the shorthand is exactly equivalent to initializing the individual values. 401801 4 93072 eric 2011-05-10 23:39:10 -0700 Comment on attachment 93072 Patch Since we're changing behavior, we'll need a test. You can break out the non-behavior change from the behavior chagne if you like. r=me on landing the non-behavior change w/o further review! 401814 5 macpherson 2011-05-11 00:16:20 -0700 For reference, http://www.w3.org/TR/CSS21/box.html#propdef-padding says "Initial: see individual properties" 402540 6 macpherson 2011-05-11 18:49:25 -0700 (In reply to comment #5) > For reference, http://www.w3.org/TR/CSS21/box.html#propdef-padding says > "Initial: see individual properties" It appears that because of the way these values are used, Length(AUTO) and Length(FIXED) are treated equivalently. for example: int RenderBoxModelObject::paddingLeft(bool) const { int w = 0; Length padding = style()->paddingLeft(); if (padding.isPercent()) w = containingBlock()->availableLogicalWidth(); return padding.calcMinValue(w); } Will return 0 for both. Using AUTO in RenderStyle means that we are depending on the w=0 above for correct behavior. RenderStyle would better reflect the CSS spec by using RenderStyle::initialPadding() for its initial value. FYI, this is passing DRT and pixel tests for me, and I count 949 uses of "padding:" under LayoutTests/fast, so we already have good coverage there. 402568 7 eric 2011-05-11 19:45:54 -0700 Can we detect this change with getComputedStyle? 402614 8 macpherson 2011-05-11 20:46:21 -0700 (In reply to comment #7) > Can we detect this change with getComputedStyle? I just tested setting "padding: 0px;", "padding: auto;", "padding: 10%". In all cases getComputedStyle(e).getPropertyValue("padding-left") returns a px value; in the first two cases is 0px. I think that means this change is not detectable from JS because it always returns the calculated pixel value. 402617 9 93072 eric 2011-05-11 20:48:40 -0700 Comment on attachment 93072 Patch Ok. It's possible it might still be reachable from CSSStyleDeclaration. getPropertyCSSValue() somehow, but that's even more obscure. Sounds fine. 402658 10 93072 commit-queue 2011-05-11 22:13:39 -0700 Comment on attachment 93072 Patch Clearing flags on attachment: 93072 Committed r86308: <http://trac.webkit.org/changeset/86308> 402659 11 commit-queue 2011-05-11 22:13:44 -0700 All reviewed patches have been landed. Closing bug. 402710 12 webkit.review.bot 2011-05-11 23:59:59 -0700 http://trac.webkit.org/changeset/86308 might have broken Windows XP Debug (Tests) The following tests are not passing: editing/deleting/delete-ligature-001.html editing/deleting/delete-ligature-002.html editing/deleting/delete-ligature-003.html 93072 2011-05-10 22:22:19 -0700 2011-05-11 22:13:39 -0700 Patch bug-60609-20110511152216.patch text/plain 5141 macpherson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg2MjEwKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAKKzIwMTEtMDUtMTAgIEx1a2UgTWFj cGhlcnNvbiAgIDxtYWNwaGVyc29uQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbXBsZW1lbnQgQ1NTUHJvcGVydHlNYXJnaW4g YW5kIENTU1Byb3BlcnR5UGFkZGluZyBpbiBDU1NTdHlsZUFwcGx5UHJvcGVydHkuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MDYwOQorCisgICAgICAg IE5vIG5ldyB0ZXN0cyAtIHJlZmFjdG9yaW5nIG9ubHkuCisKKyAgICAgICAgKiBjc3MvQ1NTU3R5 bGVBcHBseVByb3BlcnR5LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNTU1N0eWxlQXBwbHlQcm9w ZXJ0eTo6Q1NTU3R5bGVBcHBseVByb3BlcnR5KToKKyAgICAgICAgSW5pdGlhbGlzZSBDU1NQcm9w ZXJ0eU1hcmdpbiBhbmQgQ1NTUHJvcGVydHlQYWRkaW5nIGhhbmRsZXJzLgorICAgICAgICAqIGNz cy9DU1NTdHlsZVNlbGVjdG9yLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNTU1N0eWxlU2VsZWN0 b3I6OmFwcGx5UHJvcGVydHkpOgorICAgICAgICBSZW1vdmUgdW51c2VkIGltcGxlbWVudGF0aW9u cy4KKwogMjAxMS0wNS0xMCAgSmFtZXMgUm9iaW5zb24gIDxqYW1lc3JAY2hyb21pdW0ub3JnPgog CiAgICAgICAgIFJldmlld2VkIGJ5IEtlbm5ldGggUnVzc2VsbC4KSW5kZXg6IFNvdXJjZS9XZWJD b3JlL2Nzcy9DU1NTdHlsZUFwcGx5UHJvcGVydHkuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJDb3JlL2Nzcy9DU1NTdHlsZUFwcGx5UHJvcGVydHkuY3BwCShyZXZpc2lvbiA4NjIwOSkKKysr IFNvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZUFwcGx5UHJvcGVydHkuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC01MDUsMTEgKzUwNSwxMyBAQCBDU1NTdHlsZUFwcGx5UHJvcGVydHk6OkNTU1N0eWxl QXBwbHlQcm9wCiAgICAgc2V0UHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5TWFyZ2luUmlnaHQs IG5ldyBBcHBseVByb3BlcnR5TGVuZ3RoPEF1dG9FbmFibGVkPigmUmVuZGVyU3R5bGU6Om1hcmdp blJpZ2h0LCAmUmVuZGVyU3R5bGU6OnNldE1hcmdpblJpZ2h0LCAmUmVuZGVyU3R5bGU6OmluaXRp YWxNYXJnaW4pKTsKICAgICBzZXRQcm9wZXJ0eUhhbmRsZXIoQ1NTUHJvcGVydHlNYXJnaW5Cb3R0 b20sIG5ldyBBcHBseVByb3BlcnR5TGVuZ3RoPEF1dG9FbmFibGVkPigmUmVuZGVyU3R5bGU6Om1h cmdpbkJvdHRvbSwgJlJlbmRlclN0eWxlOjpzZXRNYXJnaW5Cb3R0b20sICZSZW5kZXJTdHlsZTo6 aW5pdGlhbE1hcmdpbikpOwogICAgIHNldFByb3BlcnR5SGFuZGxlcihDU1NQcm9wZXJ0eU1hcmdp bkxlZnQsIG5ldyBBcHBseVByb3BlcnR5TGVuZ3RoPEF1dG9FbmFibGVkPigmUmVuZGVyU3R5bGU6 Om1hcmdpbkxlZnQsICZSZW5kZXJTdHlsZTo6c2V0TWFyZ2luTGVmdCwgJlJlbmRlclN0eWxlOjpp bml0aWFsTWFyZ2luKSk7CisgICAgc2V0UHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5TWFyZ2lu LCBuZXcgQXBwbHlQcm9wZXJ0eUV4cGFuZGluZ1N1cHByZXNzVmFsdWUocHJvcGVydHlIYW5kbGVy KENTU1Byb3BlcnR5TWFyZ2luVG9wKSwgcHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5TWFyZ2lu UmlnaHQpLCBwcm9wZXJ0eUhhbmRsZXIoQ1NTUHJvcGVydHlNYXJnaW5Cb3R0b20pLCBwcm9wZXJ0 eUhhbmRsZXIoQ1NTUHJvcGVydHlNYXJnaW5MZWZ0KSkpOwogCiAgICAgc2V0UHJvcGVydHlIYW5k bGVyKENTU1Byb3BlcnR5UGFkZGluZ1RvcCwgbmV3IEFwcGx5UHJvcGVydHlMZW5ndGg8PigmUmVu ZGVyU3R5bGU6OnBhZGRpbmdUb3AsICZSZW5kZXJTdHlsZTo6c2V0UGFkZGluZ1RvcCwgJlJlbmRl clN0eWxlOjppbml0aWFsUGFkZGluZykpOwogICAgIHNldFByb3BlcnR5SGFuZGxlcihDU1NQcm9w ZXJ0eVBhZGRpbmdSaWdodCwgbmV3IEFwcGx5UHJvcGVydHlMZW5ndGg8PigmUmVuZGVyU3R5bGU6 OnBhZGRpbmdSaWdodCwgJlJlbmRlclN0eWxlOjpzZXRQYWRkaW5nUmlnaHQsICZSZW5kZXJTdHls ZTo6aW5pdGlhbFBhZGRpbmcpKTsKICAgICBzZXRQcm9wZXJ0eUhhbmRsZXIoQ1NTUHJvcGVydHlQ YWRkaW5nQm90dG9tLCBuZXcgQXBwbHlQcm9wZXJ0eUxlbmd0aDw+KCZSZW5kZXJTdHlsZTo6cGFk ZGluZ0JvdHRvbSwgJlJlbmRlclN0eWxlOjpzZXRQYWRkaW5nQm90dG9tLCAmUmVuZGVyU3R5bGU6 OmluaXRpYWxQYWRkaW5nKSk7CiAgICAgc2V0UHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5UGFk ZGluZ0xlZnQsIG5ldyBBcHBseVByb3BlcnR5TGVuZ3RoPD4oJlJlbmRlclN0eWxlOjpwYWRkaW5n TGVmdCwgJlJlbmRlclN0eWxlOjpzZXRQYWRkaW5nTGVmdCwgJlJlbmRlclN0eWxlOjppbml0aWFs UGFkZGluZykpOworICAgIHNldFByb3BlcnR5SGFuZGxlcihDU1NQcm9wZXJ0eVBhZGRpbmcsIG5l dyBBcHBseVByb3BlcnR5RXhwYW5kaW5nU3VwcHJlc3NWYWx1ZShwcm9wZXJ0eUhhbmRsZXIoQ1NT UHJvcGVydHlQYWRkaW5nVG9wKSwgcHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5UGFkZGluZ1Jp Z2h0KSwgcHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5UGFkZGluZ0JvdHRvbSksIHByb3BlcnR5 SGFuZGxlcihDU1NQcm9wZXJ0eVBhZGRpbmdMZWZ0KSkpOwogCiAgICAgc2V0UHJvcGVydHlIYW5k bGVyKENTU1Byb3BlcnR5V2Via2l0UGVyc3BlY3RpdmVPcmlnaW5YLCBuZXcgQXBwbHlQcm9wZXJ0 eUxlbmd0aDw+KCZSZW5kZXJTdHlsZTo6cGVyc3BlY3RpdmVPcmlnaW5YLCAmUmVuZGVyU3R5bGU6 OnNldFBlcnNwZWN0aXZlT3JpZ2luWCwgJlJlbmRlclN0eWxlOjppbml0aWFsUGVyc3BlY3RpdmVP cmlnaW5YKSk7CiAgICAgc2V0UHJvcGVydHlIYW5kbGVyKENTU1Byb3BlcnR5V2Via2l0UGVyc3Bl Y3RpdmVPcmlnaW5ZLCBuZXcgQXBwbHlQcm9wZXJ0eUxlbmd0aDw+KCZSZW5kZXJTdHlsZTo6cGVy c3BlY3RpdmVPcmlnaW5ZLCAmUmVuZGVyU3R5bGU6OnNldFBlcnNwZWN0aXZlT3JpZ2luWSwgJlJl bmRlclN0eWxlOjppbml0aWFsUGVyc3BlY3RpdmVPcmlnaW5ZKSk7CkluZGV4OiBTb3VyY2UvV2Vi Q29yZS9jc3MvQ1NTU3R5bGVTZWxlY3Rvci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvY3NzL0NTU1N0eWxlU2VsZWN0b3IuY3BwCShyZXZpc2lvbiA4NjIwOSkKKysrIFNvdXJjZS9X ZWJDb3JlL2Nzcy9DU1NTdHlsZVNlbGVjdG9yLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDQ0MSwy NiArNDQ0MSw2IEBAIHZvaWQgQ1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eShpbnQKICAg ICAgICAgZWxzZSBpZiAoaXNJbmhlcml0KQogICAgICAgICAgICAgbV9zdHlsZS0+aW5oZXJpdE1h c2tMYXllcnMoKm1fcGFyZW50U3R5bGUtPm1hc2tMYXllcnMoKSk7CiAgICAgICAgIHJldHVybjsK LSAgICBjYXNlIENTU1Byb3BlcnR5TWFyZ2luOgotICAgICAgICBpZiAoaXNJbmhlcml0KSB7Ci0g ICAgICAgICAgICBtX3N0eWxlLT5zZXRNYXJnaW5Ub3AobV9wYXJlbnRTdHlsZS0+bWFyZ2luVG9w KCkpOwotICAgICAgICAgICAgbV9zdHlsZS0+c2V0TWFyZ2luQm90dG9tKG1fcGFyZW50U3R5bGUt Pm1hcmdpbkJvdHRvbSgpKTsKLSAgICAgICAgICAgIG1fc3R5bGUtPnNldE1hcmdpbkxlZnQobV9w YXJlbnRTdHlsZS0+bWFyZ2luTGVmdCgpKTsKLSAgICAgICAgICAgIG1fc3R5bGUtPnNldE1hcmdp blJpZ2h0KG1fcGFyZW50U3R5bGUtPm1hcmdpblJpZ2h0KCkpOwotICAgICAgICB9Ci0gICAgICAg IGVsc2UgaWYgKGlzSW5pdGlhbCkKLSAgICAgICAgICAgIG1fc3R5bGUtPnJlc2V0TWFyZ2luKCk7 Ci0gICAgICAgIHJldHVybjsKLSAgICBjYXNlIENTU1Byb3BlcnR5UGFkZGluZzoKLSAgICAgICAg aWYgKGlzSW5oZXJpdCkgewotICAgICAgICAgICAgbV9zdHlsZS0+c2V0UGFkZGluZ1RvcChtX3Bh cmVudFN0eWxlLT5wYWRkaW5nVG9wKCkpOwotICAgICAgICAgICAgbV9zdHlsZS0+c2V0UGFkZGlu Z0JvdHRvbShtX3BhcmVudFN0eWxlLT5wYWRkaW5nQm90dG9tKCkpOwotICAgICAgICAgICAgbV9z dHlsZS0+c2V0UGFkZGluZ0xlZnQobV9wYXJlbnRTdHlsZS0+cGFkZGluZ0xlZnQoKSk7Ci0gICAg ICAgICAgICBtX3N0eWxlLT5zZXRQYWRkaW5nUmlnaHQobV9wYXJlbnRTdHlsZS0+cGFkZGluZ1Jp Z2h0KCkpOwotICAgICAgICB9Ci0gICAgICAgIGVsc2UgaWYgKGlzSW5pdGlhbCkKLSAgICAgICAg ICAgIG1fc3R5bGUtPnJlc2V0UGFkZGluZygpOwotICAgICAgICByZXR1cm47CiAgICAgY2FzZSBD U1NQcm9wZXJ0eUZvbnQ6CiAgICAgICAgIGlmIChpc0luaGVyaXQpIHsKICAgICAgICAgICAgIEZv bnREZXNjcmlwdGlvbiBmb250RGVzY3JpcHRpb24gPSBtX3BhcmVudFN0eWxlLT5mb250RGVzY3Jp cHRpb24oKTsKQEAgLTU1MzgsMTAgKzU1MTgsMTIgQEAgdm9pZCBDU1NTdHlsZVNlbGVjdG9yOjph cHBseVByb3BlcnR5KGludAogICAgIGNhc2UgQ1NTUHJvcGVydHlNYXJnaW5SaWdodDoKICAgICBj YXNlIENTU1Byb3BlcnR5TWFyZ2luQm90dG9tOgogICAgIGNhc2UgQ1NTUHJvcGVydHlNYXJnaW5M ZWZ0OgorICAgIGNhc2UgQ1NTUHJvcGVydHlNYXJnaW46CiAgICAgY2FzZSBDU1NQcm9wZXJ0eVBh ZGRpbmdUb3A6CiAgICAgY2FzZSBDU1NQcm9wZXJ0eVBhZGRpbmdSaWdodDoKICAgICBjYXNlIENT U1Byb3BlcnR5UGFkZGluZ0JvdHRvbToKICAgICBjYXNlIENTU1Byb3BlcnR5UGFkZGluZ0xlZnQ6 CisgICAgY2FzZSBDU1NQcm9wZXJ0eVBhZGRpbmc6CiAgICAgY2FzZSBDU1NQcm9wZXJ0eVRleHRJ bmRlbnQ6CiAgICAgY2FzZSBDU1NQcm9wZXJ0eU1heEhlaWdodDoKICAgICBjYXNlIENTU1Byb3Bl cnR5SGVpZ2h0Ogo=