Imrove scroll behavior to hash issuecomment(scroll position, auto expand if file is folded, and on refreshing) (#23513) (#23540)

Backport #23513 by @HesterG

Close #23466
Right now on pull request "files Changed" tab, if a file is viewed, when
the comments' links are visited, the comment will not be shown as the
file is folded after viewed. This PR is to improve the behavior, to make
the comment seen even the related file is folded, like on github.

And right now scroll position will be remembered and hence it won’t
scroll to hashed comment after refreshing, this PR also adjust the
scroll position remembering behavior: When there is hash comment in url,
do not remember the scroll position.

Before:


https://user-images.githubusercontent.com/17645053/225512079-6cf79581-9346-44cf-95d6-06919642e6a8.mov


After:


https://user-images.githubusercontent.com/17645053/225523753-3f6728f2-977b-4ed0-a65c-63dcef2ace80.mov


Update - long comment's behavior after using `scrollTop ` (Comment div
scroll to the position which is 30px below the diff header, or 30px
below top on conversation tab):


https://user-images.githubusercontent.com/17645053/225614460-0602c1a6-229c-41f4-84d2-334e78251486.mov

Co-authored-by: Hester Gong <hestergong@gmail.com>

modules/git/command.go

@@ -179,7 +179,7 @@ func (c *Command) AddDashesAndList(list ...string) *Command {
 }
 
 // ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs
-// In most cases, it shouldn't be used. Use AddXxx function instead
+// In most cases, it shouldn't be used. Use NewCommand().AddXxx() function instead
 func ToTrustedCmdArgs(args []string) TrustedCmdArgs {
 	ret := make(TrustedCmdArgs, len(args))
 	for i, arg := range args {

services/pull/merge.go

@@ -332,8 +332,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
 }
 
 func commitAndSignNoAuthor(ctx *mergeContext, message string) error {
-	if err := git.NewCommand(ctx, "commit").AddArguments(ctx.signArg...).AddOptionFormat("--message=%s", message).
-		Run(ctx.RunOpts()); err != nil {
+	cmdCommit := git.NewCommand(ctx, "commit").AddOptionFormat("--message=%s", message)
+	if ctx.signKeyID == "" {
+		cmdCommit.AddArguments("--no-gpg-sign")
+	} else {
+		cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+	}
+	if err := cmdCommit.Run(ctx.RunOpts()); err != nil {
 		log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 		return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
 	}

services/pull/merge_prepare.go

@@ -28,7 +28,7 @@ type mergeContext struct {
 	doer      *user_model.User
 	sig       *git.Signature
 	committer *git.Signature
-	signArg   git.TrustedCmdArgs
+	signKeyID string // empty for no-sign, non-empty to sign
 	env       []string
 }
 
@@ -85,12 +85,10 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
 	// Determine if we should sign
 	sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch)
 	if sign {
-		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"-S" + keyID})
+		mergeCtx.signKeyID = keyID
 		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
 			mergeCtx.committer = signer
 		}
-	} else {
-		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
 	}
 
 	commitTimeStr := time.Now().Format(time.RFC3339)
@@ -136,18 +134,11 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
 		return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", err)
 	}
 
