Merge "Add aria-label for expand and collapse toggle on comment"
diff --git a/java/com/google/gerrit/server/config/UrlFormatter.java b/java/com/google/gerrit/server/config/UrlFormatter.java
index 6b2510e..8fc0d9e 100644
--- a/java/com/google/gerrit/server/config/UrlFormatter.java
+++ b/java/com/google/gerrit/server/config/UrlFormatter.java
@@ -47,6 +47,11 @@
return getWebUrl().map(url -> url + "c/" + project.get() + "/+/" + id.get());
}
+ /** Returns the URL for viewing the comment tab view of a change. */
+ default Optional<String> getCommentsTabView(Change change) {
+ return getChangeViewUrl(change.getProject(), change.getId()).map(url -> url + "?tab=comments");
+ }
+
/** Returns the URL for viewing a file in a given patch set of a change. */
default Optional<String> getPatchFileView(Change change, int patchsetId, String filename) {
return getChangeViewUrl(change.getProject(), change.getId())
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java
index 48d342e..deaa926 100644
--- a/java/com/google/gerrit/server/mail/send/CommentSender.java
+++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -88,6 +88,11 @@
.orElse(null);
}
+ /** @return a web link to the comment tab view of a change. */
+ public String getCommentsTabLink() {
+ return args.urlFormatter.get().getCommentsTabView(change).orElse(null);
+ }
+
/**
* @return A title for the group, i.e. "Commit Message", "Merge List", or "File [[filename]]".
*/
@@ -96,6 +101,8 @@
return "Commit Message";
} else if (Patch.MERGE_LIST.equals(filename)) {
return "Merge List";
+ } else if (Patch.PATCHSET_LEVEL.equals(filename)) {
+ return "Patchset";
} else {
return "File " + filename;
}
@@ -379,7 +386,11 @@
for (CommentSender.FileCommentGroup group : getGroupedInlineComments(repo)) {
Map<String, Object> groupData = new HashMap<>();
- groupData.put("link", group.getFileLink());
+ if (group.filename.equals(Patch.PATCHSET_LEVEL)) {
+ groupData.put("link", group.getCommentsTabLink());
+ } else {
+ groupData.put("link", group.getFileLink());
+ }
groupData.put("title", group.getTitle());
groupData.put("patchSetId", group.patchSetId);
@@ -407,7 +418,10 @@
commentData.put("startLine", startLine);
// Set the comment link.
- if (comment.lineNbr == 0) {
+
+ if (comment.key.filename.equals(Patch.PATCHSET_LEVEL)) {
+ commentData.put("link", group.getCommentsTabLink());
+ } else if (comment.lineNbr == 0) {
commentData.put("link", group.getFileLink());
} else {
commentData.put("link", group.getCommentLink(comment.side, startLine));
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 4fac821..e48c9d5 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -212,6 +212,20 @@
}
@Test
+ public void patchsetLevelCommentEmailNotification() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ String ps1 = result.getCommit().name();
+
+ CommentInput comment = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment");
+ addComments(changeId, ps1, comment);
+
+ String emailBody = Iterables.getOnlyElement(email.getMessages()).body();
+ assertThat(emailBody).contains("Patchset");
+ assertThat(emailBody).doesNotContain("/PATCHSET_LEVEL");
+ }
+
+ @Test
public void patchsetLevelCommentCantHaveLine() throws Exception {
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
diff --git a/polygerrit-ui/app/constants/constants.js b/polygerrit-ui/app/constants/constants.js
index 5072594..ff34442 100644
--- a/polygerrit-ui/app/constants/constants.js
+++ b/polygerrit-ui/app/constants/constants.js
@@ -20,9 +20,12 @@
* @desc Tab names for primary tabs on change view page.
*/
export const PrimaryTab = {
- FILES: '_files',
- COMMENT_THREADS: '_commentThreads',
- FINDINGS: '_findings',
+ FILES: 'files',
+ /**
+ * When renaming this, the links in UrlFormatter must be updated.
+ */
+ COMMENT_THREADS: 'comments',
+ FINDINGS: 'findings',
};
/**
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
index 13b3c24..b31ab9b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.js
@@ -117,7 +117,7 @@
<style include="gr-change-list-styles">
/* Workaround for empty style block - see https://github.com/Polymer/tools/issues/408 */
</style>
- <td class="cell leftPadding"></td>
+ <td aria-hidden="true" class="cell leftPadding"></td>
<td class="cell star" hidden$="[[!showStar]]" hidden="">
<gr-change-star change="{{change}}"></gr-change-star>
</td>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
index e5193f8..f7c50e6 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
@@ -52,8 +52,13 @@
<template is="dom-if" if="[[changeSection.name]]">
<tbody>
<tr class="groupHeader">
- <td class="leftPadding"></td>
- <td class="star" hidden$="[[!showStar]]" hidden=""></td>
+ <td aria-hidden="true" class="leftPadding"></td>
+ <td
+ aria-hidden="true"
+ class="star"
+ hidden$="[[!showStar]]"
+ hidden=""
+ ></td>
<td
class="cell"
colspan$="[[_computeColspan(changeTableColumns, labelNames)]]"
@@ -74,8 +79,8 @@
<tbody class="groupContent">
<template is="dom-if" if="[[_isEmpty(changeSection)]]">
<tr class="noChanges">
- <td class="leftPadding"></td>
- <td class="star" hidden$="[[!showStar]]" hidden=""></td>
+ <td aria-hidden="true" class="leftPadding"></td>
+ <td aria-hidden="true" class="star" hidden></td>
<td
class="cell"
colspan$="[[_computeColspan(changeTableColumns, labelNames)]]"
@@ -91,8 +96,13 @@
</template>
<template is="dom-if" if="[[!_isEmpty(changeSection)]]">
<tr class="groupTitle">
- <td class="leftPadding"></td>
- <td class="star" hidden$="[[!showStar]]" hidden=""></td>
+ <td aria-hidden="true" class="leftPadding"></td>
+ <td
+ aria-label="Star status column"
+ class="star"
+ hidden$="[[!showStar]]"
+ hidden=""
+ ></td>
<td class="number" hidden$="[[!showNumber]]" hidden="">#</td>
<template is="dom-repeat" items="[[changeTableColumns]]" as="item">
<td
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 5035818..05a3ae5 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -1098,16 +1098,9 @@
tab: primaryTab,
},
});
-
- // TODO: should drop this once we move CommentThreads tab
- // to primary as well
- let secondaryTab = SecondaryTab.CHANGE_LOG;
- if (params.queryMap && params.queryMap.has('secondaryTab')) {
- secondaryTab = params.queryMap.get('secondaryTab');
- }
this._setActiveSecondaryTab({
detail: {
- tab: secondaryTab,
+ tab: SecondaryTab.CHANGE_LOG,
},
});
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
index 1895064..6667ddb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
@@ -820,20 +820,6 @@
sandbox.spy(element, '_paramsChanged');
element.params = {view: 'change', changeNum: '1'};
});
-
- test('invalid secondaryTab should not switch tab', done => {
- assert.equal(element._activeTabs[1], SecondaryTab.CHANGE_LOG);
- const queryMap = new Map();
- queryMap.set('secondaryTab', 'random');
- // view is required
- element.params = Object.assign({
- view: GerritNav.View.CHANGE,
- }, element.params, {queryMap});
- flush(() => {
- assert.equal(element._activeTabs[1], SecondaryTab.CHANGE_LOG);
- done();
- });
- });
});
suite('Findings comment tab', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
index df6f43d..600b0bc 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
@@ -255,6 +255,8 @@
}
}
.pathLink:hover gr-copy-clipboard,
+ .pathLink:focus gr-copy-clipboard,
+ .oldPath:focus gr-copy-clipboard,
.oldPath:hover gr-copy-clipboard {
visibility: visible;
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.js b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
index 3dda25b..b8a405a 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
@@ -273,7 +273,7 @@
_computeMessageContentCollapsed(content, tag, commentThreads) {
const summary =
this._computeMessageContent(content, tag, false);
- if (summary || !commentThreads) return;
+ if (summary || !commentThreads) return summary;
return this._patchsetCommentSummary(commentThreads);
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
index edcb7f6..332ca52 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
@@ -252,6 +252,8 @@
const original = 'Uploaded patch set 1.';
const tag = 'autogenerated:gerrit:newPatchSet';
let actual = element._computeMessageContent(original, tag, true);
+ assert.equal(actual, element._computeMessageContentCollapsed(
+ original, tag, []));
assert.equal(actual, original);
actual = element._computeMessageContent(original, tag, false);
assert.equal(actual, original);
@@ -263,6 +265,8 @@
const expected = 'Patch Set 26 was rebased';
let actual = element._computeMessageContent(original, tag, true);
assert.equal(actual, expected);
+ assert.equal(actual, element._computeMessageContentCollapsed(
+ original, tag, []));
actual = element._computeMessageContent(original, tag, false);
assert.equal(actual, expected);
});
@@ -273,6 +277,8 @@
const expected = 'This change is ready for review.';
let actual = element._computeMessageContent(original, tag, true);
assert.equal(actual, expected);
+ assert.equal(actual, element._computeMessageContentCollapsed(
+ original, tag, []));
actual = element._computeMessageContent(original, tag, false);
assert.equal(actual, expected);
});
@@ -414,8 +420,9 @@
path: '/PATCHSET_LEVEL',
rootId: 'e365b138_bed65caa',
}];
- assert.equal(element._patchsetCommentSummary(threads),
- 'testing the load');
+ assert.equal(element._computeMessageContentCollapsed(
+ '', undefined, threads), 'testing the load');
+ assert.equal(element._computeMessageContent('', undefined, false), '');
});
test('single patchset comment with reply', () => {
@@ -446,7 +453,9 @@
path: '/PATCHSET_LEVEL',
rootId: 'e365b138_bed65caa',
}];
- assert.equal(element._patchsetCommentSummary(threads), 'n');
+ assert.equal(element._computeMessageContentCollapsed(
+ '', undefined, threads), 'n');
+ assert.equal(element._computeMessageContent('', undefined, false), '');
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.js b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.js
index 721e7510..162a7c0 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.js
@@ -57,20 +57,16 @@
<template is="dom-if" if="[[!hideToggleButtons]]">
<div class="header">
<div class="toggleItem">
- <paper-toggle-button
- id="unresolvedToggle"
- checked="{{_unresolvedOnly}}"
- ></paper-toggle-button>
- Only unresolved threads
+ <paper-toggle-button id="unresolvedToggle" checked="{{_unresolvedOnly}}"
+ >Only unresolved threads</paper-toggle-button
+ >
</div>
<div
class$="toggleItem draftToggle [[_computeShowDraftToggle(loggedIn)]]"
>
- <paper-toggle-button
- id="draftToggle"
- checked="{{_draftsOnly}}"
- ></paper-toggle-button>
- Only threads with drafts
+ <paper-toggle-button id="draftToggle" checked="{{_draftsOnly}}"
+ >Only threads with drafts</paper-toggle-button
+ >
</div>
</div>
</template>
diff --git a/polygerrit-ui/app/elements/shared/gr-account-chip/gr-account-chip_html.js b/polygerrit-ui/app/elements/shared/gr-account-chip/gr-account-chip_html.js
index 96e0160..ee19374 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-chip/gr-account-chip_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-account-chip/gr-account-chip_html.js
@@ -92,7 +92,6 @@
link=""
hidden$="[[!removable]]"
hidden=""
- tabindex="-1"
aria-label="Remove"
class$="remove [[_getBackgroundClass(transparentBackground)]]"
on-click="_handleRemoveTap"
diff --git a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.js b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.js
index 17e7f49..ce2dc9e 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.js
@@ -40,7 +40,7 @@
}
</style>
<span>
- <a href$="[[_computeOwnerLink(account)]]" tabindex="-1">
+ <a href$="[[_computeOwnerLink(account)]]">
<gr-account-label
show-attention="[[showAttention]]"
hide-avatar="[[hideAvatar]]"
diff --git a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_html.js b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_html.js
index 8378de5..9fc83c8 100644
--- a/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-copy-clipboard/gr-copy-clipboard_html.js
@@ -79,6 +79,7 @@
class="copyToClipboard"
title="[[buttonTitle]]"
on-click="_copyToClipboard"
+ aria-label="Click to copy to clipboard"
>
<iron-icon id="icon" icon="gr-icons:content-copy"></iron-icon>
</gr-button>
diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_html.js b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_html.js
index 2a86669..1722d02 100644
--- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info_html.js
@@ -109,7 +109,7 @@
<td>
<gr-button
link=""
- aria-label="Remove"
+ aria-label="Remove vote"
on-click="_onDeleteVote"
tooltip="Remove vote"
data-account-id$="[[mappedLabel.account._account_id]]"
