menu key support is now broken in the latest master

BlairDuncan commented 10 years ago

Bisected to this commit: commit 2b0754c6a0f079d033f1da41a8ea4c15e1198b6e

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.

cappbot commented 10 years ago

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

BlairDuncan commented 10 years ago

I think that in CPApplication within sendEvent, line 608. Before returning should have a:

    [[theWindow platformWindow] _propagateCurrentDOMEvent:NO];

BlairDuncan commented 10 years ago

But in some cases such as CPTextField, we want cut copy paste to be handled by the browser so it should be:

    // Check if this is a candidate for key equivalent...
    if ([anEvent _couldBeKeyEquivalent] && [self _handleKeyEquivalent:anEvent])
    {
        switch([anEvent keyCode])
        {
            case 67:
            case 86:
            case 88:
                break;
            default:
                [[theWindow platformWindow] _propagateCurrentDOMEvent:NO];
        }
        return;
    }

aljungberg commented 10 years ago

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 2b0754c6a0f079d033f1da41a8ea4c15e1198b6e 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:

But in some cases such as CPTextField, we want cut copy paste to be handled by the browser so it should be:
// Check if this is a candidate for key equivalent...
if ([anEvent _couldBeKeyEquivalent] && [self _handleKeyEquivalent:anEvent])
{
    switch([anEvent keyCode])
    {
        case 67:
        case 86:
        case 88:
            break;
        default:
            [[theWindow platformWindow] _propagateCurrentDOMEvent:NO];
    }
    return;
}
— Reply to this email directly or view it on GitHub.

BlairDuncan commented 10 years ago

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.


@implementation CPTextField (suggestedFix)

- (void)copy:(id)sender
{
    // First write to the Cappuccino clipboard.
    var selectedRange = [self selectedRange];

   if (selectedRange.length < 1)
        return;

    var handledByBrowser =  [CPPlatform isBrowser] && ([[CPApp currentEvent] type] == CPKeyDown),
        pasteboard = [CPPasteboard generalPasteboard],
        stringForPasting = [_stringValue substringWithRange:selectedRange];

console.log("Cappuccino: " + _cmd);
    [pasteboard declareTypes:[CPStringPboardType] owner:nil];
    [pasteboard setString:stringForPasting forType:CPStringPboardType];

    if (handledByBrowser)
    {
console.log("System: " + _cmd);
        // Then also allow the browser to capture the copied text into the system clipboard.
            [[[self window] platformWindow] _propagateCurrentDOMEvent:YES];
    }
}

- (void)cut:(id)sender
{
    var handledByBrowser =  [CPPlatform isBrowser] && ([[CPApp currentEvent] type] == CPKeyDown);
console.log(_cmd);
    [self copy:sender];

    if (!handledByBrowser)
    {
console.log("Cappuccino: " + _cmd);
        [self delete:sender];
    }
    else
    {
console.log("System: " + _cmd);
        // Allow the browser's standard cut handling.
        [[[self window] platformWindow] _propagateCurrentDOMEvent:YES];

        // If we don't have an oninput listener, we won't detect the change made by the cut and need to fake a key up "soon".
        if (!CPFeatureIsCompatible(CPInputOnInputEventFeature))
            [CPTimer scheduledTimerWithTimeInterval:0.0 target:self selector:@selector(keyUp:) userInfo:nil repeats:NO];
    }
}

- (void)paste:(id)sender
{
    var handledByBrowser =  [CPPlatform isBrowser] && ([[CPApp currentEvent] type] == CPKeyDown);
    if (!handledByBrowser)
    {
console.log("Cappuccino: " + _cmd);
        var pasteboard = [CPPasteboard generalPasteboard];

        if (![[pasteboard types] containsObject:CPStringPboardType])
            return;

        [self delete:sender];

        var selectedRange = [self selectedRange],
            stringForPasting = [pasteboard stringForType:CPStringPboardType],
            newValue = [_stringValue stringByReplacingCharactersInRange:selectedRange withString:stringForPasting];

        [self setStringValue:newValue];
        [self setSelectedRange:CPMakeRange(selectedRange.location + stringForPasting.length, 0)];
    }
    else
    {
console.log("System: " + _cmd);
        // Allow the browser's standard paste handling.
        [[[self window] platformWindow] _propagateCurrentDOMEvent:YES];

        // If we don't have an oninput listener, we won't detect the change made by the cut and need to fake a key up "soon".
        if (!CPFeatureIsCompatible(CPInputOnInputEventFeature))
            [CPTimer scheduledTimerWithTimeInterval:0.0 target:self selector:@selector(keyUp:) userInfo:nil repeats:NO];
    }
}

- (void)delete:(id)sender
{
    var selectedRange = [self selectedRange];

    if (selectedRange.length < 1)
         return;

    var newValue = [_stringValue stringByReplacingCharactersInRange:selectedRange withString:""];

    [self setStringValue:newValue];
    [self setSelectedRange:CPMakeRange(selectedRange.location, 0)];
}

@end

aljungberg commented 10 years ago

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.

ahankinson commented 10 years ago

-#new +bug +AppKit +#needs-patch

cappbot commented 10 years ago

Milestone: Someday. Labels: #needs-patch, AppKit, bug. What's next? This issue needs a volunteer to write and submit code to address it.

aljungberg commented 10 years ago

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.

BlairDuncan commented 10 years ago

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. Remember the application still has the option to forward it on to the browser if that is needed.

When a user enters "Cmd ," they are expecting to see the application's preferences NOT Safari's preferences. "Cmd n" should open a new document, NOT a new browser window. As was always the case in the past, if the developer wants "Cmd n" to not open a new document, there should be no menu shortcut assigned "Cmd n"

aljungberg commented 10 years ago

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).

saikat commented 10 years ago

@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?):

- (BOOL)performKeyEquivalent:(CPEvent)anEvent
{
    var characters = [anEvent characters],
        modifierFlags = [anEvent modifierFlags];

    if ([[self window] firstResponder] === self && (characters == "c" || characters == "x" || characters == "v" || characters == "a") && (modifierFlags & CPPlatformActionKeyMask))
    {
        [[[self window] platformWindow] _propagateCurrentDOMEvent:YES];
        return YES;
    }
    return NO;
}

Thanks Alexander!

aljungberg commented 10 years ago

@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 CPTextField always want propagation (otherwise you couldn't type anything), so that's why we enable it in keyDown.

But then come the various special cases. If you check out CPPlatformPasteboard you'll see I ended up mapping out three flows for copy and paste depending on the browser's capabilities:

clipboard API
pasteboard input element hack
native browser CNP

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 CopyAndPaste test, running in Chrome, I can select some text in Regular Text Field and cut it with Cmd-X. I can paste it back in the same field, or outside of the browser. And this app does have an Edit menu. Is your experience different there?

(By the way, in Cocoa if you remove the Edit menu, copy and paste stops working altogether AFAIK.)

daboe01 commented 6 years ago

@aljungberg, in my opinion @BlairDuncan is right. the average cappuccino-developer expects, that his menu shortcuts just work. there should be no need to override the event mechanics in every control. i suggest to give precedence to cappuccino shortcuts over browser-shortcuts (with the obvious exception of copy/paste).

aljungberg commented 6 years ago

@daboe01 Let's say we switch to never propagate when there is any event handler.

Now a user can hit Cmd-N and make a new Cappuccino window, if that's a Cappuccino shortcut.
...and if there is no such keyboard shortcut Cmd-N makes a new browser window as expected.
But they can still hit Cmd-D and make a bookmark if that's not a Cappuccino shortcut.

Okay so far so good. But now let's say a text field is the key responder.

Now a user hitting Cmd-N would open a new browser window AND a new Cappuccino window because the text field enables propagation unconditionally. It needs to do this to make sure text can be typed and copy and paste works even when there's a Cappuccino edit menu.

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 null.

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 null. If after the event loop the state is either "yes" or null, we propagate.

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.

daboe01 commented 6 years ago

@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?

aljungberg commented 6 years ago

@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.

daboe01 commented 6 years ago

@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?

aljungberg commented 6 years ago

@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.

daboe01 commented 6 years ago

@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 case 2: cmd-anything not handled by a CPMenu shortcut->propagate to browser case 3: cmd-anything handled by CPKeyBinding.j->no propagation case 4: cmd-anything not handled by CPKeyBinding.j ->propagate to browser case 5: cmd-c/x/v in -> handled via e.clipboardData.setData, no propagation necessary.

did i miss anything?

aljungberg commented 6 years ago

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: ife.clipboardData.setData actually worked universally across browsers our whole copy and paste system would have been a million times easier. But it doesn't, hence our massive amount of complex workarounds.)

daboe01 commented 6 years ago

@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?

daboe01 commented 6 years ago

@aljungberg we can transparently handle 99% of all use cases when we switch to never propagate when there is any event handler. 1% of the controls with very special clipboard needs will bypass the cappuccino event-system anyway (as CPTextView does). this would fix 3+ github issues and give a better UI experience for the keyboard-affine end-users

aljungberg commented 6 years ago

@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.

cappuccino / cappuccino

menu key support is now broken in the latest master #1964