Skip to content
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

Global hotkeys don't fire #801

Closed
waltonseymour opened this issue Mar 7, 2017 · 7 comments
Closed

Global hotkeys don't fire #801

waltonseymour opened this issue Mar 7, 2017 · 7 comments

Comments

@waltonseymour
Copy link

waltonseymour commented Mar 7, 2017

Bug report

  • Package version(s): 1.7.0
  • Browser and OS versions: Chrome 56.0.2924.87, MacOS 10.11.6

Steps to reproduce

  1. On a component decorated with the HotkeysTarget decorator, define a global hotkey.
  2. Now add public componentDidMount = () => { console.log("hello"); } to that component.
  3. Load the page in the browser.
  4. Press the global hotkey combination we just defined.

Actual behavior

The global hotkey does not trigger as expected. It is also not included in the hotkeys dialog (shown when you press shift + S. This occurs only when we use fat-arrow syntax to define componentDidMount; if we express as public componentDidMount() {, everything works as expected.

Expected behavior

Global hotkeys defined by a HotkeysTarget should work whether or not (and also in whichever manner) a componentDidMount is defined.


Original issue text

For some reason this setup does not capture command+s at all

public renderHotkeys() {
        return <Hotkeys>
            <Hotkey
                global
                combo="mod+s"
                label="Save notebook"
                onKeyDown={(e) => {
                    e.preventDefault();
                    this.onSaveNotebook();
                }}
            />
        </Hotkeys>;
    }

But this one does when focused

public renderHotkeys() {
        return <Hotkeys>
            <Hotkey
                combo="mod+s"
                label="Save notebook"
                onKeyDown={(e) => {
                    e.preventDefault();
                    this.onSaveNotebook();
                }}
            />
        </Hotkeys>;
    }

The page also has Monaco embedded in it which has its own hotkeys, so it might somehow be interfering.

@giladgray
Copy link
Contributor

I'll bet it is Monaco capturing the event. Can you try changing the hotkey to "mod+shift+s" and see if that works?

@waltonseymour
Copy link
Author

Tried changing to that, still no luck.

I also tried removing monaco and I still had the same issue. 😞

@cmslewis
Copy link
Contributor

cmslewis commented Mar 7, 2017

Hi @waltonseymour, I'm going to take on this issue this week. Hopefully we can get things resolved by our release on Friday.

I'll need some help reproducing your issue. When you have a moment, could you construct for me a minimum viable scenario that manifests the bug you're seeing? We just chatted here in the office, and we suspect there's an issue between our Hotkeys component and something else in your setup. The behavior (unfortunately?) works as expected on our end though.

In the meantime, I'll try to poke at our Hotkeys component to see if I can break it in a manner consistent with the one you're experiencing.

@cmslewis cmslewis added this to the 1.12.0 milestone Mar 7, 2017
@waltonseymour
Copy link
Author

Found a repro for this.

Adding a event listener on componentDidMount breaks global hotkeys for me.

ie.

public componentDidMount = () => {
    console.log("hello");
}

@cmslewis
Copy link
Contributor

cmslewis commented Mar 7, 2017

Thanks for that @waltonseymour. Will take a look.

@cmslewis
Copy link
Contributor

cmslewis commented Mar 9, 2017

@waltonseymour Thanks for chatting with me offline. Looks like we can repro now. I've updated the issue description with the steps to reproduce.

@cmslewis
Copy link
Contributor

Spoke with @adidahiya. Seems like the solution here is to extend tslint with a rule that would prevent declaring React lifecycle methods with a fat arrow. Will track this effort in palantir/tslint-react#74 and close this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants