51390 2010-12-21 06:11:11 -0800 Web Inspector: After scrolling new property value and cancelling, removed property remains active in page style 2010-12-23 03:12:33 -0800 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 apavlov apavlov apavlov bweinstein commit-queue joepeck keishi loislo pfeldman pmuellr rik timothy yurys oldest_to_newest 324974 0 apavlov 2010-12-21 06:11:11 -0800 1. 324977 1 apavlov 2010-12-21 06:14:19 -0800 Somehow the blank report got committed. 1. In any page, add a numeric property, say, "height" for the "body" element 2. Type in 20% and press Up or Down to have the value inc/decremented. 3. Hit Esc to cancel the property addition Observe that the effect of the property (e.g. height: 21%) remains active in the page. 325023 2 77119 apavlov 2010-12-21 08:38:59 -0800 Created attachment 77119 [PATCH] Suggested fix 325060 3 77119 joepeck 2010-12-21 10:27:57 -0800 Comment on attachment 77119 [PATCH] Suggested fix View in context: https://bugs.webkit.org/attachment.cgi?id=77119&action=review > WebCore/inspector/front-end/StylesSidebarPane.js:1857 > + if (!styleTextLength && updateInterface && this._newProperty && !("originalPropertyText" in this)) { How can something be in the state where this._newProperty is true and it has "originalPropertyText"? 325063 4 apavlov 2010-12-21 10:31:47 -0800 (In reply to comment #3) > (From update of attachment 77119 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77119&action=review > > > WebCore/inspector/front-end/StylesSidebarPane.js:1857 > > + if (!styleTextLength && updateInterface && this._newProperty && !("originalPropertyText" in this)) { > > How can something be in the state where this._newProperty is true and it has "originalPropertyText"? It happens exactly after the steps 1-3 are executed. In this case, originalPropertyText === "" (it is remembered after the Up or Down keys are hit to increment/decrement the property value). Setting the property text back to "" effectively removes the property from the style. 325105 5 joepeck 2010-12-21 11:32:46 -0800 I see, you explained this to me on IRC: • "originalPropertyText" • Before patch: Up/Down + Esc = the property gets removed from UI (sidebar) but kept in CSSStyleDeclarations (page) • After patch: Up/Down + Esc = the property gets removed both from UI (sidebar) and CSSStyleDeclarations (page) That sounds good to me. For the sake of understanding this, moving the ("originalPropertyText" in this) into a helper function under a more descriptive name. Something like: _hasBeenAppliedToPageViaUpDown: { return ("originalPropertyText" in this); } In doing so, the reading becomes: if (!styleTextLength && updateInterface && this._newProperty && !this._hasBeenAppliedToPageViaUpDown()) { // Delete from UI (sidebar) } So the code inside this block doesn't run! Now I see what you mean when you said: Setting the property text back to "" effectively removes the property from the style. That is the part I wasn't seeing. I think that part should be commented // New properties applied via up/down have an originalPropertyText and will be deleted later // on, if cancelled, when the empty string gets applied as their style text. If that sounds correct then r=me. I applied the patch and it worked nicely. However, up/down seemed broken in other ways. I think I just need to update for some recent fixes. 325107 6 joepeck 2010-12-21 11:33:19 -0800 > • "originalPropertyText" is used with up/down only, to save the original value 325463 7 77210 apavlov 2010-12-22 05:42:20 -0800 Created attachment 77210 [PATCH] Comments addressed 325482 8 apavlov 2010-12-22 06:47:07 -0800 (In reply to comment #5) > I see, you explained this to me on IRC: > ... > If that sounds correct then r=me. I applied the patch and it worked nicely. However, up/down > seemed broken in other ways. I think I just need to update for some recent fixes. Yes, all your statements are valid, I have fixed the nits in the latest patch. Just in case, can you say how up/down is broken for you? Some changes around it are coming in the CSS value autocompletion (bug 41414). 325535 9 joepeck 2010-12-22 10:01:47 -0800 (In reply to comment #8) > Just in case, can you say how up/down is broken for you? Some changes around > it are coming in the CSS value autocompletion (bug 41414). I just tested with a fresh ToT build (and with your patch applied). Things were working better then when I made my comment. Dabbling with the inline style attribute I found some issues: <http://webkit.org/b/51477> Web Inspector: ASSERT removing inline HTML style property <http://webkit.org/b/51478> Web Inspector: Inline HTML style property out of sync with element.style in Sidebar 325536 10 77210 joepeck 2010-12-22 10:03:45 -0800 Comment on attachment 77210 [PATCH] Comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=77210&action=review > WebCore/inspector/front-end/StylesSidebarPane.js:1854 > + // New properties applied via up/down have an originalPropertyText and will be deleted later > + // on, if cancelled, when the empty string gets applied as their style text. This comment would be better suited down below, in `applyStyleText`, next to the usage of the _hasBeenAppliedToPageViaUpDown function. 325619 11 77210 apavlov 2010-12-22 12:02:17 -0800 Comment on attachment 77210 [PATCH] Comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=77210&action=review >> WebCore/inspector/front-end/StylesSidebarPane.js:1854 >> + // on, if cancelled, when the empty string gets applied as their style text. > > This comment would be better suited down below, in `applyStyleText`, next to the usage of > the _hasBeenAppliedToPageViaUpDown function. There is a very similar comment down below (which had been there for ages, I just augmented it), so I decided to keep all things clear :) 326005 12 77210 apavlov 2010-12-23 02:55:06 -0800 Comment on attachment 77210 [PATCH] Comments addressed Landing as is, since there is a very similar comment in the snippet you are pointing to. Please speak up if you think both comments should be changed, and I can drive-by fix them in one of subsequent changes (you submitted a good many of bugs yesterday :)) 326008 13 77210 commit-queue 2010-12-23 03:12:26 -0800 Comment on attachment 77210 [PATCH] Comments addressed Clearing flags on attachment: 77210 Committed r74548: <http://trac.webkit.org/changeset/74548> 326009 14 commit-queue 2010-12-23 03:12:33 -0800 All reviewed patches have been landed. Closing bug. 77119 2010-12-21 08:38:59 -0800 2010-12-22 05:42:20 -0800 [PATCH] Suggested fix scrollcancel.patch text/plain 1839 apavlov Y29tbWl0IDZmYjI2ZTQzNWUyMWZkM2MzYzYwMDRlNDY4ZjcwMDgyNmM4YmE3MGEKQXV0aG9yOiBB bGV4YW5kZXIgUGF2bG92IDxhcGF2bG92QGNocm9taXVtLm9yZz4KRGF0ZTogICBUdWUgRGVjIDIx IDE5OjMzOjE4IDIwMTAgKzAzMDAKCiAgICBSZXZlcnRQcm9wZXJ0eUFmdGVyU2Nyb2xsCgpkaWZm IC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBkYmMz MGJmLi41YzcyYmFlIDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3Jl L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDEwLTEyLTIxICBBbGV4YW5kZXIgUGF2bG92 ICA8YXBhdmxvdkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogQWZ0ZXIgc2Nyb2xsaW5nIG5ldyBwcm9w ZXJ0eSB2YWx1ZSBhbmQgY2FuY2VsbGluZywgcmVtb3ZlZCBwcm9wZXJ0eSByZW1haW5zIGFjdGl2 ZSBpbiBwYWdlIHN0eWxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD01MTM5MAorCisgICAgICAgICogaW5zcGVjdG9yL2Zyb250LWVuZC9TdHlsZXNTaWRl YmFyUGFuZS5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5TdHlsZVByb3BlcnR5VHJlZUVsZW1l bnQucHJvdG90eXBlKToKKwogMjAxMC0xMi0yMSAgTWFoZXNoIEt1bGthcm5pICA8bWFoZXNoLmt1 bGthcm5pQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbmRyZWFzIEtsaW5nLgpk aWZmIC0tZ2l0IGEvV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL1N0eWxlc1NpZGViYXJQYW5l LmpzIGIvV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL1N0eWxlc1NpZGViYXJQYW5lLmpzCmlu ZGV4IGM4OWU0NTIuLmU3ZDRmMjYgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvaW5zcGVjdG9yL2Zyb250 LWVuZC9TdHlsZXNTaWRlYmFyUGFuZS5qcworKysgYi9XZWJDb3JlL2luc3BlY3Rvci9mcm9udC1l bmQvU3R5bGVzU2lkZWJhclBhbmUuanMKQEAgLTE4NTQsOCArMTg1NCw4IEBAIFdlYkluc3BlY3Rv ci5TdHlsZVByb3BlcnR5VHJlZUVsZW1lbnQucHJvdG90eXBlID0gewogICAgICAgICB2YXIgZWxl bWVudHNQYW5lbCA9IFdlYkluc3BlY3Rvci5wYW5lbHMuZWxlbWVudHM7CiAgICAgICAgIHN0eWxl VGV4dCA9IHN0eWxlVGV4dC5yZXBsYWNlKC9ccy9nLCAiICIpLnRyaW0oKTsgLy8gUmVwbGFjZSAm bmJzcDsgd2l0aCB3aGl0ZXNwYWNlLgogICAgICAgICB2YXIgc3R5bGVUZXh0TGVuZ3RoID0gc3R5 bGVUZXh0Lmxlbmd0aDsKLSAgICAgICAgaWYgKCFzdHlsZVRleHRMZW5ndGggJiYgdXBkYXRlSW50 ZXJmYWNlICYmIHRoaXMuX25ld1Byb3BlcnR5KSB7Ci0gICAgICAgICAgICAvLyBUaGUgdXNlciBk ZWxldGVkIGV2ZXJ5dGhpbmcsIHNvIHJlbW92ZSB0aGUgdHJlZSBlbGVtZW50IGFuZCB1cGRhdGUu CisgICAgICAgIGlmICghc3R5bGVUZXh0TGVuZ3RoICYmIHVwZGF0ZUludGVyZmFjZSAmJiB0aGlz Ll9uZXdQcm9wZXJ0eSAmJiAhKCJvcmlnaW5hbFByb3BlcnR5VGV4dCIgaW4gdGhpcykpIHsKKyAg ICAgICAgICAgIC8vIFRoZSB1c2VyIGRlbGV0ZWQgZXZlcnl0aGluZyBhbmQgbmV2ZXIgYXBwbGll ZCBhIG5ldyBwcm9wZXJ0eSB2YWx1ZSB2aWEgVXAvRG93biBzY3JvbGxpbmcsIHNvIHJlbW92ZSB0 aGUgdHJlZSBlbGVtZW50IGFuZCB1cGRhdGUuCiAgICAgICAgICAgICB0aGlzLnBhcmVudC5yZW1v dmVDaGlsZCh0aGlzKTsKICAgICAgICAgICAgIHNlY3Rpb24uYWZ0ZXJVcGRhdGUoKTsKICAgICAg ICAgICAgIHJldHVybjsK 77210 2010-12-22 05:42:20 -0800 2010-12-23 03:12:25 -0800 [PATCH] Comments addressed scrollcancel.patch text/plain 2800 apavlov Y29tbWl0IGYzNmI1ZjllZjI0OTdlNWQ1MGE2NWUxYzg3NjBhYThjODMyNDZiNzEKQXV0aG9yOiBB bGV4YW5kZXIgUGF2bG92IDxhcGF2bG92QGNocm9taXVtLm9yZz4KRGF0ZTogICBUdWUgRGVjIDIx IDE5OjMzOjE4IDIwMTAgKzAzMDAKCiAgICBSZXZlcnRQcm9wZXJ0eUFmdGVyU2Nyb2xsCgpkaWZm IC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA1MDJi ZWMyLi42NzM5YzQ5IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3Jl L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDEwLTEyLTIyICBBbGV4YW5kZXIgUGF2bG92 ICA8YXBhdmxvdkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogQWZ0ZXIgc2Nyb2xsaW5nIG5ldyBwcm9w ZXJ0eSB2YWx1ZSBhbmQgY2FuY2VsbGluZywgcmVtb3ZlZCBwcm9wZXJ0eSByZW1haW5zIGFjdGl2 ZSBpbiBwYWdlIHN0eWxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD01MTM5MAorCisgICAgICAgICogaW5zcGVjdG9yL2Zyb250LWVuZC9TdHlsZXNTaWRl YmFyUGFuZS5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5TdHlsZVByb3BlcnR5VHJlZUVsZW1l bnQucHJvdG90eXBlKToKKwogMjAxMC0xMi0yMiAgRGlyayBTY2h1bHplICA8a3JpdEB3YmVraXQu b3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IE5pa29sYXMgWmltbWVybWFubi4KZGlmZiAtLWdp dCBhL1dlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9TdHlsZXNTaWRlYmFyUGFuZS5qcyBiL1dl YkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9TdHlsZXNTaWRlYmFyUGFuZS5qcwppbmRleCBjODll NDUyLi44YzUwNDM2IDEwMDY0NAotLS0gYS9XZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvU3R5 bGVzU2lkZWJhclBhbmUuanMKKysrIGIvV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL1N0eWxl c1NpZGViYXJQYW5lLmpzCkBAIC0xNzE0LDggKzE3MTQsOCBAQCBXZWJJbnNwZWN0b3IuU3R5bGVQ cm9wZXJ0eVRyZWVFbGVtZW50LnByb3RvdHlwZSA9IHsKICAgICAgICAgZXZlbnQucHJldmVudERl ZmF1bHQoKTsKIAogICAgICAgICBpZiAoISgib3JpZ2luYWxQcm9wZXJ0eVRleHQiIGluIHRoaXMp KSB7Ci0gICAgICAgICAgICAvLyBSZW1lbWJlciB0aGUgcnVsZSdzIG9yaWdpbmFsIENTUyB0ZXh0 LCBzbyBpdCBjYW4gYmUgcmVzdG9yZWQKLSAgICAgICAgICAgIC8vIGlmIHRoZSBlZGl0aW5nIGlz IGNhbmNlbGVkIGFuZCBiZWZvcmUgZWFjaCBhcHBseS4KKyAgICAgICAgICAgIC8vIFJlbWVtYmVy IHRoZSBydWxlJ3Mgb3JpZ2luYWwgQ1NTIHRleHQgb24gW1BhZ2VdKFVwfERvd24pLCBzbyBpdCBj YW4gYmUgcmVzdG9yZWQKKyAgICAgICAgICAgIC8vIGlmIHRoZSBlZGl0aW5nIGlzIGNhbmNlbGVk LgogICAgICAgICAgICAgdGhpcy5vcmlnaW5hbFByb3BlcnR5VGV4dCA9IHRoaXMucHJvcGVydHku cHJvcGVydHlUZXh0OwogICAgICAgICB9CiAKQEAgLTE4NDgsMTQgKzE4NDgsMjEgQEAgV2ViSW5z cGVjdG9yLlN0eWxlUHJvcGVydHlUcmVlRWxlbWVudC5wcm90b3R5cGUgPSB7CiAgICAgICAgIH0K ICAgICB9LAogCisgICAgX2hhc0JlZW5BcHBsaWVkVG9QYWdlVmlhVXBEb3duOiBmdW5jdGlvbigp CisgICAgeworICAgICAgICAvLyBOZXcgcHJvcGVydGllcyBhcHBsaWVkIHZpYSB1cC9kb3duIGhh dmUgYW4gb3JpZ2luYWxQcm9wZXJ0eVRleHQgYW5kIHdpbGwgYmUgZGVsZXRlZCBsYXRlcgorICAg ICAgICAvLyBvbiwgaWYgY2FuY2VsbGVkLCB3aGVuIHRoZSBlbXB0eSBzdHJpbmcgZ2V0cyBhcHBs aWVkIGFzIHRoZWlyIHN0eWxlIHRleHQuCisgICAgICAgIHJldHVybiAoIm9yaWdpbmFsUHJvcGVy dHlUZXh0IiBpbiB0aGlzKTsKKyAgICB9LAorCiAgICAgYXBwbHlTdHlsZVRleHQ6IGZ1bmN0aW9u KHN0eWxlVGV4dCwgdXBkYXRlSW50ZXJmYWNlKQogICAgIHsKICAgICAgICAgdmFyIHNlY3Rpb24g PSB0aGlzLnRyZWVPdXRsaW5lLnNlY3Rpb247CiAgICAgICAgIHZhciBlbGVtZW50c1BhbmVsID0g V2ViSW5zcGVjdG9yLnBhbmVscy5lbGVtZW50czsKICAgICAgICAgc3R5bGVUZXh0ID0gc3R5bGVU ZXh0LnJlcGxhY2UoL1xzL2csICIgIikudHJpbSgpOyAvLyBSZXBsYWNlICZuYnNwOyB3aXRoIHdo aXRlc3BhY2UuCiAgICAgICAgIHZhciBzdHlsZVRleHRMZW5ndGggPSBzdHlsZVRleHQubGVuZ3Ro OwotICAgICAgICBpZiAoIXN0eWxlVGV4dExlbmd0aCAmJiB1cGRhdGVJbnRlcmZhY2UgJiYgdGhp cy5fbmV3UHJvcGVydHkpIHsKLSAgICAgICAgICAgIC8vIFRoZSB1c2VyIGRlbGV0ZWQgZXZlcnl0 aGluZywgc28gcmVtb3ZlIHRoZSB0cmVlIGVsZW1lbnQgYW5kIHVwZGF0ZS4KKyAgICAgICAgaWYg KCFzdHlsZVRleHRMZW5ndGggJiYgdXBkYXRlSW50ZXJmYWNlICYmIHRoaXMuX25ld1Byb3BlcnR5 ICYmICF0aGlzLl9oYXNCZWVuQXBwbGllZFRvUGFnZVZpYVVwRG93bigpKSB7CisgICAgICAgICAg ICAvLyBUaGUgdXNlciBkZWxldGVkIGV2ZXJ5dGhpbmcgYW5kIG5ldmVyIGFwcGxpZWQgYSBuZXcg cHJvcGVydHkgdmFsdWUgdmlhIFVwL0Rvd24gc2Nyb2xsaW5nLCBzbyByZW1vdmUgdGhlIHRyZWUg ZWxlbWVudCBhbmQgdXBkYXRlLgogICAgICAgICAgICAgdGhpcy5wYXJlbnQucmVtb3ZlQ2hpbGQo dGhpcyk7CiAgICAgICAgICAgICBzZWN0aW9uLmFmdGVyVXBkYXRlKCk7CiAgICAgICAgICAgICBy ZXR1cm47Cg==