Bug 27769 - Advanced editor shouldn't break copying selected text with Ctrl+C
Summary: Advanced editor shouldn't break copying selected text with Ctrl+C
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on: 17179
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-24 04:11 UTC by Phil Ringnalda
Modified: 2024-02-20 23:02 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
TODO: relevant stuff in comment 11
Version(s) released in:


Attachments
Bug 27769: Advanced editor shouldn't break copying selected text with Ctrl+C (2.07 KB, patch)
2021-02-24 04:39 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 27769: Update default key mapping to copy text (1.59 KB, patch)
2024-02-07 17:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27769: Remove 'brief' class from advanced editor clipboard (1.29 KB, patch)
2024-02-07 17:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27769: Update default key mapping to copy text (1.63 KB, patch)
2024-02-07 21:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 27769: Remove 'brief' class from advanced editor clipboard (1.34 KB, patch)
2024-02-07 21:56 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2021-02-24 04:11:10 UTC
Bug 17179 added a shortcut for Ctrl+C to copy the line where the cursor is into the advanced editor's clipboard div. That only works if you don't have text selected.

If you have text selected, the shortcut silently does nothing, but Ctrl+C, which I've used for decades to copy selected text, doesn't copy the text. Dozens of times a day, I copy the OCLC control number from a record, or the contents of an 024, do an advanced search for that number (where I can no longer actually see what record I'm replacing), and replace the record. Despite having now spent months trying to train myself that Ctrl+C has been broken (difficult training since I catalog both on a Mac, where Cmd+C has not been broken, and on Windows where it has), every couple of weeks I catch myself having just replaced a record with some record I had previously successfully right-click copied data from. How often do I *not* catch myself, and replace a record with a record for a completely different bib? Dunno, I'd really rather not think about that.

The keyhandler looks like it'a a CodeMirror command, and the very scanty documentation says that a CodeMirror key handling command can return CodeMirror.Pass to let other handlers or the default behavior handle the key. Currently the handler does "return true" when it sees selected text and decides not to handle Ctrl+C.

If instead returning cm.Pass would make the system clipboard work when text is selected, and the advanced editor clipboard work when no text is selected, that would be the best outcome.

Second best would be deciding that silently causing the system clipboard to fail was a bad idea, and switching the default shortcut to something else. Yes, there is a system-wide option to change keys. For me, that means getting 14 people to agree on a different key, then starting the process of persuading an admin to make the change.
Comment 1 Phil Ringnalda 2021-02-24 04:39:06 UTC
Created attachment 117236 [details] [review]
Bug 27769: Advanced editor shouldn't break copying selected text with Ctrl+C

When text is selected, the editor keyboard shortcut handler intentionally
doesn't copy the line into the editor's clipboard, but also breaks the
system clipboard copying it on Windows and Linux where Ctrl+C is copy.
Returning CodeMirror.Pass should let the system copy when text is selected.

Test plan:
1. Set the system preference EnableAdvancedCatalogingEditor to Enable
2. Cataloging - Advanced editor
3. Type some text in 245 subfield a
4. Select the text you typed, and type Ctrl+C
5. Verify that the entire line was not copied to the advanced editor's
   Clipboard div in the lower left corner of the page
6. In another app or in one of the Search inputs, Ctrl+V to paste from the
   system clipboard, verify that you pasted the text you selected in 245$a
7. Clear your text selection, but put the text insertion point into the 245
   and type Ctrl+C, and verify that the entire line was copied into the
   editor's Clipboard div in the lower left
Comment 2 Phil Ringnalda 2021-02-24 04:55:56 UTC
Comment on attachment 117236 [details] [review]
Bug 27769: Advanced editor shouldn't break copying selected text with Ctrl+C

Would have been nice if was that simple, but that doesn't work. Perhaps that function is returning to some other function that then returns to CodeMirror?
Comment 3 Esther Melander 2023-07-25 19:20:15 UTC
The expectation of Ctrl+C is to select any amount of text/lines of text to copy. Across software it is almost a universal expectation. Koha says that Ctrl+C is to copy a field only. This seems to be causing a conflict as noted above.

I think the much simpler solution is to change the keyboard shortcut to copy a field from Ctrl+C to Ctrl+Alt+C by default. I tested the new key combination and this returns Ctrl+C to the expected behavior while allowing the ability to copy a field with a different key combination.
Comment 4 Katrin Fischer 2023-07-26 18:26:16 UTC
I agree, we could update the default mappings that are installed with Koha. This would be a fix for any new installation, existing installations are harder because people might be used to the behavior or might have changed the mappings already.
Comment 5 Dani Elder 2024-01-31 18:51:43 UTC
Please change to expected behavior of CTRL+C.
Comment 6 Nick Clemens 2024-02-05 18:56:12 UTC
When originally implemented, the feature was unable to use the system clipboard, this was a browser limitation at the time. The internal clipboard was developed as a workaround, and used the familiar key combinations to provide the closest experience possible.

We could change the default keys for new installations, however, I agree with Katrin that making a change to existing setups might be more confusing.
Comment 7 Heather 2024-02-05 19:42:09 UTC
OK, y'all, Real Cataloger here, one who uses the Advanced Bib Editor all the time--like, all the time.

Please, please, please change Ctrl-C to conform to the standard behavior of all the other software applications.

I do appreciate people thinking it might be confusing to have the "Koha Ctrl C" implemented in all its weirdness and then change it, but here are my arguments:

--We catalogers are *not* using Koha in a vacuum; we are using Koha alongside many other applications within which Ctrl-C is behaving consistently, such as OCLC software, MarcEdit, spreadsheets, databases, text editors, web browsers, etc.--it's what we do.  Then we pop into Koha and it's, "Oh for fuschia's sake, that's right, Koha-Ctrl-C is different."  (Please note that I'm not mentally saying "fuschia" but am mentally thinking another word in English that is one syllable.):)

--We catalogers copy & paste OFTEN because it is a time honored technique to avoid typos and promote consistency.  Standardizing this behavior would speed up our workflow by promoting consistency in keystrokes.

--Which brings me to Consistency:  We catalogers love, adore, and are professionally committed to Consistency.  'Koha Ctrl C' is Inconsistent and slows up every cataloger who then has to pause and think, "Oh, right...it's Koha-Ctrl-C rather than Ctrl-C.

I would be absolutely delighted to be outvoted on this by other catalogers, but I strongly suspect that all catalogers would vote for consistency and request that Ctrl-C in the Advanced Bib Editor be changed to conformed with standard practice!

As is, Koha-Ctrl-C *is* behaving in ways that are invisible to the user (i.e., not showing anything in the Clipboard to the lower left)--it is copying anything highlighted or being it's own wacky non-standard self and copying the entire MARC tag if nothing is hightlighted).  If the Koha Clipboard even showed what Koha-Ctrl-C copied, I could see an argument for maintaining this inconsistent behavior, but seeing as the Koha Clipboard doesn't even show what was copied, I think this feature is broken and needs fixing, and as it needs fixing, I advocate fixing it to standard & consistent behavior.
Comment 8 Phil Ringnalda 2024-02-07 16:40:20 UTC
In case it wasn't clear, I didn't completely exhaust all possibilities for making what the code intends to do, just pass on handling the keystroke and let the browser handle it, actually work when something is selected. Since my dev install is on a Mac, I can only test by attaching a patch and driving 8 miles to test on a sandbox from a Windows machine, so I only tried the one most obvious patch.

It's also entirely possible that it's a Codemirror bug which was fixed between our 5.40.2 and the current 5.65.16 (or the current version 6).
Comment 9 Nick Clemens 2024-02-07 17:26:46 UTC
Created attachment 161834 [details] [review]
Bug 27769: Update default key mapping to copy text

When implemented, the keyboard shortcuts could not use the system clipboard so
Citrl-C was mapped to use the editor clipboard. As this now works with the system clipboard
we should allow standard functionality and remap the Koha shortcut.

This will not affect existing installations, however, they can modify the keyboard shortcut if they
wish by visting:
Administration->Keyboard shortcuts
 or
Clicking the 'Redefine shortcuts' link under 'Keyboard shortcuts' in the advanced editor.

To test:
1 - Apply this patch
2 - Reset all
3 - Confirm Ctrl-Alt-C is the command to 'Copy current field' in Keyboard shortcuts in the advanced editor
Comment 10 Nick Clemens 2024-02-07 17:26:48 UTC
Created attachment 161835 [details] [review]
Bug 27769: Remove 'brief' class from advanced editor clipboard

This field is intended to be a list of items copied from the editor, removing the brief
class allows it to expand to 10 rows as intended.

To test:
1 - Apply this patch
2 - Confirm clipboard section of advanced editor is several rows
3 - Use Ctrl-Alt-C to copy some fields
4 - Confirm you can see multiple fields in the clipboard
Comment 11 Nick Clemens 2024-02-07 17:28:02 UTC
I added a patch, I only update the default for new installs. Current users can easily remap this field currently, and it should work. We can highlight this in the release notes to let users know
Comment 12 David Nind 2024-02-07 21:56:46 UTC
Created attachment 161853 [details] [review]
Bug 27769: Update default key mapping to copy text

When implemented, the keyboard shortcuts could not use the system clipboard so
Citrl-C was mapped to use the editor clipboard. As this now works with the system clipboard
we should allow standard functionality and remap the Koha shortcut.

This will not affect existing installations, however, they can modify the keyboard shortcut if they
wish by visting:
Administration->Keyboard shortcuts
 or
Clicking the 'Redefine shortcuts' link under 'Keyboard shortcuts' in the advanced editor.

To test:
1 - Apply this patch
2 - Reset all
3 - Confirm Ctrl-Alt-C is the command to 'Copy current field' in Keyboard shortcuts in the advanced editor

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2024-02-07 21:56:49 UTC
Created attachment 161854 [details] [review]
Bug 27769: Remove 'brief' class from advanced editor clipboard

This field is intended to be a list of items copied from the editor, removing the brief
class allows it to expand to 10 rows as intended.

To test:
1 - Apply this patch
2 - Confirm clipboard section of advanced editor is several rows
3 - Use Ctrl-Alt-C to copy some fields
4 - Confirm you can see multiple fields in the clipboard

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2024-02-07 22:10:27 UTC
I have signed off, but I did wonder why Ctrl-C doesn't copy to the clipboard as well as Ctrl-Alt-C (but assuming there is some 'technical' reason).
Comment 15 Victor Grousset/tuxayo 2024-02-19 23:38:08 UTC
> 3 - Use Ctrl-Alt-C to copy some fields
> 4 - Confirm you can see multiple fields in the clipboard

There is an issue that doesn't happen without the patch:
https://pic.infini.fr/enm6JRvb/yxFMJ7Bt.png

On the same record (279 in sample data) copying the line for the field 490 without the patch doesn't change the width. With the patch, if a long field is copied, it will stay and the list and keep the list too wide.
Comment 16 Victor Grousset/tuxayo 2024-02-20 00:11:53 UTC
@Phil @Esther @Dani @Heather

For your current problem, you can immediately fix it by changing the shortcut in: Administration > Keyboard shortcuts

As there are installations that have likely customized their shortcuts, the proposed patch only changes the default for new Koha installations and it makes sense.

---

(In reply to David Nind from comment #14)
> I have signed off, but I did wonder why Ctrl-C doesn't copy to the clipboard
> as well as Ctrl-Alt-C (but assuming there is some 'technical' reason).

More of a functional reason. If a page wants to catch ctrl-c to do something else, then it's better for the browser to not process ctrl-c as well. For ctrl-c isn't not a big deal but for pasting the shortcut is ctrl-p which for browsers is for printing the page.

Oh wait, maybe you mean it doesn't copy to the editor clipboard. When I first tried it seems it wasn't doing it and even pasting didn't work. ctrl-p would open the print modal of the browser...
I went to compare with another Koha without patches and it worked there, it copied and pasted. (I'm not sure about the exact chronology) Then on my local Koha with the patches on Chromium and shortcuts worked. And then they worked on Firefox! 😵‍💫
If you had something weird know you are not alone ^^"
At least it's not related to the patches, I had the issue before applying them.
Comment 17 Phil Ringnalda 2024-02-20 00:32:56 UTC
I changed the shortcut almost three years ago.

But our code still intends to pass on handling the shortcut when text is selected, which would be an elegant solution, and still fails to pass the shortcut on to other handlers.
Comment 18 Victor Grousset/tuxayo 2024-02-20 23:02:29 UTC
Is it about the patch on comment 1?

Shame that it didn't work. Though it would be less simple to validate it functionally.
It looks confusing UX wise to have the two clipboards sharing the same copy shortcut. Even if there is indeed elegance in it.

Maybe it's not that confusing but if you manage to make such a solution work, it would be best in another ticket since here the direction changed to the most straight forward approach of having separate shortcuts for the two clipboards to simply fix the current situation of copying to the system clipboard not working.