Page MenuHomePhorge

Prevent Line Numbers in Diffusion being copied as Tabs
AcceptedPublic

Authored by Leon95 on Oct 10 2021, 14:59.

Details

Reviewers
speck
Ekubischta
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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
Branch
arcpatch-D25024
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 53
Build 53: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 10 2021, 14:59
Leon95 requested review of this revision.Oct 10 2021, 14:59

I am not seeing this behavior. (Using Edge)

I did this

  1. When to rP repo in diffusion
  2. Clicked on .arcconfig file https://we.phorge.it/source/phorge/browse/master/.arcconfig
  3. Copied all of the lines
  4. 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?

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83

Honestly I have no idea. I am not an css expert.

webroot/rsrc/css/layout/phabricator-source-code-view.css
79–83

Looks like Safari still requires the vendor prefix

image.png (536×1 px, 101 KB)

Chrome supports without vendor prefix as of about 2016, Edge as of 2018

We should keep the webkit one for sure...

speck requested changes to this revision.Oct 12 2021, 22:23

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?

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 -

  • Add browser-specific css flags

Since it matches the other user select that’s setup in this file this is fine but it could just be the standard + WebKit one

webroot/rsrc/css/layout/phabricator-source-code-view.css
30

I think we determined that the only additional line is this WebKit one

This revision is now accepted and ready to land.Oct 15 2021, 19:17

No Problem. Lets drop the other Flags then.

  • Use css flag for webkit