Add describe() and describeElement()

[lm-n]

Changes:

• create folder in source for accessibility features
• src/accessibility/describe.js adds two functions describe() and describeElement() to the p5.js library that make adding fallback descriptions of the canvas easy for p5.js users. Descriptions generated with these functions can be useful for screen reader users and for search engines.
  • describe() function
  • describeElement() function
• in app.js import './accessibility/describe.js'
• in constants adds constants for functions describe() and describeElement()

PR Checklist

• npm run lint passes
• [Inline documentation] is included / updated
• [Unit tests] are included / updated

Pending:

• Styling of examples in reference. Currently the examples look ugly. @kjhollen and I looked into it but unsure of how to proceed/fix them. @lmccart perhaps you have some ideas of how we could approach this.

[kjhollen]

Hey Luis, this is in great shape! I've asked some minor questions and made some suggestions in the review. The one big thing to do is to write some unit tests for this new functionality. I'll look for the appropriate guides before we meet today—it's been a while since I've done this, so we can look it over together.

[lm-n]

Hi Luis, thanks for your patience while I've been moving! The updates and added tests look great. I found one minor typo and suggest adding a couple more test cases (see comments inline), then this will be ready to go. Great work!

Hi Kate! No worries! Just added the test cases and "!" "?" exceptions in the regex. 😸

[lmccart]

🎉 Let mostly code readability comments on this. Main things to look out for:

• writing out variable and function names for readability
• instead of checking (blah === null) or (blah === false), you can write (!blah)
• internal helper functions don't need to be attached to p5.prototype

also wondering if performance-wise it's useful to store any of the DOM elements instead of needing to use getElementByID

[lm-n]

@lmccart thx for the feedback! I've made all of the suggested changes! 😺

@kjhollen I still got a couple of new issues with tests —that came up with the new changes — to fix before merge, will comment on them in case you get a chance to look at them. I should be able to work through them before our next meeting.

[akshay-99]

Hey @lm-n! So I took a quick look at the tests ( I hope you don't mind 😅 ).
My understanding here is quite limited but I think they are failing at the moment because the dummy variable in describe.js still remains populated after a test completes. So what I saw was that the first test that called describe() ran perfectly but subsequent tests didn't.

That's probably because even though the p tag which is created during a test, is deleted in teardown(), the dummy variable is not cleared. So we won't get a new p tag for the next test. And thus the error Uncaught Error: Uncaught TypeError: Cannot read property 'innerHTML' of null (http://localhost:9001/test/unit/accessibility/describe.js:45).

If we have a new internal function as

p5.prototype._clearDummy = function() {
  dummy = { fallbackElements: {}, labelElements: {} };
}

and call this in teardown(), it should fix this problem.

Also, I didn't see the need for using setTimeout() for testing as all of the code in describe.js is synchronous.

[lm-n]

Thanks @kjhollen and @akshay-99 ❤️ ❤️ ❤️ ! Akshay you were right! We had some problems with dummy beyond what you identified. It should work now 👯‍♀️ 👯‍♂️ !

[kjhollen]

comments are a great addition! I think the dummy DOM is a little different than I'd expected, but is in good shape (I think I just thought there would be a variable attached to the p5 instance for the description DOM nodes). More context in comment inline.

[kjhollen]

I'm curious why this method is attached to the prototype but not the fallback elements or label elements. Wondering if it makes sense for fallbackElements and labelElements to be attached to a p5 instance? (I recall that this got added to fix the tests, am wondering if attaching to the instance would also fix that or if there are other possible advantages)

[kjhollen]

I built everything this morning to double check that tests are passing and all seems to be in good shape! I think we can merge this as-is.

@lm-n and I chatted yesterday and thought there might be a way to move the "dummy" object into the p5 instance, to simplify some of the element lookup & tests. This part of the code is also used in #4703, so we thought that work could continue there—and that it makes sense to merge this patch so that the code doesn't need to be updated in 2 different branches. if that sounds ok to you @lmccart I'll merge this later today!

[kjhollen]

hey @lm-n I was double-checking this with a merge from main branch locally on my machine and hit some snags (that do not appear to be in your code). I repeatedly get an error that says:

No parser and no filepath given, using 'babel' the parser now but this will throw an error in the future. Please specify a parser or a filepath so one can be inferred.

Then some of the examples (in p5.Font.js, p5.Image.js, vertex.js, image.js, pixels.js, dom.js, files.js, and p5.Graphics.js) seem to have trouble with the linter (prettier) in the task eslint-samples:source build. It looks like the linter now wants a bunch of parenthesis around some math. Wondering if anyone else can advise on what's happening?

I'm not sure if it's safe to merge so I'm going to hold off for now! But knowing that you've got newer work ready to go / review, I'm hoping we can still get this resolved quickly.

[lmccart]

@kjhollen all good to merge by me once these last little issues get resolved

[kjhollen]

ok, I realized that I had locally run npm update because I saw:

Browserslist: caniuse-lite is outdated. Please run next command `npm update`

which updated my package-lock.json and package.json files to versions beyond what the project expected, and caused the issues I wrote about yesterday. I figured this out by doing a clean checkout of the main branch, where everything worked ok until I ran npm update, when I also noticed the additional files had been changed. (Posting the solution here in case it's useful to someone later, but my mistake 😅)

Anyway, it was a clean build once I figured this out and all tests pass, so I'm going to merge!

Congrats @lm-n! 🎉