Guard comment textarea and overlays behind dom-if templates
Until the thread list was added, we rarely rendered a large number of
diff comments at once. A diff would add them incrementally in an
asynchronous loop, and even then, their number was reduced by being
filtered to to the given path and patch range.
Rendering all comments at once in the thread list exposed inefficiencies
in instantiating gr-diff-comments, and, for changes with many comments,
could temporarily lockup the browser.
The biggest cause of slowness in comment instantiation was the textarea
for editing. Because existing comments are not in an edit state by
default (...and because only the final comment in a thread can be edited
anyway ... assuming the user even chooses to edit ... assuming there
even is an authenticated user...) it's more efficient to put the
textarea in a dom-if until it's needed.
The overlays are also expensive but infrequently used. These are put
inside a dom-if as well.
Change-Id: I50702afe2b178221110ad3200c5e3862a7722c5d
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
index b6409792..f1bce4c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -273,14 +273,16 @@
[[comment.robot_id]]
</div>
</template>
- <gr-textarea
- id="editTextarea"
- class="editMessage"
- autocomplete="on"
- monospace
- disabled="{{disabled}}"
- rows="4"
- text="{{_messageText}}"></gr-textarea>
+ <template is="dom-if" if="[[editing]]">
+ <gr-textarea
+ id="editTextarea"
+ class="editMessage"
+ autocomplete="on"
+ monospace
+ disabled="{{disabled}}"
+ rows="4"
+ text="{{_messageText}}"></gr-textarea>
+ </template>
<!--The message class is needed to ensure selectability from
gr-diff-selection.-->
<gr-formatted-text class="message"
@@ -339,27 +341,29 @@
</div>
</div>
</div>
- <gr-overlay id="confirmDeleteOverlay" with-backdrop>
- <gr-confirm-delete-comment-dialog id="confirmDeleteComment"
- on-confirm="_handleConfirmDeleteComment"
- on-cancel="_handleCancelDeleteComment">
- </gr-confirm-delete-comment-dialog>
- </gr-overlay>
- <gr-overlay id="confirmDiscardOverlay" with-backdrop>
- <gr-confirm-dialog
- id="confirmDiscardDialog"
- confirm-label="Discard"
- confirm-on-enter
- on-confirm="_handleConfirmDiscard"
- on-cancel="_closeConfirmDiscardOverlay">
- <div class="header" slot="header">
- Discard comment
- </div>
- <div class="main" slot="main">
- Are you sure you want to discard this draft comment?
- </div>
- </gr-confirm-dialog>
- </gr-overlay>
+ <template is="dom-if" if="[[_enableOverlay]]">
+ <gr-overlay id="confirmDeleteOverlay" with-backdrop>
+ <gr-confirm-delete-comment-dialog id="confirmDeleteComment"
+ on-confirm="_handleConfirmDeleteComment"
+ on-cancel="_handleCancelDeleteComment">
+ </gr-confirm-delete-comment-dialog>
+ </gr-overlay>
+ <gr-overlay id="confirmDiscardOverlay" with-backdrop>
+ <gr-confirm-dialog
+ id="confirmDiscardDialog"
+ confirm-label="Discard"
+ confirm-on-enter
+ on-confirm="_handleConfirmDiscard"
+ on-cancel="_closeConfirmDiscardOverlay">
+ <div class="header" slot="header">
+ Discard comment
+ </div>
+ <div class="main" slot="main">
+ Are you sure you want to discard this draft comment?
+ </div>
+ </gr-confirm-dialog>
+ </gr-overlay>
+ </template>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
</template>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
index 9e5ae87..8612011 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -124,6 +124,22 @@
},
_savingMessage: String,
+
+ _enableOverlay: {
+ type: Boolean,
+ value: false,
+ },
+
+ /**
+ * Property for storing references to overlay elements. When the overlays
+ * are moved to Gerrit.getRootElement() to be shown they are no-longer
+ * children, so they can't be queried along the tree, so they are stored
+ * here.
+ */
+ _overlays: {
+ type: Object,
+ value: () => ({}),
+ },
},
observers: [
@@ -155,7 +171,31 @@
detached() {
this.cancelDebouncer('fire-update');
- this.$.editTextarea.closeDropdown();
+ if (this.textarea) {
+ this.textarea.closeDropdown();
+ }
+ },
+
+ get textarea() {
+ return this.$$('#editTextarea');
+ },
+
+ get confirmDeleteOverlay() {
+ if (!this._overlays.confirmDelete) {
+ this._enableOverlay = true;
+ Polymer.dom.flush();
+ this._overlays.confirmDelete = this.$$('#confirmDeleteOverlay');
+ }
+ return this._overlays.confirmDelete;
+ },
+
+ get confirmDiscardOverlay() {
+ if (!this._overlays.confirmDiscard) {
+ this._enableOverlay = true;
+ Polymer.dom.flush();
+ this._overlays.confirmDiscard = this.$$('#confirmDiscardOverlay');
+ }
+ return this._overlays.confirmDiscard;
},
_computeShowHideText(collapsed) {
@@ -272,9 +312,6 @@
_editingChanged(editing, previousValue) {
this.$.container.classList.toggle('editing', editing);
- if (editing) {
- this.$.editTextarea.putCursorAtEnd();
- }
if (this.comment && this.comment.id) {
this.$$('.cancel').hidden = !editing;
}
@@ -285,6 +322,12 @@
// To prevent event firing on comment creation.
this._fireUpdate();
}
+ if (editing) {
+ this.async(() => {
+ Polymer.dom.flush();
+ this.textarea.putCursorAtEnd();
+ }, 1);
+ }
},
_computeLinkToComment(comment) {
@@ -437,7 +480,7 @@
this._discardDraft();
return;
}
- this._openOverlay(this.$.confirmDiscardOverlay);
+ this._openOverlay(this.confirmDiscardOverlay);
},
_handleConfirmDiscard(e) {
@@ -475,7 +518,7 @@
},
_closeConfirmDiscardOverlay() {
- this._closeOverlay(this.$.confirmDiscardOverlay);
+ this._closeOverlay(this.confirmDiscardOverlay);
},
_getSavingMessage(numPending) {
@@ -593,11 +636,11 @@
},
_handleCommentDelete() {
- this._openOverlay(this.$.confirmDeleteOverlay);
+ this._openOverlay(this.confirmDeleteOverlay);
},
_handleCancelDeleteComment() {
- this._closeOverlay(this.$.confirmDeleteOverlay);
+ this._closeOverlay(this.confirmDeleteOverlay);
},
_openOverlay(overlay) {
@@ -613,9 +656,11 @@
},
_handleConfirmDeleteComment() {
+ const dialog =
+ this.confirmDeleteOverlay.querySelector('#confirmDeleteComment');
this.$.restAPI.deleteComment(
- this.changeNum, this.patchNum, this.comment.id,
- this.$.confirmDeleteComment.message).then(newComment => {
+ this.changeNum, this.patchNum, this.comment.id, dialog.message)
+ .then(newComment => {
this._handleCancelDeleteComment();
this.comment = newComment;
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
index d124764..5b43430 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -80,8 +80,7 @@
'gr-formatted-text is not visible');
assert.isFalse(isVisible(element.$$('.actions')),
'actions are not visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
// The header middle content is only visible when comments are collapsed.
// It shows the message in a condensed way, and limits to a single line.
@@ -95,8 +94,7 @@
'gr-formatted-text is visible');
assert.isTrue(isVisible(element.$$('.actions')),
'actions are visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
assert.isFalse(isVisible(element.$$('.collapsedContent')),
'header middle content is not visible');
});
@@ -167,8 +165,7 @@
'gr-formatted-text is not visible');
assert.isFalse(isVisible(element.$$('.actions')),
'actions are not visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
assert.isTrue(isVisible(element.$$('.collapsedContent')),
'header middle content is visible');
@@ -178,8 +175,7 @@
'gr-formatted-text is visible');
assert.isTrue(isVisible(element.$$('.actions')),
'actions are visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
assert.isFalse(isVisible(element.$$('.collapsedContent')),
'header middle content is is not visible');
});
@@ -201,50 +197,50 @@
test('esc closes comment when text is empty', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 27); // esc
+ element.textarea, 27); // esc
assert.isTrue(element._handleCancel.called);
});
test('ctrl+enter does not save', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 13, 'ctrl'); // ctrl + enter
+ element.textarea, 13, 'ctrl'); // ctrl + enter
assert.isFalse(element._handleSave.called);
});
test('meta+enter does not save', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 13, 'meta'); // meta + enter
+ element.textarea, 13, 'meta'); // meta + enter
assert.isFalse(element._handleSave.called);
});
test('ctrl+s does not save', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 83, 'ctrl'); // ctrl + s
+ element.textarea, 83, 'ctrl'); // ctrl + s
assert.isFalse(element._handleSave.called);
});
});
test('esc does not close comment that has content', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 27); // esc
+ element.textarea, 27); // esc
assert.isFalse(element._handleCancel.called);
});
test('ctrl+enter saves', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 13, 'ctrl'); // ctrl + enter
+ element.textarea, 13, 'ctrl'); // ctrl + enter
assert.isTrue(element._handleSave.called);
});
test('meta+enter saves', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 13, 'meta'); // meta + enter
+ element.textarea, 13, 'meta'); // meta + enter
assert.isTrue(element._handleSave.called);
});
test('ctrl+s saves', () => {
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea, 83, 'ctrl'); // ctrl + s
+ element.textarea, 83, 'ctrl'); // ctrl + s
assert.isTrue(element._handleSave.called);
});
});
@@ -264,7 +260,7 @@
test('delete comment', done => {
sandbox.stub(
element.$.restAPI, 'deleteComment').returns(Promise.resolve({}));
- sandbox.spy(element.$.confirmDeleteOverlay, 'open');
+ sandbox.spy(element.confirmDeleteOverlay, 'open');
element.changeNum = 42;
element.patchNum = 0xDEADBEEF;
element._isAdmin = true;
@@ -272,8 +268,10 @@
.classList.contains('showDeleteButtons'));
MockInteractions.tap(element.$$('.action.delete'));
flush(() => {
- element.$.confirmDeleteOverlay.open.lastCall.returnValue.then(() => {
- element.$.confirmDeleteComment.message = 'removal reason';
+ element.confirmDeleteOverlay.open.lastCall.returnValue.then(() => {
+ const dialog =
+ this.confirmDeleteOverlay.querySelector('#confirmDeleteComment');
+ dialog.message = 'removal reason';
element._handleConfirmDeleteComment();
assert.isTrue(element.$.restAPI.deleteComment.calledWith(
42, 0xDEADBEEF, 'baf0414d_60047215', 'removal reason'));
@@ -351,6 +349,7 @@
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.editing = true;
+ flushAsynchronousOperations();
assert.isFalse(isVisible(element.$$('.edit')), 'edit is not visible');
assert.isFalse(isVisible(element.$$('.discard')), 'discard not visible');
assert.isTrue(isVisible(element.$$('.save')), 'save is visible');
@@ -361,6 +360,7 @@
element.draft = false;
element.editing = false;
+ flushAsynchronousOperations();
assert.isFalse(isVisible(element.$$('.edit')), 'edit is not visible');
assert.isFalse(isVisible(element.$$('.discard')),
'discard is not visible');
@@ -372,6 +372,7 @@
element.comment.id = 'foo';
element.draft = true;
element.editing = true;
+ flushAsynchronousOperations();
assert.isTrue(isVisible(element.$$('.cancel')), 'cancel is visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
@@ -411,8 +412,7 @@
'gr-formatted-text is not visible');
assert.isFalse(isVisible(element.$$('.actions')),
'actions are not visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
assert.isTrue(isVisible(element.$$('.collapsedContent')),
'header middle content is visible');
@@ -422,21 +422,20 @@
'gr-formatted-text is visible');
assert.isTrue(isVisible(element.$$('.actions')),
'actions are visible');
- assert.isFalse(isVisible(element.$$('gr-textarea')),
- 'textarea is not visible');
+ assert.isNotOk(element.textarea, 'textarea is not visible');
assert.isFalse(isVisible(element.$$('.collapsedContent')),
'header middle content is is not visible');
// When the edit button is pressed, should still see the actions
// and also textarea
MockInteractions.tap(element.$$('.edit'));
+ flushAsynchronousOperations();
assert.isFalse(element.collapsed);
assert.isFalse(isVisible(element.$$('gr-formatted-text')),
'gr-formatted-text is not visible');
assert.isTrue(isVisible(element.$$('.actions')),
'actions are visible');
- assert.isTrue(isVisible(element.$$('gr-textarea')),
- 'textarea is visible');
+ assert.isTrue(isVisible(element.textarea), 'textarea is visible');
assert.isFalse(isVisible(element.$$('.collapsedContent')),
'header middle content is not visible');
@@ -460,8 +459,7 @@
'gr-formatted-text is not visible');
assert.isTrue(isVisible(element.$$('.actions')),
'actions are visible');
- assert.isTrue(isVisible(element.$$('gr-textarea')),
- 'textarea is visible');
+ assert.isTrue(isVisible(element.textarea), 'textarea is visible');
assert.isFalse(isVisible(element.$$('.collapsedContent')),
'header middle content is not visible');
});
@@ -496,7 +494,8 @@
MockInteractions.tap(element.$$('.cancel'));
element.flushDebouncer('fire-update');
element._messageText = '';
- MockInteractions.pressAndReleaseKeyOn(element.$.editTextarea, 27); // esc
+ flushAsynchronousOperations();
+ MockInteractions.pressAndReleaseKeyOn(element.textarea, 27); // esc
});
test('draft discard removes message from storage', done => {
@@ -562,7 +561,7 @@
test('confirms discard of comments with message text', () => {
element._messageText = 'test';
element._handleDiscard(mockEvent);
- assert.isTrue(overlayStub.calledWith(element.$.confirmDiscardOverlay));
+ assert.isTrue(overlayStub.calledWith(element.confirmDiscardOverlay));
assert.isFalse(discardStub.called);
});
@@ -581,8 +580,10 @@
done();
});
element._messageText = 'is that the horse from horsing around??';
+ element.editing = true;
+ flushAsynchronousOperations();
MockInteractions.pressAndReleaseKeyOn(
- element.$.editTextarea.$.textarea.textarea,
+ element.textarea.$.textarea.textarea,
83, 'ctrl'); // 'ctrl + s'
});
