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
menu key support is now broken in the latest master #1964
Comments
Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue. |
I think that in CPApplication within sendEvent, line 608. Before returning should have a:
|
But in some cases such as CPTextField, we want cut copy paste to be handled by the browser so it should be:
|
The goal is that CPApp should not be hard-coding any DOM specific behaviour, but that any such behaviour should be in the controls or in CPPlatformWindow+DOM. Commit 2b0754c has some more notes on this thinking. Normally we pass all Cmd- key equivalents to the browser as to not prevent standard browser shortcuts. When we handle such a key equivalent internally though we want to stop propagation. So the question is where this special case behaviour is best implemented with the least amount of surprise and the least amount of littering DOM code specific code all over. On 20 Jul 2013, at 00:48, Blair Duncan notifications@github.com wrote:
|
Yes, wherever is appropriate as long as it gets fixed so that users can use command keys to navigate the UI. Copy and Paste of text to work properly requires either access the System clipboard OR the ability to send a keyboard event directly to the browser. Without either, it would seem that we will always be dealing with 2 clipboards, the Systems and Cappuccinos. Currently CPTextField does not handle copy and paste at all unless it is initiated by the command keys. You can see this using the CopyAndPaste Manual Test. If you enter text into the text field, you can cut, copy and paste using the keyboard or browsers edit menu but not from the Cappuccino edit menu. I have a lot of users that do not use keyboard commands (mostly windows users). They tend to use the menu and or toolbar buttons for cut, copy, paste. Below is a suggestion for a CPTextField fix that solves this by only allowing the browser to perform the action when it is created by a keyboard command or the browsers own edit menu. It adds a proper delete that is menu accessible and is required for cut to work properly. (Previously cut relied on deleteBackward which always left an extra character at the start when handled by cappuccino). You'll also notice in the log, this exposes a bug in Safari and Chrome where cut, copy and paste are being sent twice. It does not happen Firefox.
|
To copy and paste to the native clipboard in the browser, the copy and paste must happen within a user initiated CNP event, and it's the browser which decides what that means. The Cappuccino copy and paste menu is not something the browser recognises as a user initiated event. It's a browser restriction. We'll never see exactly the right behaviour from the Cappuccino edit menu in the browser. On other platforms (NativeHost) this could be different. If your users keep being drawn to the Cappuccino edit menu you might have to use a Flash based solution. If you're interested in changing something regarding copy and paste, those changes should go into CPPlatformPasteboard.j. Remember that CNP is not something specific to text fields. You can copy and paste from a table view, a collection view, a token field, a multiline text field and so on. Also the code paths for the old and the new style CNP are quite different (hidden input box versus clipboard API). The current keyboard or browser menu behaviour in CPPlatformPasteboard.j is tested carefully with the Tests/Manual/CopyAndPaste/ in the 4 major browsers, both for the collection view and the text fields. So any changes we make from here on out will need the same care in testing. For instance, in Chrome it should be possibly to copy "Rabbit" from the collection view to the text view, paste into the input field, delete a few characters, select a few, cut them, get the right result, and then paste that result back into the collection view even if that view is in a different browser, all using the keyboard or browser menu. I'll see if we can improve the behaviour of the Edit menu to make sure cut works right with the Capp pasteboard at least. And I'll consider the original issue on Cmd-O. |
-#new |
Milestone: Someday. Labels: #needs-patch, AppKit, bug. What's next? This issue needs a volunteer to write and submit code to address it. |
This made it impossible to select some (non-editable) text, and then to click a Cappuccino menu option like Edit > Copy. Upon the click of Edit, the just selected text would be lost. This was caused by some very mysterious code focusing and blurring an input every time propagation was stopped. Since it wasn't documented, the snippet was just removed for now. If it turns out it was useful we'll need to add it back and make sure it's not called in this scenario (while documenting it properly). Refs #1964.
When the Edit menu is used to cut or to paste, Cappuccino needs to do all the work of making it happen. But the current code relied on the browser doing part of the work. This fix adds new state so CPTextField can know if it should expect the browser to do some of the work or not. Refs #1964.
This menu item deletes the current selection if there is one. Refs #1964.
Copy, cut and delete are now disabled if there's no selection in the active text field. Cut, paste and delete are greyed out if the text field is not editable (a label). Refs #1964.
I have extended the support for the Edit menu. Unfortunately I can't think of a clean solution to the Cmd-O problem. In particular, CPApp doesn't know whether to stop an event or not - the control receiving the event might have an opinion. We could add this code in CPMenu (and CPButton) before the key equivalent is handled, e.g. setting propagation to NO since there's a handler, but letting the handler possibly override this. But that wouldn't work if a CPTextField was the first responder since it unconditionally propagates. So then CPTextField needs to be modified to only conditionally propagate. But on what condition? Only propagate if the meta key is not depressed? That's not safe. The OS could recognise an edit by a meta key press - in fact we know it does for e.g. Cmd-X. So we can't just say "because cmd is depressed and it was a menu equivalent we should not propagate". It's not very clean. Patches and thoughts welcome. |
It is not a unique Cmd-O problem, it is a Cmd-Anything problem (as stated in my original comment). With the exception of Cut/Copy/Paste which is a special case… if the key combination is specified as a menu keyboard shortcut it should be handled by the application only. When a user enters "Cmd ," they are expecting to see the application's preferences NOT Safari's preferences. |
I understand what the issue is. What I'm explaining is that there is no obvious solution to it. You write that Cut/Copy/Paste are special cases, but that's only one of many other possible special cases. This is why we need to leave it up to the controls themselves to decide when an event should propagate, and when it should not. At this stage I think your best option is to alter your Cmd-N, Cmd-O, Cmd-Whatever handler to stop propagation. It's not clear how to do this from a top down point of view since CPApp has no idea what the controls are up to (and shouldn't need to know). |
@aljungberg I read through the above and I think I follow, but sorry if I missed something/this was already suggested. For the specific case of CPTextField actions like cut/copy/paste being hidden by CPMainMenu shortcuts, what do you think of a solution where, whenever a CPTextField becomes first responder, any main menu shortcuts that would hide CPTextField actions become disabled? So, basically, the CPTextField would return some set of characters that it intends to always handle (like cut/copy/paste), and CPWindow, when it sets a CPTextField as first responder, can compare that against all the current keyEquivalents in the main menu and disable the items in the main menu that overlap. I think you are right in general that controls themselves should decide when to propagate an event, but I wonder if, just for CPTextField, this should be built-in to Cappuccino so the expected behavior for a text field works when a menu is enabled in the application. Also, since CPTextField is a Cappiuccino built-in class, I think Cappuccino should handle setting up the proper propagation rules for it. The solution above would have the nice side effect of keeping users from using the Cappuccino edit menu to try to do text cut/copy/paste since those would get disabled when you are focused on a CPTextField. It seems like this is an issue for @BlairDuncan's users. A simpler solution, of course, would be to just add something like this to CPTextField.j. There may be more than c/x/v/a that we'd want to add here though. I know you are arguing against something this simplistic above, but I'm not sure I entirely understand your argument -- why would the code below be bad (other than us hardcoding in the keys CPTextField needs to handle, or is that the main argument?):
Thanks Alexander! |
@saikat it has been a while since I worked on this but one thing with the copy and paste handling is that the DOM event propagation is very finicky. In general But then come the various special cases. If you check out
When I was working on it the goal was to support copy and paste as generally as possible, like between collection views and text fields. So basically you want to disable copy and paste menu options when a text field becomes the first responder, since they can't access the browser pasteboard anyhow? I saw in your other ticket that you wrote that text is not cuttable in a text field if you have edit menu options, but this doesn't seem to be the case for me. In the (By the way, in Cocoa if you remove the Edit menu, copy and paste stops working altogether AFAIK.) |
@aljungberg, in my opinion @BlairDuncan is right. |
@daboe01 Let's say we switch to never propagate when there is any event handler.
Okay so far so good. But now let's say a text field is the key responder.
So the bottom line is for this "default to not propagate when there's a handler" model can only work with a bunch of extra logic to selectively propagate. It can't say "never propagate Cmd-X". It needs to say "propagate cmd-X unless we handled it and copy and paste didn't say we have to propagate it anyhow". If I were to implement this I'd probably make propagation control a 3 state system instead. Not just yes or no but also When it's a key handler's time to act, it can choose "yes, I definitely need this to propagate", "no, I handled it, please don't propagate" or say nothing. Now the initial state is A CPMenu handler would set the "no" choice, because it would believe its menu item fully handles the event. But in the case of copy, the copy and paste handler would override the "no" with a "yes" (the CNP handler would need to be later in the event processing queue). These events must propagate even that there is a menu item for copy. A CPTextField would generally say nothing. I don't know, I'm not as familiar with this system as I used to be. Maybe I'm overlooking something. But note that even in this system the controls/event handlers themselves are the decision makes. There is no one size fits all decision we can make at the top level. Only the menu control knows if it actually handled a control. Only the text field (and the copy and paste adjunct code) knows when it has to put its foot down and demand propagation. |
@aljungberg does this mean, that we can fix all this by dumping the native input field and use a CPTextView as our field editor instead? |
@daboe01 I can't answer that question. That depends entirely on how well CPTextView can emulate a native text field. Native text fields are very feature rich. |
@aljungberg i know for sure, that CPTextView does not unconditionally forward events, but only does so for cut/copy/paste. all other text handling shortcuts are handled by CPKeyBinding.j from my understanding of your comments, this would fix the issue, wouldn't it? |
@daboe01 even if it were a perfect drop in replacement, perfectly able to handle all keyboard events and mouse events the native one does (which again I can't answer), you'd still need to implement the 3 state solution above or something comparable to handle native propagation. Some events should propagate, some should not. |
@aljungberg i do not understand the need for this "3-state thing". all the logic can be transparently handled by cappuccino. this is how it would work given we use a CPTextView based new CPTextField: case 1: cmd-anything handled by a CPMenu shortcut->no propagation did i miss anything? |
All the logic cannot be transparently handled by Cappuccino, that's the whole issue. Sometimes we have to propagate, sometimes we don't, for the sake of copy and paste -- and even if we replace CPTextView with an input-free version there'd still be user controls to worry about. Just because there's a CPMenu with Cmd-C doesn't mean it doesn't need to propagate. (Footnote: if |
@aljungberg i do not see any issues with our current e.clipboardData.setData based copy and paste in CPTextView across the current major browsers, neither on windows, nor on mac. Do you? |
@aljungberg we can transparently handle 99% of all use cases when we switch to never propagate when there is any event handler. |
@daboe01 when I looked at the clipboard API last a couple of years ago it was a no-go. If you feel that situation has changed, feel free to investigate. You can use the manual test setup and steps I described above to verify it (less text field focused activities like copying and pasting into a collection view or between browsers are important to keep in mind). I stand by my opinion that whether to propagate needs to be decidable by the participants of the event chain, but I'm not actively working on this. If you have a better way, go for it. |
Bisected to this commit:
commit 2b0754c
Can be verified in the manual CPDocumentController test.
Command + O opens a new document and sends a message to the browser to open a new page.
The text was updated successfully, but these errors were encountered: