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

[5.1] Replace deprecated KeyboardEvent code #42715

Merged
merged 9 commits into from Feb 2, 2024

Conversation

C-Lodder
Copy link
Member

Summary of Changes

Both the keypress event and keyCode property are deprecated in Javascript. This PR replaces them accordingly.

Testing Instructions

Calendar:

Open a calendar and ensure:

  1. Shift + Space appends the date to the input.
  2. ESC closes the calendar.
  3. Tab closes the calendar.
  4. The arrow keys (, , , ) navigate through the calendar accordingly.
  5. Repeat step 4 in RTL mode.

Web Install:

  1. Go to administrator/index.php?option=com_installer&view=install, then navigate to the "Install from Web" tab.
  2. Enter a phrase in the "Search" field.
  3. Press Enter.
  4. The search should be initiated.

@dgrammatiko I removed the following conditional statement:

if (ev.target === this.inputField && !(K>48 || K<57 || K===186 || K===189 || K===190 || K===32)) {
  return stopCalEvent(ev);
}

because I could not meet the condition, and using K > 48 || K < 57 makes no sense. It's possible that this was a mistake but the code doesn't contain any comments, sol....

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.1-dev labels Jan 26, 2024
@Fedik
Copy link
Member

Fedik commented Jan 26, 2024

Please use event.code instead of event.key, because the key is altered by keyboard locale and layout. And code always represents a physical key on the keyboard.

@dgrammatiko
Copy link
Contributor

It's possible that this was a mistake but the code doesn't contain any comments

the conditional is basically a way to ignore keys other than the ones actually used by the calendar but not sure if there’s a need for it. This is code from the early 2000’s so who knows anymore…

@C-Lodder
Copy link
Member Author

@Fedik Oops, done

@Fedik Fedik added the Feature label Jan 26, 2024
@Quy
Copy link
Contributor

Quy commented Jan 26, 2024

In RTL, the left key moves to the right and right key moves to the left. Not sure if this is the correct behavior as this is not the case without this PR.

@C-Lodder
Copy link
Member Author

@Quy Fixed RTL. Thanks for testing

@Fedik
Copy link
Member

Fedik commented Jan 28, 2024

@Quy are you sure?
To me, it sounds very strange, but I cannot really check, or find any info about it.
The codes represents a physical keys on keyboard, With physical direction Up/Down/Lft/Rgt which should not changes, no matter what.

@Quy
Copy link
Contributor

Quy commented Jan 29, 2024

I have tested this item ✅ successfully on 0625506


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42715.

@Quy
Copy link
Contributor

Quy commented Jan 29, 2024

@Fedik I was pointing out that this PR did the opposite for left/right keys which has been fixed since.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 0625506


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42715.

@Quy
Copy link
Contributor

Quy commented Jan 29, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42715.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 29, 2024
@LadySolveig LadySolveig added this to the Joomla! 5.1.0 milestone Jan 30, 2024
@LadySolveig LadySolveig merged commit 7849988 into joomla:5.1-dev Feb 2, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 2, 2024
@LadySolveig
Copy link

Thank you @C-Lodder and also for testing, support and review @dgrammatiko @Fedik @Quy @viocassel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.1-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants