Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make diff line-marker non-selectable #7279

Merged

Conversation

zeripath
Copy link
Contributor

This PR makes the first character of the diff line non-selectable.

Fixes #4076

@codecov-io
Copy link

codecov-io commented Jun 22, 2019

Codecov Report

Merging #7279 into master will decrease coverage by 0.01%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7279      +/-   ##
==========================================
- Coverage   41.17%   41.15%   -0.02%     
==========================================
  Files         464      464              
  Lines       62771    62763       -8     
==========================================
- Hits        25846    25832      -14     
  Misses      33537    33537              
- Partials     3388     3394       +6
Impacted Files Coverage Δ
models/git_diff.go 68.04% <70%> (-0.45%) ⬇️
models/issue_list.go 66.06% <0%> (-1.78%) ⬇️
models/repo_list.go 72.08% <0%> (-1.02%) ⬇️
models/issue.go 48.74% <0%> (+0.08%) ⬆️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baefea3...c8d8539. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 22, 2019
@lunny lunny added the type/bug label Jun 23, 2019
@lunny lunny added this to the 1.9.0 milestone Jun 23, 2019
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 23, 2019
@mrsdizzie
Copy link
Member

While testing this I found out that while webkit obeys the "don't select" part, it still copies the contents if something around it has been selected:

Screen Shot 2019-06-22 at 11 04 00 PM

Then the copied text is:

# repo38
2	
+
3	
+Description for repo38

This appears to be an old issue unfortunately, https://bugs.webkit.org/show_bug.cgi?id=80159

I don't have this issue on Github or other similar sites, so I tried to see what was different:

It appears Github gets this to work through adding the line numbers and +/- using a pseudo class, like:

.blob-num:before {
content: attr(data-line-number);
}

Probably similar to what @silverwind was mentioning in the linked Issue.

I tried to create a really small example of how this might look to see if it would work on Safari and it does seem to:

https://codepen.io/mrsdizzie/pen/agWNwL

Knowledge of HTML/CSS is pretty limited though

@zeripath
Copy link
Contributor Author

@mrsdizzie OK done.

@@ -465,6 +465,7 @@ footer .ui.left,footer .ui.right{line-height:40px}
.repository.file.list .non-diff-file-content .code-view table{width:100%}
.repository.file.list .non-diff-file-content .code-view .lines-num{vertical-align:top;text-align:right;color:#999;background:#f5f5f5;width:1%;-webkit-user-select:none;-moz-user-select:none;-ms-user-select:none;user-select:none}
.repository.file.list .non-diff-file-content .code-view .lines-num span{line-height:20px;padding:0 10px;cursor:pointer;display:block}
.repository.file.list .non-diff-file-content .code-view .lines-num[data-line-num]::before{content:attr(data-line-num);display:inline-block}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have been in .diff-file-box not .non-diff-file-content ? Or maybe it belongs in both? I don't see the line numbers in a commit diff with the latest push

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn you're right. Don't know how I missed that.

@mrsdizzie
Copy link
Member

Aside from the comment above about line numbers the copy/paste now works in Safari, thanks!!

Also this is totally non blocking but since you are changing it:

I think it would nice if the +/- were in their own as well, since it would/could give a bit more padding like in most the diffs here on Github (but not the comments on diffs strangely). Also on Safari when it is part of the same content it still lets you visually select the +/- chars on lines that also have selectable text, which might make people think that it will copy them as well.

It doesn't actually copy them any more so the real problem is solved and that is just a slight cosmetic issue at that point. Could be done in a separate PR if needed though.

@zeripath
Copy link
Contributor Author

OK @mrsdizzie I think that's done

Copy link
Member

@mrsdizzie mrsdizzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks great!

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 24, 2019
@zeripath
Copy link
Contributor Author

make lg-tm work

@zeripath zeripath merged commit 5908bb1 into go-gitea:master Jun 24, 2019
@zeripath zeripath deleted the fix-4076-make-line-marker-none-selectable branch June 24, 2019 20:23
@silverwind
Copy link
Member

Thanks, the +/- issue looks solved nicely. In Firefox, I do observe extraneous newlines being copied (4 newlines for each actual newline). Probably need to remove some whitespace inside the HTML tags holding the code lines.

@zeripath
Copy link
Contributor Author

Hmm is that new? I haven't actually changed the template part that renders the code line.

@silverwind
Copy link
Member

silverwind commented Jun 24, 2019

I think previously it was 3 newlines per line, with the introduction of .lines-type-marker it became 4. #7288 should solve it at least for Firefox.

jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
* Make diff line-marker non-selectable
* Move to use data-* as per @mrsdizzie
* fix missing line nums
* Add a minimum-width to force right-align of the line num
* Move line-type-marker into separate column
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy/paste issues with diffs
6 participants