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.

Sign up for GitHub

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

Fix Space Bar in <summary> element #24558

Closed
wants to merge 2 commits into from
Closed

Fix Space Bar in <summary> element #24558

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
916bb44
Fix Space Bar in <summary> element
Aug 12, 2020
fc46fde
Clarify comment
georgeh Aug 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/rich-text/src/component/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,12 @@ function RichText( {
function handleSpace( event ) {
const { keyCode, shiftKey, altKey, metaKey, ctrlKey } = event;

// Allow space characters to be typed in a <summary>.
if ( keyCode === SPACE && TagName === 'summary' ) {
handleChange( insert( createRecord(), ' ' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think RichText should be responsible for inserting characters, because this varies by keyboards and language and user settings. If the browser is blocking spaces to be inserted because of some other behaviour, the behaviour should be prevented some other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this! Are you saying that the fix for this issue belongs somewhere else, or that we should be smarter about the locale of the character we are inserting on line 532? I want to do it the right way, and would appreciate your help understanding what that is.

I spent a lot of time on #23940 looking for a way to fix this at the block level but couldn't get it to work without any luck. Something about the way that Firefox handles the spacebar in a <summary contenteditable> element eats the space character, so the two solutions I came up with were:

  1. Add this space inserter
  2. Make the RichText a div child of the <summary> element, which means either rendering that on the client side or having separate CSS for the editor and client

I appreciate any pointers on how to make this better, and thanks again for the feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know honestly. First, since this is a problem only with the summary element, we should fix it in the block and not in rich text imo. Maybe you can prevent default the event? Not sure. If the only way is to insert the character ourselves, maybe have a look at the data property in the event and insert that data?

return;
}

if (
// Only override when no modifiers are pressed.
shiftKey ||
Expand Down