Add Checkboxes and Radios conditional reveal #616

NickColley · 2018-03-22T13:58:36Z

This pull requests adds the conditional reveal functionality for radios and checkboxes.

It replicates the behaviour in GOV.UK Elements, see known issues below for possible improvements at a later date.

I took the initial code in elements, rewrote the tests, made sure they were passing against the old code. Then refactored into modules as seen in this pull request.

It also includes:

Macro interface
New polyfills

Manual tests

Browsers

IE8+
Latest Safari, Chrome, Firefox, Edge

IE8

IE9+

Safari

Chrome

Firefox 58 with custom colours

AT testing

Custom colours
VoiceOver on Safari
Checkboxes: Email, unchecked, checkbox > Email checked > Email address, edit text
Radios: Email, collapsed, radio button, 1 of 3. > Email, selected expanded, radio button, 1 of 3 > Email address, edit text
JAWS17/18 on IE11
Checkboxes: Email checkbox, not checked > Checked collapsed; Checked expanded > Email address. How do you want to be contacted? Email address edit. Type in text.
Radios: Email radio button, checked. Email address. How do you want to be contacted? Email address edit. Type in text.
Note: on checkboxes, the initial state is being announced before the final state (i.e. Checked collapsed; Checked expanded); however this behaviour is consistent with Elements so we'll try to address this as a separate PR.
NDVA on Firefox52/IE11
Checkboxes: Email checkbox collapsed not checked/Checked expanded > Email address edit.
Radios: Email radio button expanded 1 of 3 > Email address edit.
Dragon Naturally Speaking on Firefox52/IE11

TODO:

Complete cross browser testing
Complete assistive technology testing
Add template tests
Add accessibility acceptance criteria for radios

Known issues

Radios are not announcing as expected, this is an issue seen in GOV.UK Elements too: alphagov/govuk_elements#575

Review links:

https://trello.com/c/R5WwqbpH/803-5-add-conditional-reveal-script-to-radios-and-checkboxes

NickColley · 2018-03-26T15:46:05Z

This is blocked on modules, since polyfills overlap components and we'll have a massive amount of duplication between components. See #619

NickColley · 2018-04-19T10:06:01Z

src/checkboxes/template.njk

@@ -20,6 +30,11 @@
        for: id
      }) | indent(4) | trim }}
    </div>
+    {% if item.conditional %}
+      <div class="govuk-checkboxes__conditional" id="{{ conditionalId }}">
+        {{ item.conditional.html | safe }}


This feels like a big security risk considering we expect people to put html in here that contains form inputs which often replay user input...

src/radios/radios.test.js

+      expect(isVisible).toBeTruthy()
+    })
+  })
+  describe('must be hidden if JavaScript is avaliable and is collapsed', () => {


NickColley · 2018-04-23T15:34:42Z

src/checkboxes/_checkboxes.scss

+    border-left: $conditional-border-width solid $govuk-border-colour;
+
+    // If JavaScript fails, make sure conditional content is not hidden.
+    .js-enabled &[aria-hidden="true"] {


Investigate if we can do this without a global class

alex-ju

Reviewed offline yesterday. Required changes were made. Did a second round of tests today.
👍 Great work in here. We should try to align the other scripts to this approach.

Don't forget to update the changelog and rebase/mark the fix commits to squash them.

NickColley · 2018-04-24T08:34:58Z

@alex-ju just need to clean the tests up, and then this'll be ready to go.

kr8n3r · 2018-04-24T08:58:47Z

TODO:
changelog

36degrees · 2018-04-24T09:26:20Z

src/checkboxes/checkboxes.test.js

+})
+
+describe('Checkboxes', () => {
+  describe('must be visible as static content if JavaScript is unavaliable or fails', () => {


It'd be good to nail these describe and it blocks – it's unclear to me reading this what we're trying to test and what is context.

For example 'with no aria attributes' and 'with visible content' as currently written could easily be interpreted as context ('when the thing does not have aria attributes', 'when the thing has visible content').

I'd suggest this would be better as something like:

Describe checkboxes with conditional reveals Describe when Javascript is unavailable or fails It does not have any ARIA attributes It makes the conditional content visible

The benefit of this structure is that the nesting still 'works', but each line also stands on its own – describe blocks read as context and it blocks read as assertions.

Also, typo on unavaliable (appears a few times)

yeah like I said above, I'm cleaning these up after the review with alex and jani

36degrees · 2018-04-24T09:26:49Z

src/checkboxes/checkboxes.test.js

+      const $ = await goToAndGetComponent('checkboxes', 'with-conditional')
+      const $component = $('.govuk-checkboxes')
+
+      const hasAriaHidden = $component.find('[aria-hidden]').length


The checkbox itself would never have aria-hidden right? That attribute would be on the conditional content? Should this be a separate test?

.find will look for any child element with aria-hidden so this should catch any issues.

Makes sense, though not sure that's clear. Can we be more specific and check the elements that we put the aria attributes on?

Out of date

alex-ju

Went through the code again. Looks good to be merged to me.

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 March 22, 2018 13:58 Inactive

NickColley changed the title ~~WIP: Add checkbox conditional reveal~~ WIP: Add conditional reveal Mar 22, 2018

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 March 22, 2018 14:48 Inactive

NickColley force-pushed the conditional-styles branch from 29aa807 to 1964600 Compare April 17, 2018 10:12

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 14:26 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 15:39 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 15:51 Inactive

NickColley force-pushed the conditional-styles branch from 461d8b0 to 335704f Compare April 17, 2018 15:58

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 15:58 Inactive

NickColley force-pushed the conditional-styles branch from 335704f to 5a13bbd Compare April 17, 2018 16:15

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:15 Inactive

NickColley force-pushed the conditional-styles branch from 5a13bbd to 4ab7a08 Compare April 17, 2018 16:17

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:17 Inactive

NickColley force-pushed the conditional-styles branch from 4ab7a08 to 3035af8 Compare April 17, 2018 16:20

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:20 Inactive

NickColley force-pushed the conditional-styles branch from 3035af8 to d1b6a4c Compare April 17, 2018 16:34

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:35 Inactive

NickColley force-pushed the conditional-styles branch from d1b6a4c to 6581d52 Compare April 17, 2018 16:36

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:37 Inactive

NickColley force-pushed the conditional-styles branch from 6581d52 to 02f98c6 Compare April 17, 2018 16:51

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:51 Inactive

NickColley force-pushed the conditional-styles branch from 02f98c6 to 3815bd6 Compare April 17, 2018 16:57

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 17, 2018 16:57 Inactive

NickColley force-pushed the conditional-styles branch from 3815bd6 to 3e5390e Compare April 19, 2018 09:57

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 19, 2018 09:57 Inactive

NickColley mentioned this pull request Apr 19, 2018

Use consistent initialise logic for modules #666

Closed

NickColley force-pushed the conditional-styles branch from 3e5390e to 6f23c7c Compare April 19, 2018 10:04

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 19, 2018 10:04 Inactive

NickColley commented Apr 19, 2018

View reviewed changes

NickColley commented Apr 23, 2018

View reviewed changes

src/radios/radios.test.js Outdated

expect(isVisible).toBeTruthy()

})

})

describe('must be hidden if JavaScript is avaliable and is collapsed', () => {

This comment was marked as resolved.

Sign in to view

NickColley commented Apr 23, 2018

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 23, 2018 16:40 Inactive

alex-ju previously approved these changes Apr 24, 2018

View reviewed changes

36degrees reviewed Apr 24, 2018

View reviewed changes

NickColley force-pushed the conditional-styles branch from 4051b11 to 01fd602 Compare April 24, 2018 15:42

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 24, 2018 15:43 Inactive

NickColley changed the title ~~Add conditional reveal~~ Add Checkboxes and Radios conditional reveal Apr 24, 2018

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 24, 2018 15:48 Inactive

NickColley force-pushed the conditional-styles branch from fd07ef4 to d82790b Compare April 24, 2018 15:59

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-616 April 24, 2018 15:59 Inactive

NickColley added 7 commits April 24, 2018 17:01

Add checkbox conditional reveal

02ffa5d

Add radio conditional reveal

f294097

Add polyfills used in Checkbox and Radios

4445060

Move nodeListForEach into shared common

90cf86f

Add radio template tests for conditional feature

efb2a80

Add checkboxes template test for conditional feature

ef06813

Update CHANGELOG

0a0ce3c

NickColley force-pushed the conditional-styles branch from d82790b to 0a0ce3c Compare April 24, 2018 16:01

govuk-design-system-ci deployed to govuk-frontend-review-pr-616 April 24, 2018 16:01 View deployment

alex-ju approved these changes Apr 25, 2018

View reviewed changes

NickColley merged commit 9d0010e into master Apr 25, 2018

NickColley deleted the conditional-styles branch April 25, 2018 16:23

kr8n3r mentioned this pull request May 18, 2018

Release v0.0.29-alpha #700

Merged

36degrees mentioned this pull request Jun 22, 2018

Radios labels are full width - should be the width of the label text? #543

Closed

NickColley mentioned this pull request Sep 7, 2018

Conditionally revealing content with checkboxes not announcing collapsed/expanded state on macOS with VoiceOver #989

Closed

colinrotherham mentioned this pull request Dec 7, 2022

Use polyfills from npm package #3082

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Checkboxes and Radios conditional reveal #616

Add Checkboxes and Radios conditional reveal #616

NickColley commented Mar 22, 2018 •

edited by alex-ju

Loading

NickColley commented Mar 26, 2018

NickColley Apr 19, 2018

NickColley Apr 19, 2018

This comment was marked as resolved.

NickColley Apr 23, 2018

alex-ju left a comment •

edited

Loading

NickColley commented Apr 24, 2018

kr8n3r commented Apr 24, 2018

36degrees Apr 24, 2018

36degrees Apr 24, 2018

NickColley Apr 24, 2018

36degrees Apr 24, 2018

NickColley Apr 24, 2018

36degrees Apr 24, 2018

NickColley Apr 24, 2018

alex-ju left a comment

Add Checkboxes and Radios conditional reveal #616

Add Checkboxes and Radios conditional reveal #616

Conversation

NickColley commented Mar 22, 2018 • edited by alex-ju Loading

Manual tests

Browsers

AT testing

Known issues

NickColley commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

alex-ju left a comment • edited Loading

Choose a reason for hiding this comment

NickColley commented Apr 24, 2018

kr8n3r commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju left a comment

Choose a reason for hiding this comment

NickColley commented Mar 22, 2018 •

edited by alex-ju

Loading

alex-ju left a comment •

edited

Loading