Fix focus handling for comment replies
Since we have included unsaved comment drafts into the comment model
we have an issue with replying to comments when two `gr-comment-thread`
instances are in the DOM for the same thread. `gr-comment` would just
universally identify itself in `firstWillUpdate()` whether is has to go
into editing mode or not. But only one of the two widgets should go into
editing mode: The one that the user has interacted with.
Fixing this means moving the responsibility up one level from
`gr-comment` into `gr-comment-thread`. The thread will either request
going into editing mode when the thread is completely new, or when the
user has requested to reply with editing intention.
While debugging we have seen a lot of `focus()` calls to `gr-comment`
and `gr-textarea` that had no effect, because the `<textarea>` was not
in the DOM. We are leaving comments for future developers to be aware
of this potential issue.
Release-Notes: skip
Google-Bug-Id: b/282196918
Change-Id: I6deac7dd897eb15f187ac739e10c5d15a5dd25a4
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 64dff29..d43dadc 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -74,6 +74,7 @@
import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
+import {waitUntil} from '../../../utils/async-util';
declare global {
interface HTMLElementEventMap {
@@ -112,6 +113,9 @@
@query('.comment-box')
commentBox?: HTMLElement;
+ @query('gr-comment.draft')
+ draftElement?: GrComment;
+
@queryAll('gr-comment')
commentElements?: NodeList;
@@ -495,6 +499,7 @@
: !this.unresolved);
return html`
<gr-comment
+ class=${classMap({draft: isDraft(comment)})}
.comment=${comment}
.comments=${this.thread!.comments}
?initially-collapsed=${initiallyCollapsed}
@@ -646,6 +651,15 @@
}, 500);
});
}
+ if (this.thread && isDraft(this.getFirstComment())) {
+ const msg = this.getFirstComment()?.message ?? '';
+ if (msg.length === 0) this.editDraft();
+ }
+ }
+
+ private async editDraft() {
+ await waitUntil(() => !!this.draftElement);
+ this.draftElement!.edit();
}
private isDraft() {
@@ -797,6 +811,7 @@
const newReply = createNewReply(replyingTo, content, unresolved);
if (userWantsToEdit) {
this.getCommentsModel().addNewDraft(newReply);
+ this.editDraft();
} else {
try {
this.saving = true;
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 1027a62..14ed24f 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -120,7 +120,11 @@
robot-button-disabled=""
show-patchset=""
></gr-comment>
- <gr-comment robot-button-disabled="" show-patchset=""></gr-comment>
+ <gr-comment
+ class="draft"
+ robot-button-disabled=""
+ show-patchset=""
+ ></gr-comment>
</div>
</div>
`
@@ -145,7 +149,11 @@
<div id="container">
<h3 class="assistive-tech-only">Draft Comment thread by Yoda</h3>
<div class="comment-box" tabindex="0">
- <gr-comment robot-button-disabled="" show-patchset=""></gr-comment>
+ <gr-comment
+ class="draft"
+ robot-button-disabled=""
+ show-patchset=""
+ ></gr-comment>
</div>
</div>
`
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 74c5806..158799d 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -924,13 +924,6 @@
if (this.permanentEditingMode) {
this.edit();
}
- if (
- isDraft(this.comment) &&
- isNew(this.comment) &&
- !isSaving(this.comment)
- ) {
- this.edit();
- }
if (isDraft(this.comment)) {
this.collapsed = false;
} else {
@@ -941,6 +934,10 @@
override updated(changed: PropertyValues) {
if (changed.has('editing')) {
if (this.editing && !this.permanentEditingMode) {
+ // Note that this is a bit fragile, because we are relying on the
+ // comment to become visible soonish. If that does not happen, then we
+ // will be waiting indefinitely and grab focus at some point in the
+ // distant future.
whenVisible(this, () => this.textarea?.putCursorAtEnd());
}
}
@@ -981,7 +978,7 @@
}
/** Enter editing mode. */
- private edit() {
+ edit() {
assert(isDraft(this.comment), 'only drafts are editable');
if (this.editing) return;
this.editing = true;
@@ -1065,6 +1062,8 @@
}
override focus() {
+ // Note that this may not work as intended, because the textarea is not
+ // rendered yet.
this.textarea?.focus();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 7779fff..95f1b8a 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -295,6 +295,8 @@
}
override focus() {
+ // Note that this may not work as intended, because the textarea is not
+ // rendered yet.
this.textarea?.textarea.focus();
}
@@ -627,7 +629,7 @@
this.currentSearchString = '';
this.closeDropdown();
this.specialCharIndex = -1;
- this.textarea?.textarea.focus();
+ this.focus();
}
private fireChangedEvents() {
