Details

Reviewers

speck
Ekubischta

Group Reviewers

O1: Blessed Committers

Commits

rP69b2710af9c0: Prevent Line Numbers in Diffusion being copied as Tabs

Summary

Marks Line Numbers in Diffusion File Preview as unselectable

Test Plan

Select multiple Lines from a File Preview in Diffusion
Copy them into a Text Editor
The Leading Tabs should no longer included

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Leon95 created this revision.Oct 10 2021, 14:59

Owners added a reviewer: O1: Blessed Committers.Oct 10 2021, 14:59

Herald added subscribers: valerio.bozzolan, tobiaswiese, speck. · View Herald TranscriptOct 10 2021, 14:59

Leon95 requested review of this revision.Oct 10 2021, 14:59

Harbormaster completed remote builds in B48: Diff 68.Oct 10 2021, 14:59

I am not seeing this behavior. (Using Edge)

I did this

When to rP repo in diffusion
Clicked on .arcconfig file https://we.phorge.it/source/phorge/browse/master/.arcconfig
Copied all of the lines
Pasted into notepad.

I did not get tabs

l got this behaviour using Firefox on a Linux system. I have no Windows at Hand Right now, but will check it tomorow with different browsers.

Happens with Firefox on Windows too. Chome and Edge are not including tabs, but when selecting from bottom to top and ending the selection while hovering a line number, one tab gets copied. This is also prevented with this patch.

I tried on Firefox on mac and there are tabs included

Screen Shot 2021-10-12 at 3.30.44 PM.png (368×764 px, 29 KB)

I think this change makes sense to me.

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83	Should we be using these browser-specific flags or are these ancient?

Leon95 added inline comments.Oct 12 2021, 21:32

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83	Honestly I have no idea. I am not an css expert.

Ekubischta added inline comments.Oct 12 2021, 22:13

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83	Looks like Safari still requires the vendor prefix Chrome supports without vendor prefix as of about 2016, Edge as of 2018 We should keep the webkit one for sure...

Let’s update to include the same set of user-select cross-browser as the blame info

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83	Thanks for checking. My question is aimed at being consistent and not necessarily updating this css here, but making sure the user-select addition above is accounting for the same compatibility.

This revision now requires changes to proceed.Oct 12 2021, 22:23

I tested this revision on Edge just to make sure there were no negative side effects - No issues to report.

It seems like only the WebKit variant may still be necessary. What do you think about updating both areas to just have the WebKit version in addition to the standard?

Things seem wildly inconsistent for this particular css string https://we.phorge.it/source/phorge/browse/master/?grep=user-select

Ah interesting. Could we follow the information you found and just include the WebKit one for now. I’m not as concerned with consistency after your findings. I appreciate that you investigated!

@Leon95 - I think this revision will be approved with just including the webkit alternate -