accessModeSufficient check is broken, as it will not accept a list of more than two comma-separated values

[JohnThomson]

Ace version 1.1.1
Node version 12.14.0
Windows 10

For example, if content.opf contains <opf:meta property="schema:accessModeSufficient">textual,visual,auditory</opf:meta>
the ACE json report contains
"assertions": [
{
"@type": "earl:assertion",
"earl:result": {
"earl:outcome": "fail",
"dct:description": "'schema:accessModeSufficient' metadata must be set to one of the expected values"
},
"earl:assertedBy": "Ace",
"earl:mode": "automatic",
"earl:test": {
"earl:impact": "moderate",
"dct:title": "metadata-accessmodesufficient-invalid",
"dct:description": "Value 'visual,auditory' is invalid for 'schema:accessModeSufficient' metadata",
"help": {
"url": "http://kb.daisy.org/publishing/docs/metadata/schema-org.html",
"dct:title": "Schema.org Accessibility Metadata",
"dct:description": "Use one of the metadata values defined by schema.org"
},
"rulesetTags": [
"EPUB"
]
}
}
],
Note that it has evaluated the 2nd and 3rd elements in the list as a single item "visual,auditory"

I found the problem on line 78 of checker-epub.js, where replace(',', ' ') only replaces the first comma.
It should be replace(/,/g, ' ')

The attached zip is actually a complete epub. Not a minimal demonstration, but it does produce the error.

Hoping to submit a PR shortly...

Atchoum !-français.zip

[JohnThomson]

#332 should fix this.

Note: I tested it on the attached book, but was unable to get unit tests to run with or without the proposed change to test data. "yarn test" (after yarn and yarn install) just complains that " [31724]: c:\ws\src\node_file.cc:1337: Assertion `(argc) >= (3)' failed." and I can't figure out what more arguments it wants.

[danielweck]

Thank you for reporting this bug and for proposing a fix.

However, I believe this problem - as well as a few others - was fixed some time ago in the "next" version of Ace, currently distributed as release 1.2.0-beta.12 (note that although it is labelled as "beta", it actually ships in Ace App which appears to be satisfactorily stable, see https://github.com/daisy/ace-gui/releases )

NPM package tagged as next:
https://www.npmjs.com/package/@daisy/ace/v/next
(note that Ace version 1.1.1 is published under the latest NPM tag: https://www.npmjs.com/package/@daisy/ace/v/latest )

Pending Pull Request, which will supersede the master branch / version 1.1.1:
#314
...associated ace-next branch:
https://github.com/daisy/ace/tree/ace-next

Would you mind running your test against Ace version 1.2.0-beta.12? You can install it globally using npm -g install ace@next or locally in your project (i.e. node_modules folder) or using a temporary execution with npx ace@next {COMMAND_LINE_PARAMS}.

[danielweck]

For future reference, here is the fix that ships in 1.2.0-beta.12:

ace/packages/ace-core/src/checker/checker-epub.js

Lines 103 to 109 in a6fbed4

	// comma-separated only! (not space-separated)
	// regexp note: /\s\s+/g === /\s{2,}/g
	// no whitespace collapsing, individual items can contain (incorrect) whitespaces, which will be reported
	const splitValues =
	name === 'schema:accessModeSufficient' ?
	value.trim().split(',').map(item => item.trim()).filter(item => item.length) :
	[value];

...versus:

https://github.com/daisy/ace/pull/332/files#diff-998b7bddbd544ebbd20af762228e4167L162-L164

[danielweck]

Also note the associated unit tests:

ace/tests/__tests__/epub-rules.test.js

Lines 57 to 152 in a6fbed4

	test('missing metadata is reported', async () => {
	const report = await ace('../data/epubrules-metadata');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessibilitysummary',
	}),
	}),
	]));
	});
	test('incorrect metadata value is reported', async () => {
	const report = await ace('../data/epubrules-metadata');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessmode-invalid',
	}),
	}),
	]));
	});
	test('accessibilityControl metadata is multiple correct items but only single value is allowed', async () => {
	const report = await ace('../data/epubrules-metadata');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessibilitycontrol-invalid',
	}),
	}),
	]));
	});
	test('accessModeSufficient metadata is missing', async () => {
	const report = await ace('../data/epubrules-metadata-accessModeSufficient-missing');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessmodesufficient',
	}),
	}),
	]));
	});
	test('accessModeSufficient metadata is valid', async () => {
	const report = await ace('../data/epubrules-metadata-accessModeSufficient-valid');
	expect(report['earl:result']['earl:outcome']).toEqual('pass');
	// const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	// expect(assertions).not.toEqual(expect.arrayContaining([
	// expect.objectContaining({
	// 'earl:test': expect.objectContaining({
	// 'dct:title': 'metadata-accessmodesufficient-invalid',
	// }),
	// }),
	// ]));
	});
	test('accessModeSufficient metadata invalid separator', async () => {
	const report = await ace('../data/epubrules-metadata-accessModeSufficient-invalid-separator');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessmodesufficient-invalid',
	}),
	}),
	]));
	});
	test('accessModeSufficient metadata invalid item', async () => {
	const report = await ace('../data/epubrules-metadata-accessModeSufficient-invalid-item');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessmodesufficient-invalid',
	}),
	}),
	]));
	});
	test('accessibilityFeature case-sensitivity', async () => {
	const report = await ace('../data/epubrules-metadata-accessibilityFeature-case-sensitive');
	expect(report['earl:result']['earl:outcome']).toEqual('fail');
	const assertions = findAssertionsForDoc(report, 'EPUB/package.opf');
	expect(assertions).toEqual(expect.arrayContaining([
	expect.objectContaining({
	'earl:test': expect.objectContaining({
	'dct:title': 'metadata-accessibilityfeature-invalid',
	}),
	}),
	]));
	});

