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

fix extra newlines when copying from diff in Firefox #7288

Merged
merged 19 commits into from
Jun 26, 2019

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jun 24, 2019

Followup on #7279 to fix copy from diff in Firefox.

First, the <pre><code> seems to add a forced newline that is not possible to get rid of via CSS, so I replaced it with just a <code>. Secondly, .lines-type-marker also forced a newline in the copied text, but that was possible to get rid of via user-select: none.

This should fix it for Firefox and Chrome. Safari still has a extraneous newline in the copied text of unknown origin, but this should not block stop this PR.

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1273836

See https://bugzilla.mozilla.org/show_bug.cgi?id=1273836

Basically, the <pre><code> seems to add a forced newline that is not
possible to get rid of via CSS, so I replaced it with just a <code>.

Secondly, .lines-type-marker also forced a newline in the copied text,
but that was possible to get rid of via user-select.

Safari still has a extraneous newline in the copied text of unknown
origin, but this should not block stop this PR.
@silverwind silverwind changed the title fix extra newlines when copying from diff fix extra newlines when copying from diff in Firefox Jun 24, 2019
@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #7288 into master will decrease coverage by <.01%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7288      +/-   ##
==========================================
- Coverage   41.24%   41.24%   -0.01%     
==========================================
  Files         464      464              
  Lines       62841    62846       +5     
==========================================
+ Hits        25917    25918       +1     
- Misses      33533    33537       +4     
  Partials     3391     3391
Impacted Files Coverage Δ
models/git_diff.go 68.34% <84.61%> (+0.29%) ⬆️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/repo_list.go 72.08% <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 edc94c7...3c17bb4. 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 24, 2019
@mrsdizzie
Copy link
Member

Hmm this PR makes the diff look different and not alligned in Safari/Firefox/Chrome for me:

Before:
Screen Shot 2019-06-24 at 6 55 57 PM
After:
Screen Shot 2019-06-24 at 6 55 27 PM

@silverwind
Copy link
Member Author

Fixed that by removing the <pre> in the types marker. I had removed the rule that removes that inherent <pre> margin, forgetting that the types marker still needed it. I don't see a reason for it to be <pre> (there is no whitespace in it), so simplified it to just a <code>.

This removes the extra newlines that Safari copies as well. Safari still has a issue with weird indendation being copied, thought.

@zeripath
Copy link
Contributor

I had assumed that the pre was there for a reason that's why I left it in.

@silverwind
Copy link
Member Author

silverwind commented Jun 25, 2019

<pre> is only needed if you want to preserve whitespace, and even then, you can get the same effect via white-space: pre (GitHub actually does that). We already have a similiar white-space: pre-wrap on the <code>, so it's sufficient.

@silverwind
Copy link
Member Author

Noticed an issue with content being absent in the split diff, will fix that later and also investigate Safari.

@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 25, 2019
@zeripath
Copy link
Contributor

Damn it's not copying the data-line-num either.

@zeripath
Copy link
Contributor

OK fixed that too. I'll stop hammering your PR now.

@silverwind
Copy link
Member Author

silverwind commented Jun 25, 2019

Wow, good find. Would never have imagined we would have sneaky JS embedded in template code. Very bad practice imho.

I will give this PR another run of tests tomorrow, pretty sure a few fixups will be needed, so please don't merge yet, unless this fix is deemed critical (for a release).

@mrsdizzie
Copy link
Member

Won't approve yet based on last comment, but my initial padding/alignment issue seems fixed and all of the fixes for the split view seem OK and that appears to be working as it was before. Also agree a real "wtf" for this template.

@zeripath
Copy link
Contributor

Yeah that's probably why I missed it. That the comment box template is also stored in index.is is also unexpected too.

It should not be this fiddly to adjust the diff format! Diff was already on my naughty list, but it might just have risen a bit higher.

Once again I'm so sorry for messing this up. I really did not expect it to be so fragile!

@silverwind
Copy link
Member Author

silverwind commented Jun 26, 2019

Fixed that Safari copy/paste issue. Turns out it was tripping on the whitespace between HTML tags which I've now removed. It makes the template a bit uglier to read but it unfortunately is neccesary. Does not look like go template has any built-in way to auto-strip whitespace around tags.

So this PR should be ready for review. It contains a important fix for the split diff among the copy/paste fixes for Firefox and Safari.

@mrsdizzie
Copy link
Member

It seems to not copy any newlines now, even intentional ones:

So a diff like this:

+ echo test
+
+ echo test
+
+ echo test

Would get copied as

echo test
echo test
echo test

@silverwind
Copy link
Member Author

You're right. It seems we need to render a <br> like GitHub on empty lines for that to work.

@silverwind
Copy link
Member Author

@mrsdizzie should be fixed as of last commit.

@zeripath
Copy link
Contributor

zeripath commented Jun 26, 2019

Damn I think https://try.gitea.io/silverwind/symlink-test/commit/f2f2a03c79d7b2ab89785fca78c2f85d123b7d78?style=split is broke again.

I'm being a muppet!

@zeripath
Copy link
Contributor

@silverwind thank you so much for fixing my idiotic broken PR. I'm so sorry once again for so many problems. I can't understand how I missed how broken it was.

@zeripath zeripath added this to the 1.9.0 milestone Jun 26, 2019
@zeripath zeripath added type/bug topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile labels Jun 26, 2019
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.

Looks good thanks again @silverwind for going beyond the initial scope of this to help fix everything!

@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 26, 2019
@zeripath
Copy link
Contributor

@silverwind are you happy with my last change. I think we will be able to merge if so.

@silverwind
Copy link
Member Author

silverwind commented Jun 26, 2019

I'm happy if @mrsdizzie is happy 🤣

In hindsight, it would've probably been easier if you had opened a separate PR for those fixes, thought on the other hand, it was kind of convenient to have the fixes in the branch 😉

@zeripath zeripath merged commit da23041 into go-gitea:master Jun 26, 2019
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
* fix extra newlines when copying from diff

See https://bugzilla.mozilla.org/show_bug.cgi?id=1273836

Basically, the <pre><code> seems to add a forced newline that is not
possible to get rid of via CSS, so I replaced it with just a <code>.

Secondly, .lines-type-marker also forced a newline in the copied text,
but that was possible to get rid of via user-select.

Safari still has a extraneous newline in the copied text of unknown
origin, but this should not block stop this PR.

* simplify .line-type-marker

* fix selector

* remove erronous ^^^

* Fix empty split diff

* Fix arc-theme-green

* fix add comment

* ensure line-num is copied too

* Update templates/repo/diff/box.tmpl

Co-Authored-By: zeripath <art27@cantab.net>

* attempt to fix safari via removing <code>

* remove useless whitespace at the end of 'class'

* remove inter-tag whitespace for code <td>s

* more inter-tag removal

* final inter-tag removal

* attempt to fix empty line copy

* move and comment getLineContent

* fix golint

* make background grey for missing added code
@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. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants