Turning on accessible canvas settings causes sketches to break

[davepagurek]

Details about the bug:

• p5.js version: 1.6.0
• Web browser and version: Firefox 111.0.1 (also reported from another user on Chrome)
• Operating System: MacOS 12.5.1
• Steps to reproduce this:

Example sketch:

function setup() {
  createCanvas(400, 400);
}

function draw() {
  background('red');
}

Then check this box in settings:

While normally this would just make a red canvas, with that setting turned on, we just get an error:

The error is coming from the p5.accessibility library editing the behaviour of colour functions:

Looking at the repo here https://github.com/processing/p5.accessibility it seems like it's no longer maintained. Is there an alternative that can be used? In the mean time, it seems to be broken.

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

[lindapaiste]

Good catch! It's a real bummer that this causes the whole sketch not to render.

Looking at the repo here https://github.com/processing/p5.accessibility it seems like it's no longer maintained. Is there an alternative that can be used? In the mean time, it seems to be broken.

I started writing out a bunch of stuff about wrapping the p5.accessibility library to catch errors, but then I realized that the accessible output functionality has been merged into the core p5.js package for quite a while.

We should be using the accessible output from p5.js rather than adding the p5.accessibility script. So I need to figure out how to configure that. We potentially need to check the p5 version and show a "friendly error" if the user has accessible outputs turned on but the p5 version in their code is not high enough.

what I started writing

I wonder if it's possibly for us to put some sort of wrapper around the p5.accessibility function calls with a try/catch to prevent them from breaking the canvas output. This would go in the "previewApp" code. The p5.accessibility code runs automatically based on the presence of certain HTML ids, but all of its variables are available in the global/window scope of the preview iframe.

I need to test this out, but it's something like:

if (window.baseInterceptor && window.baseInterceptor.prototype) {
  const originalFn = window.baseInterceptor.prototype.getColorName;
  window.baseInterceptor.prototype.getColorName = function(colArgs) {
    try {
      return originalFn(colArgs);
    } catch (e) {
      // or in this case, we do know the color 'red' and it's in colArgs[0]
      return `Unknown color `${colArgs.join(', ')}`;
    }
  }
}

Or maybe a broader wrapper, like around all functions on window.baseInterceptor.prototype.

The hard part would be making sure that our replacement executes before the package executes.

I'm so confused by this code because it looks like they're checking for a "does not match" ! when it should be a "does match"?
https://github.com/processing/p5.accessibility/blob/165b836a8153aeb0529a678a211efac112f3d4fe/src/baseInterceptor.js#L88-L90

[davepagurek]

I'm not super familiar with the accessibility features of p5, is this https://p5js.org/reference/#/p5/textOutput sort of what the accessibility script was doing before? Would we need to inject textOutput() or gridOutput() into scripts when these web editor settings are enabled?

[lindapaiste]

I'm not super familiar with the accessibility features of p5, is this https://p5js.org/reference/#/p5/textOutput sort of what the accessibility script was doing before? Would we need to inject textOutput() or gridOutput() into scripts when these web editor settings are enabled?

Me neither! We need to inject those functions but I need to figure out where and how. I think it only needs to be called one time in setup() rather than on every draw()?

If that's the right move then we would need to A) add a line in the setup() if that function already exists B) add a setup() function if there is not one in the script already. Messing with JS files as text strings could be fragile as it's easy to mess up if the user writes code abnormally. I don't know if it's more durable to parse it as an Abstract Syntax Tree using jscodeshift? The easiest thing would be to not modify the user's code at all, and instead just add an additional script file with some global call but I'm not sure if that works? Possibly works when using p5 globally like most users of the editor do. Almost certainly would not work with instance mode.

Here's how the p5.accessibility package works:

1. Global script runs once when the .js file is loaded.
2. It defines a global variable allData which is a huge (688KB) JSON object containing schemas for all of the p5 classes and methods that it should "spy" on. This was last updated in 2017!
3. It looks for DOM elements with ids 'textOutput-content' and/or 'tableOutput-content' and uses these as containers to render its output. If neither is found then it doesn't do anything.
4. It wraps all of the spied-on methods on the global p5.prototype object. Each method is replaced with a version that generates and renders the accessible output each time that it is called.

Based on what I'm seeing, I doubt that it does anything when using p5 in instance mode?

The current web editor handling for accessible outputs is here:

p5.js-web-editor/client/modules/Preview/EmbedFrame.jsx

Lines 227 to 248 in 2cd9067

	const accessiblelib = sketchDoc.createElement('script');
	accessiblelib.setAttribute(
	'src',
	'https://cdn.jsdelivr.net/gh/processing/p5.accessibility@0.1.1/dist/p5-accessibility.js'
	);
	const accessibleOutputs = sketchDoc.createElement('section');
	accessibleOutputs.setAttribute('id', 'accessible-outputs');
	accessibleOutputs.setAttribute('aria-label', 'accessible-output');
	if (textOutput || gridOutput) {
	sketchDoc.body.appendChild(accessibleOutputs);
	sketchDoc.body.appendChild(accessiblelib);
	if (textOutput) {
	const textSection = sketchDoc.createElement('section');
	textSection.setAttribute('id', 'textOutput-content');
	sketchDoc.getElementById('accessible-outputs').appendChild(textSection);
	}
	if (gridOutput) {
	const gridSection = sketchDoc.createElement('section');
	gridSection.setAttribute('id', 'tableOutput-content');
	sketchDoc.getElementById('accessible-outputs').appendChild(gridSection);
	}
	}

It's all in the index.html file of the project rather than the .js.

If the textOutput or gridOutput options are enabled, then it injects a <script src="https://cdn.jsdelivr.net/gh/processing/p5.accessibility@0.1.1/dist/p5-accessibility.js"> tag into the document and also injects the necessary output containers <section id="textOutput-content"> or <section id="tableOutput-content">.

[lindapaiste]

This is somewhat tangential, but I noticed that we have a property in the redux state, state.ide.isAccessibleOutputPlaying which does not do anything at the moment.

p5.js-web-editor/client/modules/IDE/pages/IDEView.jsx

Lines 368 to 373 in 269ca22

	<div>
	{((this.props.preferences.textOutput ||
	this.props.preferences.gridOutput) &&
	this.props.ide.isPlaying) ||
	this.props.ide.isAccessibleOutputPlaying}
	</div>

We have this very perplexing line in the IDEView which is just a div with a boolean in it. It does not do anything, and renders to the DOM as and empty div.

I had to go way back in the history to figure out what this was supposed to be doing. The boolean value was previously used to conditionally render an AccessibleOutput component. Since this change in 2018 (PR #508) it does not do anything.

Where it's set
There is a second version of the Play button which is hidden off-screen and intended to be seen by screen readers. When this button is pressed, it dispatches a startAccessibleSketch action instead of the normal startSketch action. That action sets the isAccessibleOutputPlaying flag in the state to true. Clicking that button also dispatches setTextOutput(true) and setGridOutput(true), so we are enabling the accessible output (by setting the textOutput and gridOutput preferences which are normally set through the Preferences modal). Therefore the isAccessibleOutputPlaying flag is redundant and unnecessary.

[davepagurek]

If that's the right move then we would need to A) add a line in the setup() if that function already exists B) add a setup() function if there is not one in the script already.

Another option which might be easier than parsing the source code could be to make something that works like a p5 plugin and register a method to run when the sketch is initialized. p5 has an internal _setup that is always called regardless of whether there's a proper setup function, so we could maybe augment the existing _setup to call textOutput if it hasn't already been called:

p5.prototype.ensureAccessibleCanvas = function() {
  const prevSetup = this._setup;
  this._setup = () => {
    prevSetup();

    // Call textOutput if it has not already been called
    if (!this._accessibleOutputs.text) {
      this.textOutput();
    }
  };
};
p5.prototype.registerMethod('init', p5.prototype.ensureAccessibleCanvas);

[lindapaiste]

@davepagurek That sounds much smarter! Thanks for your insight on this. I haven't used registerMethod before so I wasn't sure how this could be done on a global level.