-	gitConfigCommand := func() *git.Command {
-		return git.NewCommand(ctx, "config", "--local")
-	}
-
 	setConfig := func(key, value string) error {
-		if err := gitConfigCommand().AddArguments(git.ToTrustedCmdArgs([]string{key, value})...).
+		if err := git.NewCommand(ctx, "config", "--local").AddDynamicArguments(key, value).
 			Run(ctx.RunOpts()); err != nil {
-			if value == "" {
-				value = "<>"
-			}
-			log.Error("git config [%s -> %s ]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git config [%s -> %s ]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+			log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+			return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
 		}
 		ctx.outbuf.Reset()
 		ctx.errbuf.Reset()

services/pull/merge_squash.go

@@ -14,8 +14,8 @@ import (
 
 // doMergeStyleSquash squashes the tracking branch on the current HEAD (=base)
 func doMergeStyleSquash(ctx *mergeContext, message string) error {
-	cmd := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
-	if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmd); err != nil {
+	cmdMerge := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch)
+	if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmdMerge); err != nil {
 		log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err)
 		return err
 	}
@@ -25,27 +25,21 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error {
 		return fmt.Errorf("LoadPoster: %w", err)
 	}
 	sig := ctx.pr.Issue.Poster.NewGitSig()
-	if len(ctx.signArg) == 0 {
-		if err := git.NewCommand(ctx, "commit").
-			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
-			AddOptionFormat("--message=%s", message).
-			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
-		}
+	if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
+		// add trailer
+		message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
+	}
+	cmdCommit := git.NewCommand(ctx, "commit").
+		AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
+		AddOptionFormat("--message=%s", message)
+	if ctx.signKeyID == "" {
+		cmdCommit.AddArguments("--no-gpg-sign")
 	} else {
-		if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() {
-			// add trailer
-			message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String())
-		}
-		if err := git.NewCommand(ctx, "commit").
-			AddArguments(ctx.signArg...).
-			AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email).
-			AddOptionFormat("--message=%s", message).
-			Run(ctx.RunOpts()); err != nil {
-			log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
-			return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
-		}
+		cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID)
+	}
+	if err := cmdCommit.Run(ctx.RunOpts()); err != nil {
+		log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+		return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, err, ctx.outbuf.String(), ctx.errbuf.String())
 	}
 	ctx.outbuf.Reset()
 	ctx.errbuf.Reset()

templates/base/head_navbar.tmpl

@@ -1,4 +1,4 @@
-<div class="ui container" id="navbar" role="navigation" aria-label="{{.locale.Tr "aria.navbar"}}">
+<nav class="ui container" id="navbar" aria-label="{{.locale.Tr "aria.navbar"}}">
 	{{$notificationUnreadCount := 0}}
 	{{if .IsSigned}}
 		{{if .NotificationUnreadCount}}{{$notificationUnreadCount = call .NotificationUnreadCount}}{{end}}
@@ -150,7 +150,7 @@
 				</div><!-- end content create new menu -->
 			</div><!-- end dropdown menu create new -->
 
-			<div class="ui dropdown jump item tooltip gt-mx-0" tabindex="-1" data-content="{{.locale.Tr "user_profile_and_more"}}">
+			<div class="ui dropdown jump item tooltip gt-mx-0" data-content="{{.locale.Tr "user_profile_and_more"}}">
 				<span class="text">
 					{{avatar $.Context .SignedUser 24 "tiny"}}
 					<span class="sr-only">{{.locale.Tr "user_profile_and_more"}}</span>
@@ -190,14 +190,14 @@
 
 						<a class="{{if .PageIsAdmin}}active {{end}}item" href="{{AppSubUrl}}/admin">
 							{{svg "octicon-server"}}
-							{{.locale.Tr "admin_panel"}}<!-- Admin Panel -->
+							{{.locale.Tr "admin_panel"}}
 						</a>
 					{{end}}
 
 					<div class="divider"></div>
 					<a class="item link-action" href data-url="{{AppSubUrl}}/user/logout" data-redirect="{{AppSubUrl}}/">
 						{{svg "octicon-sign-out"}}
-						{{.locale.Tr "sign_out"}}<!-- Sign Out -->
+						{{.locale.Tr "sign_out"}}
 					</a>
 				</div><!-- end content avatar menu -->
 			</div><!-- end dropdown avatar menu -->
@@ -213,6 +213,6 @@
 			<a class="item{{if .PageIsSignIn}} active{{end}}" rel="nofollow" href="{{AppSubUrl}}/user/login{{if not .PageIsSignIn}}?redirect_to={{.CurrentURL}}{{end}}">
 				{{svg "octicon-sign-in"}} {{.locale.Tr "sign_in"}}
 			</a>
-		</div><!-- end anonymous right menu -->
+		</div><!-- end anonymous user right menu -->
 	{{end}}
-</div>
+</nav>

templates/repo/commit_page.tmpl

@@ -216,7 +216,7 @@
 				<div class="gt-df gt-ac">
 					{{if .Verification.Verified}}
 						{{if ne .Verification.SigningUser.ID 0}}
