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

Example pages codeblock keyboard trap fix #1410

Conversation

katlich112358
Copy link
Contributor

Fixes #1372

Changes:
Ace.js recently added a way to prevent keyboard trapping (ajaxorg/ace#5114). This PR updates the ace.js files within p5's repo to their v1.5.0 version (https://github.com/ajaxorg/ace-builds/releases/tag/v1.5.0) and enables the keyboard accessibility features for code blocks.

@Qianqianye
Copy link
Contributor

Thanks @katlich112358 for working on it and @paulaxisabel for reviewing it. Hey @calebfoss, can you review this PR as well? We will merge this PR when it's approved by at least 2 mentors/maintainers. Thanks!

Copy link
Contributor

@calebfoss calebfoss left a comment

Choose a reason for hiding this comment

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

I'm seeing this error when opening the reference page:

Uncaught TypeError: DocumentedMethod is not a constructor        [reference.js:4920:16]

Not sure why, and I can take a closer look later.

@calebfoss
Copy link
Contributor

I'm stumped on that error. I'm not seeing the relationship between it and the files changed, but it is only thrown when these changes are applied. I'm not seeing Ace referenced in the reference.js file at all. Can someone more familiar with how reference.js works take a look at see if there are any clues?

@nickmcintyre
Copy link
Member

nickmcintyre commented Oct 9, 2023

@calebfoss @katlich112358 I just saw this error as well and managed to fix it locally. I used the most recent ace-builds to replace the following:

I don't actually understand why that worked. Probably an issue with the other ace files in those directories.

Copy link
Member

@nickmcintyre nickmcintyre left a comment

Choose a reason for hiding this comment

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

@katlich112358 I believe you just need to update the remaining ace files in src/assets/js/vendor/ace and src/assets/js/vendor/ace-nc with the most recent versions from ace-builds

@katlich112358
Copy link
Contributor Author

@nickmcintyre Great, thanks! Just to be sure I'm reading this correctly, replace the entire ace and ace-nc folders with the newer ace.js content? Or just the files that exist within p5's directory?

@nickmcintyre
Copy link
Member

@katlich112358 good clarification. I only replaced the files that exist within p5's directory.

Minor tangent:

I just skimmed default.hbs and it looks like ace-nc/ace.js and ace-nc/mode-javascript.js are the only ace.js files that are actually used. Just deleted the ace folder and everything in ace-nc except for the new ace.js and mode-javascript.js files. The site seems to work fine with only these files.

@limzykenneth
Copy link
Member

Just want to check, is this ready for review and merging?

@katlich112358
Copy link
Contributor Author

@limzykenneth I believe so, yep!

@limzykenneth limzykenneth merged commit 0dfe766 into processing:main Jan 21, 2024
1 check passed
@limzykenneth
Copy link
Member

Looks good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

p5.js Accessibility Audit Discussion
6 participants