101682 2012-11-08 18:24:52 -0800 RemoveFormat command doesn't remove background color 2012-11-09 13:19:57 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa darin enrica mifenton ojan tony webkit-bug-importer webkit.review.bot oldest_to_newest 762406 0 rniwa 2012-11-08 18:24:52 -0800 Steps to reproduce the problem: 1. create js node 'some red text here' element 2. call execCommand with removeFormat on the created node What is the expected behavior? 'some red text here' ie: Background colour is removed What went wrong? 'some red text here' ie: Background colour is not removed http://code.google.com/p/chromium/issues/detail?id=159407 762407 1 webkit-bug-importer 2012-11-08 18:25:10 -0800 <rdar://problem/12668350> 762527 2 173196 rniwa 2012-11-08 21:25:24 -0800 Created attachment 173196 Fixes the bug 763108 3 173196 tony 2012-11-09 09:52:27 -0800 Comment on attachment 173196 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=173196&action=review > LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3 > +This tests removing format on text that has background color. There should be no span or inline style below: > + > +hello world WebKit. Should we use something other than a dumpAsText test? I think a script test that uses getComputedStyle or a reftest would be better for checking the background color. 763194 4 rniwa 2012-11-09 11:33:23 -0800 (In reply to comment #3) > (From update of attachment 173196 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173196&action=review > > > LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3 > > +This tests removing format on text that has background color. There should be no span or inline style below: > > + > > +hello world WebKit. > > Should we use something other than a dumpAsText test? I think a script test that uses getComputedStyle or a reftest would be better for checking the background color. I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible. 763211 5 173196 tony 2012-11-09 11:49:54 -0800 Comment on attachment 173196 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=173196&action=review > Source/WebCore/editing/RemoveFormatCommand.cpp:92 > + defaultStyle->style()->setProperty(CSSPropertyBackgroundColor, "transparent"); Can we pass in CSSValueTransparent instead of "transparent"? Looks like that would avoid having to run the CSS parser. >>> LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3 >>> +hello world WebKit. >> >> Should we use something other than a dumpAsText test? I think a script test that uses getComputedStyle or a reftest would be better for checking the background color. > > I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible. Ah, I see. Using dump-as-markup.js would be more clear. Should there be a test case with background-color: transparent? 763216 6 173196 rniwa 2012-11-09 11:55:54 -0800 Comment on attachment 173196 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=173196&action=review >> Source/WebCore/editing/RemoveFormatCommand.cpp:92 >> + defaultStyle->style()->setProperty(CSSPropertyBackgroundColor, "transparent"); > > Can we pass in CSSValueTransparent instead of "transparent"? Looks like that would avoid having to run the CSS parser. Oh, you’re right. We’re talking to StylePropertySet here. >>>> LayoutTests/editing/execCommand/remove-format-background-color-expected.txt:3 >>>> +hello world WebKit. >>> >>> Should we use something other than a dumpAsText test? I think a script test that uses getComputedStyle or a reftest would be better for checking the background color. >> >> I’m dumping HTML here. We don’t want to leave a span. It’s important the markup we generate is as concise as possible. > > Ah, I see. Using dump-as-markup.js would be more clear. Should there be a test case with background-color: transparent? No… I think we should be removing background-color: transparent as well in the future. We just don’t have a way to tell ApplyStyleCommand that it should remove a property regardless of its value. 763223 7 rniwa 2012-11-09 12:10:50 -0800 Committed r134096: <http://trac.webkit.org/changeset/134096> 763263 8 tony 2012-11-09 13:19:57 -0800 (In reply to comment #6) > > Should there be a test case with background-color: transparent? > > No… I think we should be removing background-color: transparent as well in the future. We just don’t have a way to tell ApplyStyleCommand that it should remove a property regardless of its value. That's fine, you could check in a failing test case for that. It would make it clear what the desired result is. 173196 2012-11-08 21:25:24 -0800 2012-11-09 11:55:54 -0800 Fixes the bug bug-101682-20121108212332.patch text/plain 3748 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNDAwNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEyLTExLTA4ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFJlbW92ZUZvcm1hdCBjb21tYW5k IGRvZXNuJ3QgcmVtb3ZlIGJhY2tncm91bmQgY29sb3IKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMTY4MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeGVkIHRoZSBidWcgYnkgcmVtb3ZpbmcgYWxsIG5v bi10cmFuc3BhcmVudCBiYWNrZ3JvdW5kLWNvbG9yIHByb3BlcnRpZXMuCisKKyAgICAgICAgVGVz dDogZWRpdGluZy9leGVjQ29tbWFuZC9yZW1vdmUtZm9ybWF0LWJhY2tncm91bmQtY29sb3IuaHRt bAorCisgICAgICAgICogZWRpdGluZy9SZW1vdmVGb3JtYXRDb21tYW5kLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbW92ZUZvcm1hdENvbW1hbmQ6OmRvQXBwbHkpOgorCiAyMDEyLTExLTA4ICBS b2JlcnQgU2VzZWsgIDxyc2VzZWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEd1YXJkIGNhbGxz IHRvIFdlYktpdFN5c3RlbUludGVyZmFjZSBtZWRpYSBjb250cm9sIGRyYXdpbmcgZnVuY3Rpb25z IGluIFJlbmRlclRoZW1lTWFjIHdpdGggUExBVEZPUk0oTUFDKQpJbmRleDogU291cmNlL1dlYkNv cmUvZWRpdGluZy9SZW1vdmVGb3JtYXRDb21tYW5kLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9lZGl0aW5nL1JlbW92ZUZvcm1hdENvbW1hbmQuY3BwCShyZXZpc2lvbiAxMzM5NTYp CisrKyBTb3VyY2UvV2ViQ29yZS9lZGl0aW5nL1JlbW92ZUZvcm1hdENvbW1hbmQuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC04Nyw2ICs4NywxMCBAQCB2b2lkIFJlbW92ZUZvcm1hdENvbW1hbmQ6OmRv QXBwbHkoKQogICAgIE5vZGUqIHJvb3QgPSBmcmFtZS0+c2VsZWN0aW9uKCktPnJvb3RFZGl0YWJs ZUVsZW1lbnQoKTsKICAgICBSZWZQdHI8RWRpdGluZ1N0eWxlPiBkZWZhdWx0U3R5bGUgPSBFZGl0 aW5nU3R5bGU6OmNyZWF0ZShyb290KTsKIAorICAgIC8vIFdlIHdhbnQgdG8gcmVtb3ZlIGV2ZXJ5 dGhpbmcgYnV0IHRyYW5zcGFyZW50IGJhY2tncm91bmQuCisgICAgLy8gRklYTUU6IFdlIHNob3Vs ZG4ndCBhY2Nlc3Mgc3R5bGUoKS4KKyAgICBkZWZhdWx0U3R5bGUtPnN0eWxlKCktPnNldFByb3Bl cnR5KENTU1Byb3BlcnR5QmFja2dyb3VuZENvbG9yLCAidHJhbnNwYXJlbnQiKTsKKwogICAgIGFw cGx5Q29tbWFuZFRvQ29tcG9zaXRlKEFwcGx5U3R5bGVDb21tYW5kOjpjcmVhdGUoZG9jdW1lbnQo KSwgZGVmYXVsdFN0eWxlLmdldCgpLCBpc0VsZW1lbnRGb3JSZW1vdmVGb3JtYXRDb21tYW5kLCBl ZGl0aW5nQWN0aW9uKCkpKTsKIH0KIApJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2aXNpb24gMTM0MDA0KQorKysgTGF5 b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTIt MTEtMDggIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmVtb3Zl Rm9ybWF0IGNvbW1hbmQgZG9lc24ndCByZW1vdmUgYmFja2dyb3VuZCBjb2xvcgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAxNjgyCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkZWQgYSByZWdyZXNzaW9u IHRlc3QuCisKKyAgICAgICAgKiBlZGl0aW5nL2V4ZWNDb21tYW5kL3JlbW92ZS1mb3JtYXQtYmFj a2dyb3VuZC1jb2xvci1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGVkaXRpbmcvZXhl Y0NvbW1hbmQvcmVtb3ZlLWZvcm1hdC1iYWNrZ3JvdW5kLWNvbG9yLmh0bWw6IEFkZGVkLgorCiAy MDEyLTExLTA4ICBIYXlhdG8gSXRvICA8aGF5YXRvQGNocm9taXVtLm9yZz4KIAogICAgICAgICBV bnJldmlld2VkLCB1cGRhdGUgY2hyb21pdW0gZXhwZWN0YXRpb25zIGZvciB0YWJsZXMvLgpJbmRl eDogTGF5b3V0VGVzdHMvZWRpdGluZy9leGVjQ29tbWFuZC9yZW1vdmUtZm9ybWF0LWJhY2tncm91 bmQtY29sb3ItZXhwZWN0ZWQudHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2VkaXRpbmcv ZXhlY0NvbW1hbmQvcmVtb3ZlLWZvcm1hdC1iYWNrZ3JvdW5kLWNvbG9yLWV4cGVjdGVkLnR4dAko cmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2VkaXRpbmcvZXhlY0NvbW1hbmQvcmVtb3ZlLWZv cm1hdC1iYWNrZ3JvdW5kLWNvbG9yLWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKQEAgLTAsMCAr MSwzIEBACitUaGlzIHRlc3RzIHJlbW92aW5nIGZvcm1hdCBvbiB0ZXh0IHRoYXQgaGFzIGJhY2tn cm91bmQgY29sb3IuIFRoZXJlIHNob3VsZCBiZSBubyBzcGFuIG9yIGlubGluZSBzdHlsZSBiZWxv dzoKKworaGVsbG8gd29ybGQgV2ViS2l0LgpJbmRleDogTGF5b3V0VGVzdHMvZWRpdGluZy9leGVj Q29tbWFuZC9yZW1vdmUtZm9ybWF0LWJhY2tncm91bmQtY29sb3IuaHRtbAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBMYXlvdXRUZXN0cy9lZGl0aW5nL2V4ZWNDb21tYW5kL3JlbW92ZS1mb3JtYXQtYmFja2dyb3Vu ZC1jb2xvci5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZWRpdGluZy9leGVjQ29t bWFuZC9yZW1vdmUtZm9ybWF0LWJhY2tncm91bmQtY29sb3IuaHRtbAkocmV2aXNpb24gMCkKQEAg LTAsMCArMSwxOSBAQAorPCFET0NUWVBFIGh0bWw+Cis8aHRtbD4KKzxib2R5PgorPHA+VGhpcyB0 ZXN0cyByZW1vdmluZyBmb3JtYXQgb24gdGV4dCB0aGF0IGhhcyBiYWNrZ3JvdW5kIGNvbG9yLiBU aGVyZSBzaG91bGQgYmUgbm8gc3BhbiBvciBpbmxpbmUgc3R5bGUgYmVsb3c6PC9wPgorPGRpdiBp ZD0iZWRpdG9yIiBjb250ZW50ZWRpdGFibGU+aGVsbG8gPHNwYW4gc3R5bGU9ImNvbG9yOiBibGFj azsgYmFja2dyb3VuZC1jb2xvcjogI2ZmMDAwMDsiPndvcmxkPC9zcGFuPiBXZWJLaXQuPC9kaXY+ Cis8c2NyaXB0IHR5cGU9InRleHQvamF2YXNjcmlwdCI+CisKK2lmICh3aW5kb3cudGVzdFJ1bm5l cikKKyAgICB0ZXN0UnVubmVyLmR1bXBBc1RleHQoKTsKKwordmFyIGVkaXRvciA9IGRvY3VtZW50 LmdldEVsZW1lbnRCeUlkKCdlZGl0b3InKTsKK2VkaXRvci5mb2N1cygpOworZG9jdW1lbnQuZXhl Y0NvbW1hbmQoJ1NlbGVjdEFsbCcsIGZhbHNlLCBudWxsKTsKK2RvY3VtZW50LmV4ZWNDb21tYW5k KCdSZW1vdmVGb3JtYXQnLCBmYWxzZSwgbnVsbCk7CitlZGl0b3IuaW5uZXJUZXh0ID0gZWRpdG9y LmlubmVySFRNTDsKKworPC9zY3JpcHQ+Cis8L2JvZHk+Cis8L2h0bWw+Cg==