-							{{svg "octicon-shield-check" 16 "gt-mr-3"}}
+							{{svg "octicon-verified" 16 "gt-mr-3"}}
 							{{if .Verification.SigningSSHKey}}
 								<span class="ui text gt-mr-3">{{.locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
 								{{.Verification.SigningSSHKey.Fingerprint}}
@@ -225,7 +225,7 @@
 								{{.Verification.SigningKey.PaddedKeyID}}
 							{{end}}
 						{{else}}
-							{{svg "octicon-shield-lock" 16 "gt-mr-3"}}
+							{{svg "octicon-unverified" 16 "gt-mr-3"}}
 							{{if .Verification.SigningSSHKey}}
 								<span class="ui text gt-mr-3 tooltip" data-content="{{.locale.Tr "gpg.default_key"}}">{{.locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
 								{{.Verification.SigningSSHKey.Fingerprint}}
@@ -235,7 +235,7 @@
 							{{end}}
 						{{end}}
 					{{else if .Verification.Warning}}
-						{{svg "octicon-shield" 16 "gt-mr-3"}}
+						{{svg "octicon-unverified" 16 "gt-mr-3"}}
 						{{if .Verification.SigningSSHKey}}
 							<span class="ui text gt-mr-3">{{.locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
 							{{.Verification.SigningSSHKey.Fingerprint}}
@@ -246,14 +246,14 @@
 					{{else}}
 						{{if .Verification.SigningKey}}
 							{{if ne .Verification.SigningKey.KeyID ""}}
-								{{svg "octicon-shield" 16 "gt-mr-3"}}
+								{{svg "octicon-verified" 16 "gt-mr-3"}}
 								<span class="ui text gt-mr-3">{{.locale.Tr "repo.commits.gpg_key_id"}}:</span>
 								{{.Verification.SigningKey.PaddedKeyID}}
 							{{end}}
 						{{end}}
 						{{if .Verification.SigningSSHKey}}
 							{{if ne .Verification.SigningSSHKey.Fingerprint ""}}
-								{{svg "octicon-shield" 16 "gt-mr-3"}}
+								{{svg "octicon-verified" 16 "gt-mr-3"}}
 								<span class="ui text gt-mr-3">{{.locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
 								{{.Verification.SigningSSHKey.Fingerprint}}
 							{{end}}

templates/repo/diff/box.tmpl

@@ -24,7 +24,7 @@
 					{{svg "octicon-diff" 16 "gt-mr-2"}}{{.locale.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion | Str2html}}
 				</div>
 			</div>
-			<div class="diff-detail-actions gt-df gt-ac gt-w-100">
+			<div class="diff-detail-actions gt-df gt-ac">
 				{{if and .PageIsPullFiles $.SignedUserID (not .IsArchived)}}
 					<progress id="viewed-files-summary" class="gt-mr-2" value="{{.Diff.NumViewedFiles}}" max="{{.Diff.NumFiles}}"></progress>
 					<label for="viewed-files-summary" id="viewed-files-summary-label" class="gt-mr-3 gt-f1" data-text-changed-template="{{.locale.Tr "repo.pulls.viewed_files_label"}}">
@@ -161,7 +161,8 @@
 									{{end}}
 								</div>
 								{{if $showFileViewToggle}}
-									<div id="diff-rendered-{{$file.NameHash}}" class="file-body file-code {{if $.IsSplitStyle}} code-diff-split{{else}} code-diff-unified{{end}}">
+									{{/* for image or CSV, it can have a horizontal scroll bar, there won't be review comment context menu (position absolute) which would be clipped by "overflow" */}}
+									<div id="diff-rendered-{{$file.NameHash}}" class="file-body file-code {{if $.IsSplitStyle}}code-diff-split{{else}}code-diff-unified{{end}} gt-overflow-x-scroll">
 										<table class="chroma gt-w-100">
 											{{if $isImage}}
 												{{template "repo/diff/image_diff" dict "file" . "root" $ "blobBase" $blobBase "blobHead" $blobHead}}

templates/repo/diff/new_review.tmpl

@@ -4,7 +4,7 @@
 		<span class="ui small label review-comments-counter" data-pending-comment-number="{{.PendingCodeCommentNumber}}">{{.PendingCodeCommentNumber}}</span>
 		{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 	</button>
-	<div class="review-box-panel gt-hidden">
+	<div class="review-box-panel tippy-target">
 		<div class="ui segment">
 			<form class="ui form" action="{{.Link}}/reviews/submit" method="post">
 			{{.CsrfTokenHtml}}

templates/user/settings/keys_gpg.tmpl

@@ -58,7 +58,7 @@
 				</div>
 				<div class="content">
 					{{if .Verified}}
-						<span class="tooltip" data-content="{{$.locale.Tr "settings.gpg_key_verified_long"}}">{{svg "octicon-shield-check"}} <strong>{{$.locale.Tr "settings.gpg_key_verified"}}</strong></span>
+						<span class="tooltip" data-content="{{$.locale.Tr "settings.gpg_key_verified_long"}}">{{svg "octicon-verified"}} <strong>{{$.locale.Tr "settings.gpg_key_verified"}}</strong></span>
 					{{end}}
 					{{if gt (len .Emails) 0}}
 						<span class="tooltip" data-content="{{$.locale.Tr "settings.gpg_key_matched_identities_long"}}">{{svg "octicon-mail"}} {{$.locale.Tr "settings.gpg_key_matched_identities"}} {{range .Emails}}<strong>{{.Email}} </strong>{{end}}</span>

templates/user/settings/keys_ssh.tmpl

@@ -51,7 +51,7 @@
 				</div>
 				<div class="content">
 						{{if .Verified}}
-							<span class="tooltip" data-content="{{$.locale.Tr "settings.ssh_key_verified_long"}}">{{svg "octicon-shield-check"}} <strong>{{$.locale.Tr "settings.ssh_key_verified"}}</strong></span>
+							<span class="tooltip" data-content="{{$.locale.Tr "settings.ssh_key_verified_long"}}">{{svg "octicon-verified"}} <strong>{{$.locale.Tr "settings.ssh_key_verified"}}</strong></span>
 						{{end}}
 						<strong>{{.Name}}</strong>
 						<div class="print meta">

web_src/css/base.css

@@ -1231,7 +1231,7 @@ a.ui.card:hover,
 /* enable fluid page widths for medium size viewports */
 @media (min-width: 768px) and (max-width: 1200px) {
   .ui.ui.ui.container:not(.fluid) {
-    width: calc(100vw - 3em);
+    width: calc(100vw - 64px);
   }
 }
 
@@ -1285,7 +1285,14 @@ a.ui.card:hover,
 .following.bar #navbar {
   width: 100vw;
   min-height: 52px;
-  padding: 0 0.5rem;
+  padding: 0 16px;
+}
+
+@media (max-width: 767px) {
+  .following.bar #navbar {
+    padding-left: 4px;
+    padding-right: 0;
+  }
 }
 
 .following.bar #navbar .brand {
@@ -1491,7 +1498,7 @@ a.ui.card:hover,
 }
 
 .ui.container.fluid.padded {
-  padding: 0 10px;
+  padding: 0 32px;
 }
 
 .ui.form .ui.button {

web_src/css/helpers.css

@@ -22,6 +22,7 @@
 /* below class names match Tailwind CSS */
 .gt-pointer-events-none { pointer-events: none !important; }
 .gt-relative { position: relative !important; }
+.gt-overflow-x-scroll { overflow-x: scroll !important; }
 
 .gt-mono {
   font-family: var(--fonts-monospace) !important;

web_src/css/repository.css

@@ -3337,10 +3337,6 @@ td.blob-excerpt {
   min-width: 100px;
 }
 
-.diff-file-body {
-  overflow-x: scroll;
-}
-
 .diff-stats-bar {
   display: inline-block;
   background-color: var(--color-red);

web_src/css/review.css

@@ -214,6 +214,10 @@ a.blob-excerpt:hover {
   color: var(--color-primary-contrast);
 }
 
+.review-box-panel .ui.segment {
+  border: none;
+}
+
 /* See the comment of createCommentEasyMDE() for the review editor */
 /* EasyMDE's options can not handle minHeight & maxHeight together correctly, we have to set minHeight in JS code */
 .review-box-panel .CodeMirror-scroll {
@@ -249,14 +253,6 @@ a.blob-excerpt:hover {
   position: relative;
 }
 
-.review-box-panel {
-  position: absolute;
-  min-width: max-content;
-  top: 45px;
-  right: -5px;
-  z-index: 2;
-}
-
 #review-box .review-comments-counter {
   background-color: var(--color-primary-light-4);
   color: var(--color-primary-contrast);

web_src/js/features/repo-issue.js

@@ -4,8 +4,9 @@ import {attachTribute} from './tribute.js';
 import {createCommentEasyMDE, getAttachedEasyMDE} from './comp/EasyMDE.js';
 import {initEasyMDEImagePaste} from './comp/ImagePaste.js';
 import {initCompMarkupContentPreviewTab} from './comp/MarkupContentPreview.js';
-import {initTooltip, showTemporaryTooltip} from '../modules/tippy.js';
+import {initTooltip, showTemporaryTooltip, createTippy} from '../modules/tippy.js';
 import {hideElem, showElem, toggleElem} from '../utils/dom.js';
+import {setFileFolding} from './file-fold.js';
 
 const {appSubUrl, csrfToken} = window.config;
 
@@ -436,17 +437,36 @@ export async function handleReply($el) {
 
 export function initRepoPullRequestReview() {
   if (window.location.hash && window.location.hash.startsWith('#issuecomment-')) {
+    // set scrollRestoration to 'manual' when there is a hash in url, so that the scroll position will not be remembered after refreshing
+    if (window.history.scrollRestoration !== 'manual') {
+      window.history.scrollRestoration = 'manual';
+    }
     const commentDiv = $(window.location.hash);
     if (commentDiv) {
       // get the name of the parent id
       const groupID = commentDiv.closest('div[id^="code-comments-"]').attr('id');
       if (groupID && groupID.startsWith('code-comments-')) {
         const id = groupID.slice(14);
+        const ancestorDiffBox = commentDiv.closest('.diff-file-box');
+        // on pages like conversation, there is no diff header
+        const diffHeader = ancestorDiffBox.find('.diff-file-header');
+        // offset is for scrolling
+        let offset = 30;
+        if (diffHeader[0]) {
+          offset += $('.diff-detail-box').outerHeight() + diffHeader.outerHeight();
+        }
         $(`#show-outdated-${id}`).addClass('gt-hidden');
         $(`#code-comments-${id}`).removeClass('gt-hidden');
         $(`#code-preview-${id}`).removeClass('gt-hidden');
         $(`#hide-outdated-${id}`).removeClass('gt-hidden');
-        commentDiv[0].scrollIntoView();
+        // if the comment box is folded, expand it
+        if (ancestorDiffBox.attr('data-folded') && ancestorDiffBox.attr('data-folded') === 'true') {
+          setFileFolding(ancestorDiffBox[0], ancestorDiffBox.find('.fold-file')[0], false);
+        }
+        window.scrollTo({
+          top: commentDiv.offset().top - offset,
+          behavior: 'instant'
+        });
       }
     }
   }
@@ -491,12 +511,23 @@ export function initRepoPullRequestReview() {
     return;
   }
 
-  $('.js-btn-review').on('click', function (e) {
+  const $reviewBtn = $('.js-btn-review');
+  const $panel = $reviewBtn.parent().find('.review-box-panel');
+  const $closeBtn = $panel.find('.close');
+
+  const tippy = createTippy($reviewBtn[0], {
+    content: $panel[0],
+    placement: 'bottom',
+    trigger: 'click',
+    role: 'menu',
+    maxWidth: 'none',
+    interactive: true,
+    hideOnClick: true,
+  });
+
+  $closeBtn.on('click', (e) => {
     e.preventDefault();
-    toggleElem($(this).parent().find('.review-box-panel'));
-  }).parent().find('.review-box-panel .close').on('click', function (e) {
-    e.preventDefault();
-    hideElem($(this).closest('.review-box-panel'));
+    tippy.hide();
   });
 
   $(document).on('click', 'a.add-code-comment', async function (e) {