ace/tests/data/epubrules-metadata-accessModeSufficient-invalid-item/EPUB/package.opf

Lines 8 to 14 in a6fbed4

	<meta property="schema:accessibilityFeature">structuralNavigation</meta>
	<meta property="schema:accessibilitySummary">everything OK!</meta>
	<meta property="schema:accessibilityHazard">noFlashingHazard</meta>
	<meta property="schema:accessibilityHazard">noSoundHazard</meta>
	<meta property="schema:accessibilityHazard">noMotionSimulationHazard</meta>
	<meta property="schema:accessMode">visual</meta>
	<meta property="schema:accessModeSufficient">visual,other,textual</meta>

ace/tests/data/epubrules-metadata-accessModeSufficient-invalid-separator/EPUB/package.opf

Lines 8 to 14 in a6fbed4

	<meta property="schema:accessibilityFeature">structuralNavigation</meta>
	<meta property="schema:accessibilitySummary">everything OK!</meta>
	<meta property="schema:accessibilityHazard">noFlashingHazard</meta>
	<meta property="schema:accessibilityHazard">noSoundHazard</meta>
	<meta property="schema:accessibilityHazard">noMotionSimulationHazard</meta>
	<meta property="schema:accessMode">visual</meta>
	<meta property="schema:accessModeSufficient">visual textual</meta>

ace/tests/data/epubrules-metadata-accessModeSufficient-missing/EPUB/package.opf

Lines 8 to 13 in a6fbed4

	<meta property="schema:accessibilityFeature">structuralNavigation</meta>
	<meta property="schema:accessibilitySummary">everything OK!</meta>
	<meta property="schema:accessibilityHazard">noFlashingHazard</meta>
	<meta property="schema:accessibilityHazard">noSoundHazard</meta>
	<meta property="schema:accessibilityHazard">noMotionSimulationHazard</meta>
	<meta property="schema:accessMode">visual</meta>

ace/tests/data/epubrules-metadata-accessModeSufficient-valid/EPUB/package.opf

Lines 8 to 16 in a6fbed4

	<meta property="schema:accessibilityFeature">structuralNavigation</meta>
	<meta property="schema:accessibilitySummary">everything OK!</meta>
	<meta property="schema:accessibilityHazard">noFlashingHazard</meta>
	<meta property="schema:accessibilityHazard">noSoundHazard</meta>
	<meta property="schema:accessibilityHazard">noMotionSimulationHazard</meta>
	<meta property="schema:accessMode">visual</meta>
	<meta property="schema:accessModeSufficient"> visual , , textual </meta>
	<meta property="schema:accessModeSufficient"> visual , textual </meta>
	<meta property="schema:accessModeSufficient">visual,textual</meta>

ace/tests/data/epubrules-metadata-accessibilityFeature-case-sensitive/EPUB/package.opf

Lines 8 to 16 in a6fbed4

	<meta property="schema:accessibilityFeature">mathML</meta>
	<meta property="schema:accessibilitySummary">everything OK!</meta>
	<meta property="schema:accessibilityHazard">noFlashingHazard</meta>
	<meta property="schema:accessibilityHazard">noSoundHazard</meta>
	<meta property="schema:accessibilityHazard">noMotionSimulationHazard</meta>
	<meta property="schema:accessMode">visual</meta>
	<meta property="schema:accessModeSufficient"> visual , , textual </meta>
	<meta property="schema:accessModeSufficient"> visual , textual </meta>
	<meta property="schema:accessModeSufficient">visual,textual</meta>

[JohnThomson]

Yes, the next version works correctly. (I found to install it I actually needed npm install -g @daisy/ace@next).

Any idea when this will be stable? The instructions for installing Ace by DAISY ship as part of our program (bloom, bloomlibrary.org), and while your 'next' branch may be stable enough to recommend to our users right now, will it always be?

[danielweck]

Yes, the "next" branch and corresponding NPM version (currently '1.2.0-beta.12') will either continue to receive updates (as part of "beta" or "release candidate" builds), or will become the main release channel in order to supersede/deprecate v'1.1.1' / current "master" branch. The "next" branch is effectively the active development channel for Ace and Ace App releases. There have been occasional code fixes directly in master, but "next" is guaranteed to include all of "master"'s history. We have been Keeping "master" alive because of initial concerns for supporting legacy / old NodeJS runtimes (which "next" doesn't support anymore, due to alignment with LTS Long Term Support NodeJS versions). We were also cautious (perhaps over cautious) to ensure sufficient field testing of Ace "beta" via the Ace App releases, and the odd 1.1 user interested in using the 1.2 release stream. We are now pretty confident that it is time to elevate 1.2 to the status of stable, and thereby make 1.1 obsolete. I anticipate this to happen by the end of the year.

[danielweck]

ace-next will be published shortly to replace Ace 1.1.1
#314

This issue can now be closed.