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

Pressing the space key doesn't execute the insertTable command #12344

Closed
Dumluregn opened this issue Aug 25, 2022 · 2 comments · Fixed by #12439
Closed

Pressing the space key doesn't execute the insertTable command #12344

Dumluregn opened this issue Aug 25, 2022 · 2 comments · Fixed by #12439
Assignees
Labels
domain:accessibility This issue reports an accessibility problem. intro Good first ticket. package:table squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Dumluregn
Copy link
Contributor

📝 Provide detailed reproduction steps (if any)

  1. Go to a demo with table, e.g. https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/examples/builds-custom/full-featured-editor.html.
  2. Open an insert table toolbar.
  3. Press space.

✔️ Expected result

Table should be inserted.

❌ Actual result

Command is not executed.

❓ Possible solution

Space should probably just get the same listener as enter key.

📃 Other details

  • Browser: all
  • OS: all
  • First affected CKEditor version: the one introducing grid keyboard handling (probably 35.1.0)
  • Installed CKEditor plugins: table

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Dumluregn Dumluregn added type:bug This issue reports a buggy (incorrect) behavior. package:table domain:accessibility This issue reports an accessibility problem. labels Aug 25, 2022
@mlewand mlewand added the intro Good first ticket. label Aug 25, 2022
@oleq
Copy link
Member

oleq commented Aug 25, 2022

To address this, we could make all grid items instances of ButtonView. This could be quite heavy but OTOH the entire grid is lazy loaded. Then we wouldn't need this enter listener

keydown: bind.to( evt => {
if ( evt.key === 'Enter' ) {
this.fire( 'execute' );
evt.preventDefault();
}
} )

just a delegated execute event from each ButtonView would do.


Alternatively, we could just add a space key listener. This would be pretty cheap and efficient, I guess 😛

@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Aug 29, 2022
@mlewand
Copy link
Contributor

mlewand commented Aug 30, 2022

To sum it up let's try first with ButtonView approach. Just test it out with some not-so-great workstation (mb air, slightly older pc, whatever).

If it performs well then let's go with ButtonView implementation.

If it shows slowdown (similar like special characters) let's fallback to alternative implementation, just a simple listener.

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Aug 30, 2022
@pszczesniak pszczesniak self-assigned this Sep 5, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Sep 6, 2022
@oleq oleq closed this as completed in 9ff9d44 Sep 14, 2022
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 14, 2022
@CKEditorBot CKEditorBot added this to the iteration 57 milestone Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:accessibility This issue reports an accessibility problem. intro Good first ticket. package:table squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants