Feature/2.x/72 module javascript needs improvement

[ctorgalson]

Closes #72

What does this change?

TL;DR: this addresses the accessibility, translatability, and theming issues in #72 without altering the themeable markup:

---

NOTE: if we we're willing to accept breaking changes (mainly to the markup), this could be rewritten much more efficiently.

---

• all text provided by the js is now passed through Drupal.t() (sometimes via theme functions),
• the buttons--including icons where relevant--are now provided by Drupal.theme functions that can be overridden by themes,
• the javascript is reasonably compact and much better organised (~65 lines of setup code calling 6 main functions and four Drupal.theme functions,
• removes aria-expanded attribute from "Show summaries" button,
• adds aria-controls attribute to "Show summaries" button,
• the javascript now sets aria-controls attributes on summaries, and a corresponding id attribute on the individual "Show summaries" buttons,
• the javascript uses Drupal.announce() to say "All summaries expanded" or "All summaries collapsed' when the "Show summaries" button is used,

How to test

• check out this branch of the module into an LGD site
• clear Drupal caches
• view one or more step by step pages
• expand and collapse some summaries
• expand and collapse the "Show summaries" button

How can we measure success?

With the following exceptions, there should be no differences in behaviour from the current implementation:

• screenreader users will no longer hear the "Show summaries" button's 'expanded' status when it's clicked (aria-expanded removed)
• screenreader users will now hear "All summaries expanded" or "All summaries collapsed" when the "Show summaries" button is used

Have we considered potential risks?

There are few risks as the rewrite goes to great lengths to preserve the prior version's markup.

Images

No changes.

Accessibility

• Tested with screen reader
• Navigable with keyboard

[ctorgalson]

@markconroy I know you've just gone off on holiday, but this was the PR I mentioned on #72.

[tonypaulbarker]

@ctorgalson There was some discussion about updating the step by step pattern so I think improving the mark up we currently have rather than wholesale changes is the right way to go for this particular MR.

[ctorgalson]

The problem is the javascript--it's currently creating the unthemeable, untranslatable markup, and it's much too tightly coupled to the existing twig markup.

The least impactful change in terms of existing implementations is to fix the js so the things it does can be translated and themed.

Fixing the twig markup (by e.g. outputting the buttons there) would be a great improvement, but would require a total rewrite of the js in that case too (making it much smaller and simpler on the way).

The TL;DR is the js needs to be rewritten.

This PR is the conservative approach, but I'd love it if we opted for a major version increment. That'd allow us to greatly simplify the script and handle 100% of the markup in twig.

[markconroy]

I think this looks great. happy to approve it.

I'd also like a follow-up issue to rewrite the JS and template (as a new major release perhaps) so it does all the things we want.

Great work. Thanks.

[tonypaulbarker]

@ctorgalson to look at test failures. Looks as though behat complains about a change of label.

[tonypaulbarker]

Marking as changes requested for addressing the tests

[ctorgalson]

Fixing tests will rely on an upstream issue with localgov_shared_workflows being solved.

Essentially, I need to run the tests locally to see what DOM structure gets created and why that behat test complains, and I can't correctly run the tests locally with that issue outstanding (afaict, the test is wrong about the result, but I can't prove it 😁)...

Edited to add much later: I changed the tests to dump HTML when they fail, and ran them here.

[finnlewis]

Some merge conflicts to review

[finnlewis]

Keen to pick this up again.

We just merged the ESLint fixes.

Can we resolve these conflicts then incorporate these changes?

@markconroy @tonypaulbarker perhaps we can review in the next week or two.

[ctorgalson]

@finnlewis, @markconroy, @tonypaulbarker I got the tests to pass. There was a sneaky regression in there. The tests will also now dump the .step-list HTML if one of the Functional Javascript tests fails.

@markconroy this includes the identical behaviour from your recent patch.

[ctorgalson]

@msayoung I've made one major and one minor accessibility change here that it'd be good for an expert to review.

Major: remove aria-expanded / aria-pressed from "Show summaries" button

I've removed the aria-expanded attribute from the "Show all summaries" button, and used Drupal.announce() to announce "summaries expanded" or "summaries collapsed". Here's the rationale:

• following Adrian Roselli's suggestion for this situation, removed aria-expanded:

  Use <button aria-pressed="true|false|mixed"> if you want to bulk expand / collapse a bunch of disclosures.

• but because the <button> text changes, followed MDN's advice not to use any aria attribute to indicate state:

  If we employ native HTML semantics with , we can toggle the label instead of toggling the pressed state, removing the need for the aria-pressed attribute.

• testing with Orca, I found the "Show summaries, Button" and "Hide summaries, Button" screenreader messages confusing because the 'hide' one is spoken immediately on expanding the button and the 'show' one is spoken immediately upon hiding them.

For that reason, I added the Drupal.announce() / aria-live messages, but I don't know if regular screenreader users would share my confusion about this interaction.

I'd be happy to rely on a static button text ("Show all summaries") and aria-pressed for the button, but this PR is meant to make the js themeable and translatable, without changing the UX.

Minor: begin using aria-controls attributes on individual "Show summary" buttons

I've also added aria-controls attributes to the original buttons and id attributes to the step containers since the buttons and divs are not siblings in the existing markup.