171902 2017-05-09 18:33:34 -0700 REGRESSION (r?): Web Inspector: Shift-click on color square in Styles sidebar should not select text 2017-05-18 15:33:35 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED EasyFix, GoodFirstBug P2 Normal --- 1 nvasilyev hi commit-queue hi inspector-bugzilla-changes mattbaker oldest_to_newest 1306700 0 nvasilyev 2017-05-09 18:33:34 -0700 Steps: 1. Inspect any element. 2. Add "color: brown" rule. 3. Click twice on the color square before "brown" while holding Shift key. Expected: Text selection shouldn't change. 1309938 1 310468 hi 2017-05-17 17:29:44 -0700 Created attachment 310468 Patch 1310241 2 310468 mattbaker 2017-05-18 13:50:00 -0700 Comment on attachment 310468 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310468&action=review r=me, with some notes. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-455 > - if (this._mouseDownCursorPosition.line === cursor.line && this._mouseDownCursorPosition.ch === cursor.ch) { Hmm, it would be nice if we could keep this check and return early if it fails. This would improve the readability of the function and avoid doing the work to determine `clickedBookmark` when not needed. To do this would require that `this._mouseDownCursorPosition = null` happen first, instead of at the end of the function. A local `mouseDownCursorPosition` would be needed since the rest of the function uses the value. I'll leave it up to you. If you think the function is cleanest as-is, I'm fine with it. 1310282 3 310468 hi 2017-05-18 15:05:16 -0700 Comment on attachment 310468 Patch (In reply to Matt Baker from comment #2) > Comment on attachment 310468 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310468&action=review > > r=me, with some notes. > > > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-455 > > - if (this._mouseDownCursorPosition.line === cursor.line && this._mouseDownCursorPosition.ch === cursor.ch) { > > Hmm, it would be nice if we could keep this check and return early if it > fails. This would improve the readability of the function and avoid doing > the work to determine `clickedBookmark` when not needed. To do this would > require that `this._mouseDownCursorPosition = null` happen first, instead of > at the end of the function. A local `mouseDownCursorPosition` would be > needed since the rest of the function uses the value. > > I'll leave it up to you. If you think the function is cleanest as-is, I'm > fine with it. I personally would prefer not to do that, as it seems a bit awkward to me to null a value that will be used later while also creating a local variable with that value. If it gets any more complex, however, it might be worth revisiting. 1310299 4 310468 commit-queue 2017-05-18 15:33:33 -0700 Comment on attachment 310468 Patch Clearing flags on attachment: 310468 Committed r217072: <http://trac.webkit.org/changeset/217072> 1310300 5 commit-queue 2017-05-18 15:33:35 -0700 All reviewed patches have been landed. Closing bug. 310468 2017-05-17 17:29:44 -0700 2017-05-18 15:33:33 -0700 Patch bug-171902-20170517172944.patch text/plain 2431 hi ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDEyNmMxNjEwYWViNWYxNjYwNzliYzkxNjc2NjMy ZWFhMGE4NzliN2YuLmY3MDU2YjY4ZDZjYjc4MmJjM2Q2NDllNzJjMTg2ZjQ2MGIxZjE1NmIgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dl Ykluc3BlY3RvclVJL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE3LTA1LTE3ICBEZXZp biBSb3Vzc28gIDxkcm91c3NvQGFwcGxlLmNvbT4KKworICAgICAgICBSRUdSRVNTSU9OIChyPyk6 IFdlYiBJbnNwZWN0b3I6IFNoaWZ0LWNsaWNrIG9uIGNvbG9yIHNxdWFyZSBpbiBTdHlsZXMgc2lk ZWJhciBzaG91bGQgbm90IHNlbGVjdCB0ZXh0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNzE5MDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIFVzZXJJbnRlcmZhY2UvVmlld3MvQ1NTU3R5bGVEZWNsYXJh dGlvblRleHRFZGl0b3IuanM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuQ1NTU3R5bGVEZWNsYXJh dGlvblRleHRFZGl0b3IucHJvdG90eXBlLl9oYW5kbGVNb3VzZVVwKToKKyAgICAgICAgRG8gbm90 IGF0dGVtcHQgdG8gc2VsZWN0IHRleHQgaWYgdGhlIGVsZW1lbnQgYmVpbmcgY2xpY2tlZCBpcyBh IGJvb2ttYXJrIChzdWNoIGFzIGEgc3dhdGNoKS4KKwogMjAxNy0wNS0xNSAgRnVqaWkgSGlyb25v cmkgIDxIaXJvbm9yaS5GdWppaUBzb255LmNvbT4KIAogICAgICAgICBbR1RLXSBXZWIgSW5zcGVj dG9yOiBBZGQgbmV3IEdUSysgaWNvbnMgZm9yIFNlYXJjaCBpY29uIG9mIFF1aWNrIE9wZW4gZGlh bG9nCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3 cy9DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVkaXRvci5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JV SS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NTU1N0eWxlRGVjbGFyYXRpb25UZXh0RWRpdG9yLmpzCmlu ZGV4IGE4OWM4NjMyNzk0N2RmNTBkNzk4NmZhODI2NThkMzU4MjUyODMzZTAuLjIyMTNkM2UwNTYz MDlmYmU3MmFmODFkNmRjNWNjNjc3YzQxZWM5M2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNw ZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NTU1N0eWxlRGVjbGFyYXRpb25UZXh0RWRpdG9y LmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NTU1N0 eWxlRGVjbGFyYXRpb25UZXh0RWRpdG9yLmpzCkBAIC00NTIsNyArNDUyLDIwIEBAIFdlYkluc3Bl Y3Rvci5DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVkaXRvciA9IGNsYXNzIENTU1N0eWxlRGVjbGFy YXRpb25UZXh0RWRpdG9yCiAgICAgICAgICAgICByZXR1cm47CiAKICAgICAgICAgbGV0IGN1cnNv ciA9IHRoaXMuX2NvZGVNaXJyb3IuY29vcmRzQ2hhcih7bGVmdDogZXZlbnQueCwgdG9wOiBldmVu dC55fSk7Ci0gICAgICAgIGlmICh0aGlzLl9tb3VzZURvd25DdXJzb3JQb3NpdGlvbi5saW5lID09 PSBjdXJzb3IubGluZSAmJiB0aGlzLl9tb3VzZURvd25DdXJzb3JQb3NpdGlvbi5jaCA9PT0gY3Vy c29yLmNoKSB7CisKKyAgICAgICAgbGV0IGNsaWNrZWRCb29rbWFyayA9IGZhbHNlOworICAgICAg ICBmb3IgKGxldCBtYXJrZXIgb2YgdGhpcy5fY29kZU1pcnJvci5maW5kTWFya3NBdChjdXJzb3Ip KSB7CisgICAgICAgICAgICBpZiAobWFya2VyLnR5cGUgIT09ICJib29rbWFyayIgfHwgbWFya2Vy LnJlcGxhY2VkV2l0aCAhPT0gZXZlbnQudGFyZ2V0KQorICAgICAgICAgICAgICAgIGNvbnRpbnVl OworCisgICAgICAgICAgICBsZXQgcG9zID0gbWFya2VyLmZpbmQoKTsKKyAgICAgICAgICAgIGlm IChwb3MubGluZSA9PT0gY3Vyc29yLmxpbmUgJiYgTWF0aC5hYnMocG9zLmNoIC0gY3Vyc29yLmNo KSA8PSAxKSB7CisgICAgICAgICAgICAgICAgY2xpY2tlZEJvb2ttYXJrID0gdHJ1ZTsKKyAgICAg ICAgICAgICAgICBicmVhazsKKyAgICAgICAgICAgIH0KKyAgICAgICAgfQorCisgICAgICAgIGlm ICghY2xpY2tlZEJvb2ttYXJrICYmIHRoaXMuX21vdXNlRG93bkN1cnNvclBvc2l0aW9uLmxpbmUg PT09IGN1cnNvci5saW5lICYmIHRoaXMuX21vdXNlRG93bkN1cnNvclBvc2l0aW9uLmNoID09PSBj dXJzb3IuY2gpIHsKICAgICAgICAgICAgIGxldCBsaW5lID0gdGhpcy5fY29kZU1pcnJvci5nZXRM aW5lKGN1cnNvci5saW5lKTsKICAgICAgICAgICAgIGlmIChjdXJzb3IuY2ggPT09IGxpbmUudHJp bVJpZ2h0KCkubGVuZ3RoKSB7CiAgICAgICAgICAgICAgICAgbGV0IG5leHRMaW5lID0gdGhpcy5f Y29kZU1pcnJvci5nZXRMaW5lKGN1cnNvci5saW5lICsgMSk7Cg==