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

XWIKI-22496: Link dialog option button is not keyboard operable #3714

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Dec 5, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22496

Changes

Description

  • Updated the HTML of the button
  • Added this button in the focusList
  • Removed weird listeners that would come with this focusList

Screenshots & Video

2024-12-05.12-44-14.mp4

Note that there's still issues with the pagepicker mode dropdown, but this PR's goal is not to fix this.

Executed Tests

Successfully built mvn clean install -f xwiki-platform-core/xwiki-platform-ckeditor/xwiki-platform-ckeditor-plugins ;mvn clean install -f xwiki-platform-core/xwiki-platform-ckeditor/xwiki-platform-ckeditor-webjar/ and tested manually these changes on a live instance (see video above)

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.10.X

* Updated the HTML of the button
* Added this button in the focusList
* Removed weird listeners that would come with this focusList
// The four elements before this button are: display link, page selection *3
this.getDialog().addFocusable(this.getElement() , 4);
// Without this, the keyboard press on this focusable element will trigger the click twice...
this.getElement().removeAllListeners();
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be safer to remove only the keydown listener that seems to be responsible for the double click? See https://github.com/ckeditor/ckeditor4/blob/82796d03d0ebbe34d48f3b65c6dde27107812a51/plugins/dialog/plugin.js#L709-L713 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I tried to add an additional listener to catch and stop the specific keydown events that are responsible with the problem (the bit that you linked), however, this relies on the CKEditor event system and classes, so it only created more problems and it wasn't easy to stop propagation.

I thought it wasn't straightforward to get rid of only one event listener without its ID in native javascript.
I'll look further into this solution, thanks for your feedback!

Copy link
Member

Choose a reason for hiding this comment

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

hmm, indeed there is no simple way to remove only the keydown listener (without having a reference to the listener) https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_dom_element.html#method-removeListener . So I'm fine to keep your current solution to remove all listeners.

@Sereza7 Sereza7 marked this pull request as draft December 10, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants