Support multiple settings being defined in *-commands.csv

[howard-e]

Preview Tests

Address #1002.

Note: Created #1126 to test the changes on tests/alert/data/jaws-commands.csv (these are not meant to replicate real world defined settings or scenarios):

• Alert review page showing that the JAWS command has an additional speechRateIncrease and commentAnnouncementOn setting. Also added a new Enter + virtualCursor + commentAnnouncementOn command
• Alert test page for JAWS commands accounting for multiple settings

[howard-e]

cc @mcking65

[jugglinmike]

This definition of settings isn't really appropriate in the presence of additionalSettings. Will this change also be reflected in the test format definition? What would be the ramifications of renaming this property to primarySetting?

[howard-e]

This definition of settings isn't really appropriate in the presence of additionalSettings ... renaming this property to primarySetting?

I agree! I figured as much when sharing the initial gist on what's different here. I proposed mainSetting then and have since used primarySetting here as well which feels more appropriate. Otherwise, it certainly presents a bit of confusion without this context.

Will this change also be reflected in the test format definition?

I don't think it necessary. Seems to be something to consider as an implementation detail.

What would be the ramifications ...

A widespread refactor across the references in this repository, aria-at-app (as well as some migration or backward compatibility work) and also in aria-at-automation-harness and related resources I assume. That will certainly need a bit of coordination that I'm uneasy trying to get that done now. Do you feel strongly otherwise?

[jugglinmike]

I agree that it's an implementation detail for the Community Group at large. That said, the amount of inter-project coordination needed to support change makes it feel like a public-facing design.

This change creates a gap between what the CG calls "settings" and what code maintainers call "settings", and to me, that's a problem which is distinct from the internally inconsistent names.

I dropped in from w3c/aria-at-automation-harness#73 to verify that my understanding of the situation was accurate. I trust your judgement when it comes to coordinating changes; I'd only advocate for prioritizing that refactoring sooner rather than later!

[howard-e]

@jugglinmike @outofambit having come back to this now, I've walked back on this implementation approach of "additionalSettings". Rather, sticking to handling the space separated string of "settings" as it is. My introducing "additional" settings represents another sense of settings priority that doesn't necessarily reflect the intention of #1002 (comment).

This change creates a gap between what the CG calls "settings" and what code maintainers call "settings", and to me, that's a problem which is distinct from the internally inconsistent names.

Fair point! I also started #1180 which I hope brings us to a "v1.5-esque" change where we can remove a lot of unneeded code, references which haven't been relevant for some time and further help standardize the language between the maintainers and the CG.

[stalgiag]

Looks great! I reviewed locally and tested making changes. The additional settings render as expected. Left a few comments inline but nothing blocking. Thanks Howard!

[stalgiag]

This structure with the primary setting as the first item in a space-separated collection feels a bit fragile. I'd rather have this as a distinct column. It feels like there is significance not surfaced by a quick read of the input. I assume that this is a design decision towards a better authoring experience so I support leaving as-is for now

[howard-e]

Relevant to my reply here, I agree with that point. I've reverted this approach

[stalgiag]

Could also possibly be combined with the similar code above?

[howard-e]

This has been changed slightly. Do you still feel the same here?

[stalgiag]

I don't feel strongly but I am looking at those lines that are identical and wondering if a getScreenTextRender() might replace them

const hasScreenText = settingsText && settings !== 'defaultMode';
const hasadditionalSettings = hasScreenText && additionalSettingsExpanded.length > 0;

let screenTextRender = hasScreenText ? ` (${settingsText}` : '';
if (hasadditionalSettings) {
      additionalSettingsExpanded.forEach(({ settingsText }) => {
        screenTextRender = `${screenTextRender} and ${settingsText}`;
      });
    }
screenTextRender = `${screenTextRender})`;

[howard-e]

Done in d3e5b14.

Note I: That snippet posted above was changed as well since I've dropped the "additional settings" concept but the reusable utilities is definitely the direction to go for a lot of this file.
Note II: Also planning to bring wider changes to promote more reusability and clean up through #1180.

[stalgiag]

Oh! My apologies. Thanks for addressing even though it was outdated haha! 😅

[stalgiag]

Thanks for taking the time to rework this is! I agree with the decisions made here and have confirmed this continues to work as expected.