Merge "Remove obsolete TODO comments about the 'changed' event."
diff --git a/.gitignore b/.gitignore
index 2d146c8..cc71eb5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,7 @@
/.aspect/
/.apt_generated
/.apt_generated_tests
+/.eslintcache
/.bazel_path
/.classpath
/.factorypath
diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock
index 7ea3e69..fe7c2be 100644
--- a/MODULE.bazel.lock
+++ b/MODULE.bazel.lock
@@ -318,7 +318,7 @@
"recordedInputs": [
"REPO_MAPPING:aspect_rules_ts+,aspect_rules_ts aspect_rules_ts+",
"REPO_MAPPING:aspect_rules_ts+,bazel_tools bazel_tools",
- "FILE:@@//package.json f230bdb8c76c59a925ec0727a1decc727388d47d4b20d95b4e5bcf328edc0c79"
+ "FILE:@@//package.json 582559a124fbee5740e1c3a5b4167aa579a1deb338e413bd82f358940feae420"
],
"generatedRepoSpecs": {
"npm_typescript": {
diff --git a/package.json b/package.json
index 97acc45..75a1a9f 100644
--- a/package.json
+++ b/package.json
@@ -66,11 +66,11 @@
"test:single:coverage": "yarn --cwd=polygerrit-ui test:single:coverage",
"safe_bazelisk": "if which bazelisk >/dev/null; then bazel_bin=bazelisk; else bazel_bin=bazel; fi && $bazel_bin",
"eslint": "npm run safe_bazelisk test polygerrit-ui/app:lint_test",
- "eslintfix": "npm run safe_bazelisk run polygerrit-ui/app:lint_bin -- -- --fix $(pwd)/polygerrit-ui/app",
- "eslintfix:modified": "git diff --name-only --diff-filter=d | grep -E 'polygerrit-ui/app/.*\\.(js|ts)$' | sed 's|^polygerrit-ui/app/||' | xargs -r npm run safe_bazelisk run polygerrit-ui/app:lint_bin -- -- --fix",
+ "eslintfix": "eslint -c polygerrit-ui/app/eslint-bazel.config.js polygerrit-ui/app --fix --cache",
+ "eslintfix:modified": "git diff --name-only --diff-filter=d | grep -E 'polygerrit-ui/app/.*\\.(js|ts)$' | xargs -r eslint -c polygerrit-ui/app/eslint-bazel.config.js --fix",
"litlint": "npm run safe_bazelisk run polygerrit-ui/app:lit_analysis",
"litlintforCI": "lit-analyzer --strict --rules.no-unknown-property off --rules.no-unknown-tag-name off --rules.no-incompatible-type-binding off --rules.no-incompatible-property-type off --rules.no-invalid-tag-name off --rules.no-property-visibility-mismatch off --rules.no-unknown-attribute off **/elements/**/*.ts",
- "lint": "eslint -c polygerrit-ui/app/eslint-bazel.config.js polygerrit-ui/app",
+ "lint": "eslint -c polygerrit-ui/app/eslint-bazel.config.js polygerrit-ui/app --cache",
"gjf": "./tools/gjf.sh run"
},
"repository": {
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 53811e9..3020bff 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
@@ -355,6 +355,7 @@
sharedStyles,
css`
:host {
+ display: block;
font-family: var(--font-family);
font-size: var(--font-size-normal);
font-weight: var(--font-weight-normal);
@@ -722,10 +723,19 @@
}
override firstUpdated() {
+ const lastComment = this.thread ? this.getLastComment() : undefined;
+ const isNewDraft =
+ isDraft(lastComment) && (lastComment?.message ?? '') === '';
if (this.shouldScrollIntoView) {
whenRendered(this, () => {
this.expandCollapseComments(false);
- this.commentBox?.focus();
+ // Because of the non-deterministic order of focus events firing from
+ // the JS event loop, focusing the comment box on a new draft can result
+ // in the draft comment not being focused, which means the user has to
+ // click into it to start typing.
+ if (!isNewDraft) {
+ this.commentBox?.focus();
+ }
// The delay is a hack because we don't know exactly when to
// scroll the comment into center.
// TODO: Find a better solution without a setTimeout
@@ -735,9 +745,9 @@
}, 500);
});
}
- if (this.thread && isDraft(this.getFirstComment())) {
- const msg = this.getFirstComment()?.message ?? '';
- if (msg.length === 0) this.editDraft();
+ // Focus the draft comment input to avoid the user having to click into it.
+ if (isNewDraft) {
+ this.editDraft();
}
}
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 816054b..8b5e166 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
@@ -173,6 +173,36 @@
);
});
+ test('focuses commentBox when it is NOT a new draft', async () => {
+ const thread = createThread(c1);
+ const element = await fixture<GrCommentThread>(html`
+ <gr-comment-thread
+ .thread=${thread}
+ .shouldScrollIntoView=${true}
+ ></gr-comment-thread>
+ `);
+ await element.updateComplete;
+ await new Promise(resolve => setTimeout(resolve, 0));
+
+ const commentBox = queryAndAssert<HTMLElement>(element, '.comment-box');
+ assert.equal(element.shadowRoot?.activeElement, commentBox);
+ });
+
+ test('does not focus commentBox when it IS a new draft', async () => {
+ const thread = createThread(createNewDraft({message: ''}));
+ const element = await fixture<GrCommentThread>(html`
+ <gr-comment-thread
+ .thread=${thread}
+ .shouldScrollIntoView=${true}
+ ></gr-comment-thread>
+ `);
+ await element.updateComplete;
+ await new Promise(resolve => setTimeout(resolve, 0));
+
+ const commentBox = queryAndAssert<HTMLElement>(element, '.comment-box');
+ assert.notEqual(element.shadowRoot?.activeElement, commentBox);
+ });
+
test('comment box spans 100% of container width', async () => {
const wrapper = await fixture(html`
<div style="width: 500px;">
@@ -518,6 +548,85 @@
// The draft should be discarded completely
assert.equal(draftElement.messageText, '');
});
+
+ test('handle Quote with multi-line message', async () => {
+ stubAdd.restore();
+ stubAdd = sinon
+ .stub(testResolver(commentsModelToken), 'addNewDraft')
+ .callsFake(draft => {
+ const newDraft = {
+ ...draft,
+ id: 'new-draft' as UrlEncodedCommentId,
+ __draft: true,
+ };
+ if (element.thread) {
+ element.thread = {
+ ...element.thread,
+ comments: [...element.thread.comments, newDraft],
+ };
+ }
+ return Promise.resolve(newDraft);
+ });
+
+ element.thread = createThread(c1, {
+ ...c2,
+ message: 'first line\nsecond line\nthird line',
+ unresolved: true,
+ });
+ await element.updateComplete;
+
+ queryAndAssert<GrButton>(element, '#quoteBtn').click();
+ assert.isTrue(stubAdd.called);
+ assert.equal(stubAdd.lastCall.firstArg.in_reply_to, c2.id);
+ await element.updateComplete;
+
+ const draftElement = queryAndAssert<GrComment>(
+ element,
+ 'gr-comment.draft'
+ );
+ await draftElement.updateComplete;
+ await waitUntil(
+ () =>
+ draftElement.messageText ===
+ '> first line\n> second line\n> third line\n\n'
+ );
+ assert.equal(
+ draftElement.messageText,
+ '> first line\n> second line\n> third line\n\n'
+ );
+ });
+
+ test('handle reply-to-comment event from child comment', async () => {
+ element.thread = createThread(c1, {...c2, unresolved: true});
+ await element.updateComplete;
+
+ const commentEl = queryAndAssert<GrComment>(element, 'gr-comment');
+ commentEl.dispatchEvent(
+ new CustomEvent('reply-to-comment', {
+ detail: {
+ content: 'custom response',
+ userWantsToEdit: true,
+ unresolved: true,
+ },
+ bubbles: true,
+ composed: true,
+ })
+ );
+
+ assert.isTrue(stubAdd.called);
+ assert.equal(stubAdd.lastCall.firstArg.in_reply_to, c2.id);
+ assert.equal(stubAdd.lastCall.firstArg.unresolved, true);
+ });
+
+ test('reply sets in_reply_to to the last comment id in thread', async () => {
+ element.thread = createThread(c1, c2);
+ await element.updateComplete;
+
+ queryAndAssert<GrButton>(element, '#replyBtn').click();
+ assert.isTrue(stubAdd.called);
+ const newDraft = stubAdd.lastCall.firstArg;
+ assert.equal(newDraft.in_reply_to, c2.id);
+ });
});
test('comments are sorted correctly', () => {
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 418ddb0..24ca939 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -1381,11 +1381,9 @@
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());
+ this.focusTextarea().catch(() => {
+ // Ignore error since failure to focus is non-fatal.
+ });
}
}
if (changed.has('changeNum') || changed.has('comment')) {
@@ -1400,6 +1398,27 @@
}
}
+ private async focusTextarea(): Promise<void> {
+ await this.updateComplete;
+ if (!this.textarea) {
+ return;
+ }
+ await this.textarea.updateComplete;
+ if (this.isVisible()) {
+ this.textarea.putCursorAtEnd();
+ } else {
+ // 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());
+ }
+ }
+
+ private isVisible(): boolean {
+ return this.offsetWidth > 0 || this.offsetHeight > 0;
+ }
+
override willUpdate(changed: PropertyValues) {
this.firstWillUpdate();
if (changed.has('comment')) {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
index 58a64bd..b437a74 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
@@ -54,6 +54,7 @@
import {suggestionsServiceToken} from '../../../services/suggestions/suggestions-service';
import {AutocompletionContext} from '../../../utils/autocomplete-cache';
import {FixSuggestionInfo} from '../../../api/rest-api';
+import {GrSuggestionTextarea} from '../gr-suggestion-textarea/gr-suggestion-textarea';
suite('gr-comment tests', () => {
let element: GrComment;
@@ -533,6 +534,24 @@
assert.isTrue(element.isSaveDisabled());
});
+ test('focuses textarea when editing is set to true', async () => {
+ const spy = sinon.spy(GrSuggestionTextarea.prototype, 'putCursorAtEnd');
+ try {
+ element.comment = createDraft();
+ element.editing = false;
+ await element.updateComplete;
+
+ element.editing = true;
+ await element.updateComplete;
+ // focusTextarea is async, wait for it to complete.
+ await new Promise(resolve => setTimeout(resolve, 0));
+
+ assert.isTrue(spy.called);
+ } finally {
+ spy.restore();
+ }
+ });
+
test('ctrl+s saves comment', async () => {
const spy = sinon.stub(element, 'save');
element.messageText = 'is that the horse from horsing around??';
