32005 2009-11-30 20:51:59 -0800 Web Inspector: Escape key in the Search Field should be more User Friendly 2009-12-01 12:09:00 -0800 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 joepeck joepeck bweinstein joepeck keishi pfeldman pmuellr rik timothy webkit.review.bot oldest_to_newest 167494 0 joepeck 2009-11-30 20:51:59 -0800 The Escape Key in the Search Field should do the following: - When manually focused (via the mouse) Escape should Clear the text and search but remain focused on the textfield. - Otherwise (⌘F) Escape should Clear the text and search but change focus to the previously selected element 167503 1 44053 joepeck 2009-11-30 22:12:26 -0800 Created attachment 44053 [PATCH] Desired Behavior previousFocusElement doesn't have a setter, and since this was the only special case I didn't think it pertinent to add a setter. I also named the function as descriptively as possible rather then searchOnMouseDown or something like we normally do, again my reasoning being this is a special case and the name is helpful. 167504 2 webkit.review.bot 2009-11-30 22:12:55 -0800 style-queue ran check-webkit-style on attachment 44053 without any errors. 167509 3 44053 eric 2009-11-30 22:20:30 -0800 Comment on attachment 44053 [PATCH] Desired Behavior Do we have any way to write a test for this? 167560 4 44053 timothy 2009-12-01 03:25:48 -0800 Comment on attachment 44053 [PATCH] Desired Behavior > + searchField.addEventListener("mousedown", this.searchFieldManualFocus.bind(this), false); // when the search field is manually selected > +WebInspector.searchFieldManualFocus = function(event) > +{ > + this.currentFocusElement = event.target; > + this._previousFocusElement = event.target; > +} This should alredy happen when the focus event fires. You shouldn't need this. 167616 5 joepeck 2009-12-01 08:04:50 -0800 (In reply to comment #4) > (From update of attachment 44053 [details]) > > + searchField.addEventListener("mousedown", this.searchFieldManualFocus.bind(this), false); // when the search field is manually selected > > +WebInspector.searchFieldManualFocus = function(event) > > +{ > > + this.currentFocusElement = event.target; > > + this._previousFocusElement = event.target; > > +} > > This should alredy happen when the focus event fires. You shouldn't need this. My goal was to make it so when you manually click on the search field, pushing Escape would keep the focus on the search field. I didn't want to create and delete a flag because that is normally dirty. An easy solution is to set both the previous and current to be the searchField. In this case the searchFieldManualFocus focus event will fire before the other due to the bubbling order. Therefore if I set: - just the previous, then when this bubbles out to the main handler then previous will get overwritten with the "old" current. - just the current, then the previous does not get changed My solution was to just set them both, to be sure they had the same value. Should I attack this in a different way? 167618 6 44053 pfeldman 2009-12-01 08:06:04 -0800 Comment on attachment 44053 [PATCH] Desired Behavior > + this.performSearch(event); We need this for cleanup, right? 167624 7 joepeck 2009-12-01 08:13:29 -0800 (In reply to comment #3) > (From update of attachment 44053 [details]) > Do we have any way to write a test for this? This is mostly a UI feature. Testing it would involve "fake" events or calling frontend functions in a particular order assuming they were fired. We typically haven't included tests for these types of inspector features but if desired I could add a test. 167625 8 joepeck 2009-12-01 08:15:14 -0800 (In reply to comment #6) > (From update of attachment 44053 [details]) > > + this.performSearch(event); > > We need this for cleanup, right? Yes. Due to having just set the text to the empty string, AND in such a way the that the inputs incremental search event doesn't automatically fire, I call performSearch which would clear any existing search. 167649 9 44053 timothy 2009-12-01 09:18:09 -0800 Comment on attachment 44053 [PATCH] Desired Behavior > + this.currentFocusElement = this.previousFocusElement; Why do you need this? Wont the focus stay on the search field like you want if you removed this line? Then you don't need the mousedown listener. 167654 10 joepeck 2009-12-01 09:26:26 -0800 (In reply to comment #9) > (From update of attachment 44053 [details]) > > > + this.currentFocusElement = this.previousFocusElement; > > Why do you need this? Wont the focus stay on the search field like you want if > you removed this line? Then you don't need the mousedown listener. This is for behaviorcase #2, in comment 1. For instance if you had selected a node in the Tree HHierarchy, pushed ⌘F, cancelled with Escape, it would go back to the element you had selected. This is similar to using ⌘F in Safari itself if you start the process with the focus on a text field. 167689 11 44091 joepeck 2009-12-01 11:40:22 -0800 Created attachment 44091 [PATCH] Desired Behavior Although I was pretty certain it was working before after a rebuild in behavior #1 pushing Escape made it so the search field had focus but I couldn't start typing in it! Throwing in a select() made it work as expected. 167691 12 webkit.review.bot 2009-12-01 11:41:54 -0800 style-queue ran check-webkit-style on attachment 44091 without any errors. 167697 13 joepeck 2009-12-01 12:09:00 -0800 Landed in http://trac.webkit.org/changeset/51553 r51553 = 02237f7508bc456dd797e280182c49e9a6a587ce 44053 2009-11-30 22:12:26 -0800 2009-12-01 11:40:22 -0800 [PATCH] Desired Behavior patch text/plain 2256 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NTQ4Y2NkZC4uYzQ3YzExMyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwxNyBAQAogMjAwOS0xMS0zMCAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgogCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IEVzY2FwZSBrZXkgaW4gdGhlIFNlYXJj aCBGaWVsZCBzaG91bGQgYmUgbW9yZSBVc2VyIEZyaWVuZGx5CisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMjAwNQorCisgICAgICAgICogaW5zcGVjdG9y L2Zyb250LWVuZC9pbnNwZWN0b3IuanM6CisgICAgICAgIChXZWJJbnNwZWN0b3IubG9hZGVkKToK KyAgICAgICAgKFdlYkluc3BlY3Rvci5zZWFyY2hGaWVsZE1hbnVhbEZvY3VzKToKKyAgICAgICAg KFdlYkluc3BlY3Rvci5zZWFyY2hLZXlEb3duKToKKworMjAwOS0xMS0zMCAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCiAgICAgICAgIFJldmlld2VkIGJ5IFBhdmVsIEZl bGRtYW4uCiAKICAgICAgICAgV2ViIEluc3BlY3RvcjogV3JvbmcgY29uc29sZSBvdXRwdXQgZm9y IFJlZ2V4cCBlc2NhcGUgc2VxdWVuY2UKZGlmZiAtLWdpdCBhL1dlYkNvcmUvaW5zcGVjdG9yL2Zy b250LWVuZC9pbnNwZWN0b3IuanMgYi9XZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvaW5zcGVj dG9yLmpzCmluZGV4IDY1YzYwYWUuLjcyMWI1ZDEgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvaW5zcGVj dG9yL2Zyb250LWVuZC9pbnNwZWN0b3IuanMKKysrIGIvV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQt ZW5kL2luc3BlY3Rvci5qcwpAQCAtNTAzLDYgKzUwMyw3IEBAIFdlYkluc3BlY3Rvci5sb2FkZWQg PSBmdW5jdGlvbigpCiAKICAgICB2YXIgc2VhcmNoRmllbGQgPSBkb2N1bWVudC5nZXRFbGVtZW50 QnlJZCgic2VhcmNoIik7CiAgICAgc2VhcmNoRmllbGQuYWRkRXZlbnRMaXN0ZW5lcigic2VhcmNo IiwgdGhpcy5wZXJmb3JtU2VhcmNoLmJpbmQodGhpcyksIGZhbHNlKTsgLy8gd2hlbiB0aGUgc2Vh cmNoIGlzIGVtcHRpZWQKKyAgICBzZWFyY2hGaWVsZC5hZGRFdmVudExpc3RlbmVyKCJtb3VzZWRv d24iLCB0aGlzLnNlYXJjaEZpZWxkTWFudWFsRm9jdXMuYmluZCh0aGlzKSwgZmFsc2UpOyAvLyB3 aGVuIHRoZSBzZWFyY2ggZmllbGQgaXMgbWFudWFsbHkgc2VsZWN0ZWQKIAogICAgIHRvb2xiYXJF bGVtZW50LmFkZEV2ZW50TGlzdGVuZXIoIm1vdXNlZG93biIsIHRoaXMudG9vbGJhckRyYWdTdGFy dCwgdHJ1ZSk7CiAgICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoImNsb3NlLWJ1dHRvbi1sZWZ0 IikuYWRkRXZlbnRMaXN0ZW5lcigiY2xpY2siLCB0aGlzLmNsb3NlLCB0cnVlKTsKQEAgLTE1Mjks OCArMTUzMCwyNCBAQCBXZWJJbnNwZWN0b3IuYWRkTWFpbkV2ZW50TGlzdGVuZXJzID0gZnVuY3Rp b24oZG9jKQogICAgIGRvYy5hZGRFdmVudExpc3RlbmVyKCJjbGljayIsIHRoaXMuZG9jdW1lbnRD bGljay5iaW5kKHRoaXMpLCB0cnVlKTsKIH0KIAorV2ViSW5zcGVjdG9yLnNlYXJjaEZpZWxkTWFu dWFsRm9jdXMgPSBmdW5jdGlvbihldmVudCkKK3sKKyAgICB0aGlzLmN1cnJlbnRGb2N1c0VsZW1l bnQgPSBldmVudC50YXJnZXQ7CisgICAgdGhpcy5fcHJldmlvdXNGb2N1c0VsZW1lbnQgPSBldmVu dC50YXJnZXQ7Cit9CisKIFdlYkluc3BlY3Rvci5zZWFyY2hLZXlEb3duID0gZnVuY3Rpb24oZXZl bnQpCiB7CisgICAgLy8gRXNjYXBlIEtleSB3aWxsIGNsZWFyIHRoZSBmaWVsZCBhbmQgY2xlYXIg dGhlIHNlYXJjaCByZXN1bHRzCisgICAgaWYgKGV2ZW50LmtleUNvZGUgPT09IFdlYkluc3BlY3Rv ci5LZXlib2FyZFNob3J0Y3V0LktleUNvZGVzLkVzYykgeworICAgICAgICBldmVudC5wcmV2ZW50 RGVmYXVsdCgpOworICAgICAgICBldmVudC5oYW5kbGVkID0gdHJ1ZTsKKyAgICAgICAgZXZlbnQu dGFyZ2V0LnZhbHVlID0gIiI7CisgICAgICAgIHRoaXMucGVyZm9ybVNlYXJjaChldmVudCk7Cisg ICAgICAgIHRoaXMuY3VycmVudEZvY3VzRWxlbWVudCA9IHRoaXMucHJldmlvdXNGb2N1c0VsZW1l bnQ7CisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CisKICAgICBpZiAoIWlzRW50ZXJLZXko ZXZlbnQpKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAK 44091 2009-12-01 11:40:22 -0800 2009-12-01 11:53:36 -0800 [PATCH] Desired Behavior patch text/plain 2385 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YzA0NTZmMi4uOThhMTU4OSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAwOS0xMi0wMSAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IFBhdmVsIEZl bGRtYW4uCisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogRXNjYXBlIGtleSBpbiB0aGUgU2VhcmNo IEZpZWxkIHNob3VsZCBiZSBtb3JlIFVzZXIgRnJpZW5kbHkKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMyMDA1CisKKyAgICAgICAgKiBpbnNwZWN0b3Iv ZnJvbnQtZW5kL2luc3BlY3Rvci5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5sb2FkZWQpOiBh ZGQgZm9jdXMgbGlzdGVuZXIgb24gdGhlIHNlYXJjaCBmaWVsZAorICAgICAgICAoV2ViSW5zcGVj dG9yLnNlYXJjaEZpZWxkTWFudWFsRm9jdXMpOiB1c2VyIGNsaWNrZWQgdG8gZm9jdXMgb24gdGhl IHNlYXJjaCBmaWVsZAorICAgICAgICAoV2ViSW5zcGVjdG9yLnNlYXJjaEtleURvd24pOiBoYW5k bGUgZXNjYXBlCisKIDIwMDktMTItMDEgIFN0ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29nbGUu Y29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERpbWl0cmkgR2xhemtvdi4KZGlmZiAtLWdpdCBh L1dlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9pbnNwZWN0b3IuanMgYi9XZWJDb3JlL2luc3Bl Y3Rvci9mcm9udC1lbmQvaW5zcGVjdG9yLmpzCmluZGV4IGNlMjM0ZjguLjdkNWFkYWYgMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9pbnNwZWN0b3IuanMKKysrIGIvV2Vi Q29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL2luc3BlY3Rvci5qcwpAQCAtNTAzLDYgKzUwMyw3IEBA IFdlYkluc3BlY3Rvci5sb2FkZWQgPSBmdW5jdGlvbigpCiAKICAgICB2YXIgc2VhcmNoRmllbGQg PSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgic2VhcmNoIik7CiAgICAgc2VhcmNoRmllbGQuYWRk RXZlbnRMaXN0ZW5lcigic2VhcmNoIiwgdGhpcy5wZXJmb3JtU2VhcmNoLmJpbmQodGhpcyksIGZh bHNlKTsgLy8gd2hlbiB0aGUgc2VhcmNoIGlzIGVtcHRpZWQKKyAgICBzZWFyY2hGaWVsZC5hZGRF dmVudExpc3RlbmVyKCJtb3VzZWRvd24iLCB0aGlzLnNlYXJjaEZpZWxkTWFudWFsRm9jdXMuYmlu ZCh0aGlzKSwgZmFsc2UpOyAvLyB3aGVuIHRoZSBzZWFyY2ggZmllbGQgaXMgbWFudWFsbHkgc2Vs ZWN0ZWQKIAogICAgIHRvb2xiYXJFbGVtZW50LmFkZEV2ZW50TGlzdGVuZXIoIm1vdXNlZG93biIs IHRoaXMudG9vbGJhckRyYWdTdGFydCwgdHJ1ZSk7CiAgICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5 SWQoImNsb3NlLWJ1dHRvbi1sZWZ0IikuYWRkRXZlbnRMaXN0ZW5lcigiY2xpY2siLCB0aGlzLmNs b3NlLCB0cnVlKTsKQEAgLTE1MjksOCArMTUzMCwyOCBAQCBXZWJJbnNwZWN0b3IuYWRkTWFpbkV2 ZW50TGlzdGVuZXJzID0gZnVuY3Rpb24oZG9jKQogICAgIGRvYy5hZGRFdmVudExpc3RlbmVyKCJj bGljayIsIHRoaXMuZG9jdW1lbnRDbGljay5iaW5kKHRoaXMpLCB0cnVlKTsKIH0KIAorV2ViSW5z cGVjdG9yLnNlYXJjaEZpZWxkTWFudWFsRm9jdXMgPSBmdW5jdGlvbihldmVudCkKK3sKKyAgICB0 aGlzLmN1cnJlbnRGb2N1c0VsZW1lbnQgPSBldmVudC50YXJnZXQ7CisgICAgdGhpcy5fcHJldmlv dXNGb2N1c0VsZW1lbnQgPSBldmVudC50YXJnZXQ7Cit9CisKIFdlYkluc3BlY3Rvci5zZWFyY2hL ZXlEb3duID0gZnVuY3Rpb24oZXZlbnQpCiB7CisgICAgLy8gRXNjYXBlIEtleSB3aWxsIGNsZWFy IHRoZSBmaWVsZCBhbmQgY2xlYXIgdGhlIHNlYXJjaCByZXN1bHRzCisgICAgaWYgKGV2ZW50Lmtl eUNvZGUgPT09IFdlYkluc3BlY3Rvci5LZXlib2FyZFNob3J0Y3V0LktleUNvZGVzLkVzYykgewor ICAgICAgICBldmVudC5wcmV2ZW50RGVmYXVsdCgpOworICAgICAgICBldmVudC5oYW5kbGVkID0g dHJ1ZTsKKyAgICAgICAgZXZlbnQudGFyZ2V0LnZhbHVlID0gIiI7CisKKyAgICAgICAgdGhpcy5w ZXJmb3JtU2VhcmNoKGV2ZW50KTsKKyAgICAgICAgdGhpcy5jdXJyZW50Rm9jdXNFbGVtZW50ID0g dGhpcy5wcmV2aW91c0ZvY3VzRWxlbWVudDsKKyAgICAgICAgaWYgKHRoaXMuY3VycmVudEZvY3Vz RWxlbWVudCA9PT0gZXZlbnQudGFyZ2V0KQorICAgICAgICAgICAgdGhpcy5jdXJyZW50Rm9jdXNF bGVtZW50LnNlbGVjdCgpOworCisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CisKICAgICBp ZiAoIWlzRW50ZXJLZXkoZXZlbnQpKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAK