From 715219c44de4f5ebdcfa2875191b097e2dd0b648 Mon Sep 17 00:00:00 2001 From: Pegah Sarram Date: Wed, 11 Oct 2017 14:03:23 -0700 Subject: [PATCH] [Edit] Fix the issue with currentTarget being null and HTML being added on Enter Remove the line break that is added when the return key is pressed. Check the current target directly. Use textContent for inline editing instead of innerHTML, which will hoist up some HTML content implicitly created around user input for a contenteditable span. Note that there is a removeAllRanges in addition to a blur here; see https://stackoverflow.com/questions/4878713/how-can-i-blur-a-div-where-contenteditable-true Fix broken tests. Fixes #1757 --- .../browse/src/ObjectHeaderController.js | 42 ++++++++++++------- .../browse/test/ObjectHeaderControllerSpec.js | 25 ++++++----- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/platform/commonUI/browse/src/ObjectHeaderController.js b/platform/commonUI/browse/src/ObjectHeaderController.js index eef6aba0fa..c968e850eb 100644 --- a/platform/commonUI/browse/src/ObjectHeaderController.js +++ b/platform/commonUI/browse/src/ObjectHeaderController.js @@ -42,23 +42,37 @@ define( * @param event the mouse event */ ObjectHeaderController.prototype.updateName = function (event) { - if (event && (event.type === 'blur' || event.which === 13)) { - var name = event.currentTarget.innerHTML; + if (!event || !event.currentTarget) { + return; + } - if (name.length === 0) { - name = "Unnamed " + this.domainObject.getCapability("type").typeDef.name; - event.currentTarget.innerHTML = name; - } + if (event.type === 'blur') { + this.updateModel(event); + } else if (event.which === 13) { + this.updateModel(event); + event.currentTarget.blur(); + window.getSelection().removeAllRanges(); + } + }; - if (name !== this.$scope.domainObject.model.name) { - this.domainObject.getCapability('mutation').mutate(function (model) { - model.name = name; - }); - } + /** + * Updates the model. + * + * @param event the mouse event + * @param private + */ + ObjectHeaderController.prototype.updateModel = function (event) { + var name = event.currentTarget.textContent.replace(/\n/g, ' '); - if (event.which === 13) { - event.currentTarget.blur(); - } + if (name.length === 0) { + name = "Unnamed " + this.domainObject.getCapability("type").typeDef.name; + event.currentTarget.textContent = name; + } + + if (name !== this.domainObject.getModel().name) { + this.domainObject.getCapability('mutation').mutate(function (model) { + model.name = name; + }); } }; diff --git a/platform/commonUI/browse/test/ObjectHeaderControllerSpec.js b/platform/commonUI/browse/test/ObjectHeaderControllerSpec.js index 2624c92659..e697beff76 100644 --- a/platform/commonUI/browse/test/ObjectHeaderControllerSpec.js +++ b/platform/commonUI/browse/test/ObjectHeaderControllerSpec.js @@ -32,6 +32,7 @@ define( mockTypeCapability, mockEvent, mockCurrentTarget, + model, controller; beforeEach(function () { @@ -47,8 +48,11 @@ define( type: mockTypeCapability }; - mockDomainObject = jasmine.createSpyObj("domainObject", ["getCapability", "model"]); - mockDomainObject.model = {name: "Test name"}; + model = { + name: "Test name" + }; + mockDomainObject = jasmine.createSpyObj("domainObject", ["getCapability", "getModel"]); + mockDomainObject.getModel.andReturn(model); mockDomainObject.getCapability.andCallFake(function (key) { return mockCapabilities[key]; }); @@ -57,7 +61,7 @@ define( domainObject: mockDomainObject }; - mockCurrentTarget = jasmine.createSpyObj("currentTarget", ["blur", "innerHTML"]); + mockCurrentTarget = jasmine.createSpyObj("currentTarget", ["blur", "textContent"]); mockCurrentTarget.blur.andReturn(mockCurrentTarget); mockEvent = { @@ -71,7 +75,7 @@ define( it("updates the model with new name on blur", function () { mockEvent.type = "blur"; - mockCurrentTarget.innerHTML = "New name"; + mockCurrentTarget.textContent = "New name"; controller.updateName(mockEvent); expect(mockMutationCapability.mutate).toHaveBeenCalled(); @@ -79,23 +83,23 @@ define( it("updates the model with a default for blank names", function () { mockEvent.type = "blur"; - mockCurrentTarget.innerHTML = ""; + mockCurrentTarget.textContent = ""; controller.updateName(mockEvent); - expect(mockCurrentTarget.innerHTML.length).not.toEqual(0); + expect(mockCurrentTarget.textContent.length).not.toEqual(0); expect(mockMutationCapability.mutate).toHaveBeenCalled(); }); it("does not update the model if the same name", function () { mockEvent.type = "blur"; - mockCurrentTarget.innerHTML = mockDomainObject.model.name; + mockCurrentTarget.textContent = mockDomainObject.getModel().name; controller.updateName(mockEvent); expect(mockMutationCapability.mutate).not.toHaveBeenCalled(); }); it("updates the model on enter keypress event only", function () { - mockCurrentTarget.innerHTML = "New name"; + mockCurrentTarget.textContent = "New name"; controller.updateName(mockEvent); expect(mockMutationCapability.mutate).not.toHaveBeenCalled(); @@ -105,12 +109,13 @@ define( expect(mockMutationCapability.mutate).toHaveBeenCalledWith(jasmine.any(Function)); - mockMutationCapability.mutate.mostRecentCall.args[0](mockDomainObject.model); + mockMutationCapability.mutate.mostRecentCall.args[0](model); - expect(mockDomainObject.model.name).toBe("New name"); + expect(mockDomainObject.getModel().name).toBe("New name"); }); it("blurs the field on enter key press", function () { + mockCurrentTarget.textContent = "New name"; mockEvent.which = 13; controller.updateName(mockEvent);