implementing optional content radiobutton groups

[agrahn]

This PR implements OC radio button groups. It could resolve #18823.

The suggested code parses the /RBGroups entry in the optional content configuration dict and adds the property myRbGroups to instances of the OptionalContentGroup class. myRbGroups takes an array of Sets , where each Set instance represents a radio button group the OptionalContentGroup instance is a member of. Such a Set instance contains all OCG ids within the corresponding RB group. The RB groups an OCG is associated with are processed when its visibility is set to true, as required by the PDF spec.

[agrahn]

@Snuffleupagus I updated the PR with your version of pdf_layer_viewer.js from master. Only two changed files remain for review :).

[Snuffleupagus]

Thanks for the patch; based on an initial look I've left a number of inline comments.

Please remember to include the information from #18825 (comment) in the commit message as well, such that the patch is easier to understand also on the Git command line.

[agrahn]

Thank you, @Snuffleupagus, for your comments. I am going to work through all of them. Is it OK to do the code changes locally and to force-push them afterwards?

[agrahn]

Please remember to include the information from #18825 (comment) in the commit message as well, such that the patch is easier to understand also on the Git command line.

That's a lot of text. I thought a commit message should fit on a single line. But I will do so when appropriate.

[Snuffleupagus]

This also needs tests, and I'd suggest adding a couple of reference tests for this.

• Add the PDF document that you attached in [Feature]: Optional Content Radio Button Groups #18823 (comment) to the test/pdfs/ folder, but name it issue18823.pdf.
• Run node test/add_test.mjs test/pdfs/issue18823.pdf in the root of your local PDF.js Git-clone.
• Open the test/test_manifest.json file and find the newly added test-case. Here we also want to add another test-case for a non-default optionalContent-state.

To make this process slightly less daunting I've done these steps in the following diff, which you're obviously welcome to copy and include as-is in your patch: master...Snuffleupagus:pdf.js:issue-18823-test

[Snuffleupagus]

Can't we define this as a separate function similar to the other existing helpers, and just invoke it directly below, rather than using an IIFE here?

Suggested change

	// Parse RBGroups entry.
	(rbGroups => {
	parseRBGroups(rbGroups) {

[Snuffleupagus]

This function doesn't need to return anything.

Suggested change

	return null;
	return;

[Snuffleupagus]

Nit:

Suggested change

	const parsedRbGroup = new Set();
	const parsedRbGroups = new Set();

[Snuffleupagus]

Again, this function shouldn't need to return anything.

Suggested change

	return null;

[Snuffleupagus]

Nit: I'd suggest re-wording this slightly to remove my/I usage, and also let's fix the quoting around true.

Suggested change

	// If my visibility is about to be set to `true' and if I belong to one or
	// more radiobutton groups, hide all other OCGs in these radiobutton groups,
	// If the visibility is about to be set to `true` and the group belongs to
	// any radiobutton groups, hide all other OCGs in these radiobutton groups,

[Snuffleupagus]

Let's do the preserveRB check first, since without that nothing else matters here.

Suggested change

	if (visible && preserveRB && group.rbGroups.length) {
	if (preserveRB && visible && group.rbGroups.length) {

[Snuffleupagus]

Suggested change

	for (const rbGrp of group.rbGroups) {
	for (const rbGroup of group.rbGroups) {

[Snuffleupagus]

For added safety, to prevent any future errors here.

Suggested change

	this.#groups.get(otherId)._setVisible(INTERNAL, false, true);
	this.#groups.get(otherId)?._setVisible(INTERNAL, false, true);

[Snuffleupagus]

Removing the IIFE, this become.

Suggested change

	})(config.get("RBGroups"));
	}
	parseRBGroups(config.get("RBGroups"));