Add hotkey to code block button #37

aaron-tdouble · 2021-03-18T17:19:37Z

Description

This PR does the following:

adds a hotkey of C for the MarkdownCodeButtonElement
forces hotkeys to be case sensitive

Issue

References: https://github.com/github/special-projects/issues/135

Screenshots

Demonstrating the current behavior of hotkeys not being case sensitive:

Risk Assessment

Low risk

relph · 2021-03-18T18:43:56Z

adds a hotkey of C for the MarkdownCodeButtonElement

@aaron-tdouble in chrome the shortcut cmd+shift+C opens dev tools. Will this conflict with that? Or when you're writing MD and focused in the text field will cmd+shift+C insert the code block and when not focused it'll continue to open dev tools?

Also, can we add the new shortcut to the tooltip that appears when hovering on the code icon in the markdown toolbar? Similar to bold:

aaron-tdouble · 2021-03-18T20:39:58Z

in chrome the shortcut cmd+shift+C opens dev tools. Will this conflict with that? Or when you're writing MD and focused in the text field will cmd+shift+C insert the code block and when not focused it'll continue to open dev tools?

@relph Correct! Because it is intercepting the event in the text area, it does not open dev tools.

Also, can we add the new shortcut to the tooltip that appears when hovering on the code icon in the markdown toolbar?

Absolutely! That change will happen inside the monorepo in a subsequent PR.

koddsson · 2021-03-18T22:43:10Z

src/index.ts

+    const key = event.shiftKey ? event.key.toUpperCase() : event.key
+    const button = findHotkey(toolbar, key)


I don't think this change is needed. I can revert this block locally and the tests still run green.

The case-sensitivity is already backed in since we do strict equality of the key pressed and the hotkey attribute.

@koddsson ~~I must be doing something wrong because this test (correctly) fails for me on main.~~

What does it do for you on the example page?

If I cmd-b or cmd-B (cmd-shift-b), it bolds the text. I would expect it only to respond to a lowercase b.

This is true for me on Chrome, Safari, and Firefox. Also true for someone that used Microsoft Edge browser on Mac.

@koddsson Wait. Scratch my first comment above. I was testing the wrong thing. Let me look into that in the morning, but my second comment ☝️ still stands. I'm curious how this behaves for you.

I agree with you that it should work as written. All the docs I read say that key will always return the actual key being pressed (as in, the uppercase B or lowercase b), but when I inspect the event in the debugger, I never see an uppercase letter for key.

What does it do for you on the example page?

If I cmd-b or cmd-B (cmd-shift-b), it bolds the text. I would expect it only to respond to a lowercase b.

⌘+B is a global shortcut on my mac that launches a application 😄

But yeah if I turn that off then I can bold with both. This is unexpected but apparently, if you are holding the meta key down KeyboardEvent.key will always be lowercase.

I made a little site to test out the keydown handler here.

Getting the tests to fail when this block is removed would be good though 👍🏻

Oh yeah... this is a specific edge case with macos reporting lower case keys for specifically cmd+shift. See https://bugs.webkit.org/show_bug.cgi?id=174782 & https://bugs.chromium.org/p/chromium/issues/detail?id=747358

Good finds and information, @koddsson and @keithamus. I'll work on getting a breaking test on this.

add breaking test for lower/uppercase hotkey

koddsson · 2021-03-18T22:56:03Z

in chrome the shortcut cmd+shift+C opens dev tools. Will this conflict with that? Or when you're writing MD and focused in the text field will cmd+shift+C insert the code block and when not focused it'll continue to open dev tools?

@relph Correct! Because it is intercepting the event in the text area, it does not open dev tools.

I never use this shortcut but this feels like this is unexpected behavior. I wonder if users will get upset if we override their browser shortcuts.

On macOS, there is an existing bug that will always report the keydown event.key as lowercase, even when pressing cmd-shift-letter. This can result in the toolbar hotkey firing for both the uppercase and lowercase meta keys. This was fixed in a prior commit, but a test is being added here to emulate this behavior in the test, so that there is a “breaking” regression test.

aaron-tdouble · 2021-03-19T13:30:07Z

I never use this shortcut but this feels like this is unexpected behavior. I wonder if users will get upset if we override their browser shortcuts.

I think this is a good question to raise. @relph or @siminapasat do you want to weigh in here?

