opendev/gerrit
Code Issues Proposed changes

Fix handling comment save in Shadow DOM

Browse Source 
e.target in native Shadow DOM will be retargetted to be some containing
element, not the gr-diff-comment element as expected and as it would be
in shady DOM.

I originally wanted to fix this by using Polymer.dom(e).rootTarget but
that has two downsides:
1) It's harder to test, because Polymer.dom does a instanceof check, and
creating and actual Event in the test means we cannot easily set target
because that is readonly.
2) It makes this component more dependent on gr-diff-comment having a
public property comment.

Luckily there is no reason to muck with e.target in the first place
because these event actually have the comment set on the detail aswell,
so just reading it from there circumvents the problem and is easy to
test.

Bug: Issue 10016
Change-Id: I08a546da4ba8d054ff4738f08d1f86ca6e510a3c
This commit is contained in:
Ole Rehmsen
2018-11-13 09:26:35 +01:00
parent 8a460d17a0
commit 75d6058f4e
2 changed files with 9 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -461,9 +461,9 @@
},
_handleCommentSave(e) {
if (!e.target.comment.__draft) { return; }
const draft = e.detail.comment;
if (!draft.__draft) { return; }
const draft = e.target.comment;
draft.patch_set = draft.patch_set || this._patchRange.patchNum;
// The use of path-based notification helpers (set, push) can’t be used
@@ -493,9 +493,9 @@
},
_handleCommentDiscard(e) {
if (!e.target.comment.__draft) { return; }
const draft = e.detail.comment;
if (!draft.__draft) { return; }
const draft = e.target.comment;
if (!this._diffDrafts[draft.path]) {
return;
}

10
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -658,12 +658,12 @@ limitations under the License.
path: '/foo/bar.txt',
text: 'hello',
};
element._handleCommentSave({target: {comment: draft}});
element._handleCommentSave({detail: {comment: draft}});
draft.patch_set = 2;
assert.deepEqual(element._diffDrafts, {'/foo/bar.txt': [draft]});
draft.patch_set = null;
draft.text = 'hello, there';
element._handleCommentSave({target: {comment: draft}});
element._handleCommentSave({detail: {comment: draft}});
draft.patch_set = 2;
assert.deepEqual(element._diffDrafts, {'/foo/bar.txt': [draft]});
const draft2 = {
@@ -672,14 +672,14 @@ limitations under the License.
path: '/foo/bar.txt',
text: 'hola',
};
element._handleCommentSave({target: {comment: draft2}});
element._handleCommentSave({detail: {comment: draft2}});
draft2.patch_set = 2;
assert.deepEqual(element._diffDrafts, {'/foo/bar.txt': [draft, draft2]});
draft.patch_set = null;
element._handleCommentDiscard({target: {comment: draft}});
element._handleCommentDiscard({detail: {comment: draft}});
draft.patch_set = 2;
assert.deepEqual(element._diffDrafts, {'/foo/bar.txt': [draft2]});
element._handleCommentDiscard({target: {comment: draft2}});
element._handleCommentDiscard({detail: {comment: draft2}});
assert.deepEqual(element._diffDrafts, {});
});