On one hand, I have all kinds of tools installed that override some default shortcuts. (In fact, I had to install ShortcutDetective just to figure out why cmd-C wasn't initially working for me in this PR. It was due to ColorSlurp.)

The difference is that I chose to install those apps, even if I didn't know it was going to hijack those shortcuts. In this case, we are in some ways forcing the shortcut on the user.

On the other hand, this (1) is very isolated to having focus in the textarea element and (2) seems to be a common keystroke for insertion of code blocks. For some users, this will be a net good thing™.

Thoughts, opinions?

koddsson · 2021-03-19T13:45:03Z

On the other hand, this (1) is very isolated to having focus in the textarea element and (2) seems to be a common keystroke for insertion of code blocks. For some users, this will be a net good thing™.

I buy that argument. Finding a shortcut that isn't used by any other application is tricky.

If we could somehow avoid this one since it's used a lot by developers and they make up the majority of our users I think that could be great but I'll default to you here.

relph · 2021-03-19T15:52:01Z

On the other hand, this (1) is very isolated to having focus in the textarea element and (2) seems to be a common keystroke for insertion of code blocks. For some users, this will be a net good thing™.

I was going to argue for this ☝️ but Chrome being by far the most popular browser used by developers I have a feeling we might be adding friction to a lot of peoples workflow. Feels easily avoidable. What if instead we follow notions lead and use ⌘+E

Thoughts?

aaron-tdouble · 2021-03-19T15:56:53Z

@relph I'm good with ⌘+E and since we will have toolbar hover help, there will be some user nudging.

aaron-tdouble · 2021-03-19T21:30:41Z

@koddsson @keithamus I changed the keyboard shortcut to ⌘+E for code blocks. Let me know if you have any other concerns or need anything else in order to merge this. Thanks!

koddsson

This looks good to me! ✨

keithamus

Agreed! 👍

aaron-tdouble added 2 commits March 18, 2021 12:58

Ensure hotkeys are case sensitive

1f58670

Add hotkey for code block

b540e7c

koddsson reviewed Mar 18, 2021

View reviewed changes

Add test for uppercased case sensitivity

891f80d

Change code block hotkey from C -> E

53b93f2

koddsson approved these changes Mar 22, 2021

View reviewed changes

keithamus approved these changes Mar 22, 2021

View reviewed changes

keithamus merged commit df9c592 into main Mar 22, 2021

keithamus deleted the td/add-hotkey-to-code-button branch March 22, 2021 10:27

koddsson mentioned this pull request Mar 23, 2021

Change code block hotkey from E -> e #38

Merged

abraithwaite mentioned this pull request Jul 6, 2021

Configurable key for code block insertion #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hotkey to code block button #37

Add hotkey to code block button #37

aaron-tdouble commented Mar 18, 2021

relph commented Mar 18, 2021

aaron-tdouble commented Mar 18, 2021 •

edited

Loading

koddsson Mar 18, 2021

aaron-tdouble Mar 19, 2021 •

edited

Loading

aaron-tdouble Mar 19, 2021 •

edited

Loading

aaron-tdouble Mar 19, 2021 •

edited

Loading

koddsson Mar 19, 2021

keithamus Mar 19, 2021

aaron-tdouble Mar 19, 2021

aaron-tdouble Mar 19, 2021

koddsson commented Mar 18, 2021

aaron-tdouble commented Mar 19, 2021 •

edited

Loading

koddsson commented Mar 19, 2021

relph commented Mar 19, 2021 •

edited

Loading

aaron-tdouble commented Mar 19, 2021

aaron-tdouble commented Mar 19, 2021

koddsson left a comment

keithamus left a comment

		const key = event.shiftKey ? event.key.toUpperCase() : event.key
		const button = findHotkey(toolbar, key)

Add hotkey to code block button #37

Add hotkey to code block button #37

Conversation

aaron-tdouble commented Mar 18, 2021

Description

Issue

Screenshots

Risk Assessment

relph commented Mar 18, 2021

aaron-tdouble commented Mar 18, 2021 • edited Loading

koddsson Mar 18, 2021

Choose a reason for hiding this comment

aaron-tdouble Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-tdouble Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-tdouble Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

koddsson Mar 19, 2021

Choose a reason for hiding this comment

keithamus Mar 19, 2021

Choose a reason for hiding this comment

aaron-tdouble Mar 19, 2021

Choose a reason for hiding this comment

aaron-tdouble Mar 19, 2021

Choose a reason for hiding this comment

koddsson commented Mar 18, 2021

aaron-tdouble commented Mar 19, 2021 • edited Loading

koddsson commented Mar 19, 2021

relph commented Mar 19, 2021 • edited Loading

aaron-tdouble commented Mar 19, 2021

aaron-tdouble commented Mar 19, 2021

koddsson left a comment

Choose a reason for hiding this comment

keithamus left a comment

Choose a reason for hiding this comment

aaron-tdouble commented Mar 18, 2021 •

edited

Loading

aaron-tdouble Mar 19, 2021 •

edited

Loading

aaron-tdouble Mar 19, 2021 •

edited

Loading

aaron-tdouble Mar 19, 2021 •

edited

Loading

aaron-tdouble commented Mar 19, 2021 •

edited

Loading

relph commented Mar 19, 2021 •

edited

Loading