Ch1972 add accessibility check for checkbox and radio #1996

cmcculloh-kr · 2017-06-21T13:10:57Z

Fixes #1972

Adds a console error message if visibility: hidden or visibility: collapse is applied to the checkbox.

Cleans up checkbox unit tests and makes UMD wrapper more consistent across all core fuel ux files.

If you are getting the following error:
For accessibility reasons, in order for tab and space to function on [checkbox|radio], visibilitymust not be set tohiddenorcollapse. See https://github.com/ExactTarget/fuelux/pull/1996 for more details.

It means that your code (probably in the CSS) is inappropriately setting the checkbox or radio visibility to hidden or collapse. You can fix it by:

(Preferred) Update to the most recent version of Lightning Theme or MC Theme.
(Allowed) If you are unable to upgrade to the latest Lightning or MC Themes, or if you are using some other custom CSS or theme that sets this, find the place where your code is doing this and don't do it instead.
- For Lightning Theme, that means editing [file] line [number] to be [thing]
- For MC Theme, that means editing [file] line [number] to be [thing]
If you have a valid reason for making the checkbox or radio hidden (ie, if it is hidden on pageload and you plan to show it later) try making the parent container hidden instead, or else setting display to none on pageload. Or (worst-case) add a data attribute data-ignore-visibility-check=true or pass an option on init $yourCheckbox.checkbox({ignoreVisibilityCheck: true}) to make checkbox ignore the error.

…adding unit tests

…ests 'more correctly'

…and ok/notOk instead of equals

… markup

…checkbox is hidden or collapsed.

…nd-radio-do-not-have-tabindex

…an one that show

…unctional programming pattern (actually uses passed in var)

…instead of just one of those twice

…of checkbox tests and weren't actually even running in some instances somehow

interactivellama · 2017-06-26T21:59:14Z

I really like this proactive check. You might consider some kind of production/dev env check, but I think you can only do that in CommonJS not ES5.

interactivellama · 2017-06-27T21:20:03Z

index.js

-	if (window.console && window.console.clear) {
-		window.console.clear();
-	}
+	// if (window.console && window.console.clear) {


Just remove it, if you don't want it.

…add-accessibility-check-for-checkbox

…b.com:cormacmccarthy/fuelux into CH1972---add-accessibility-check-for-checkbox

interactivellama · 2017-06-27T21:26:38Z

js/scheduler.js

 	// -- END UMD WRAPPER PREFACE --

 	// -- BEGIN MODULE CODE HERE --
+	if (!$.fn.combobox || !$.fn.datepicker || !$.fn.radio || !$.fn.selectlist || !$.fn.spinbox) {


I don't think any of these namespace checks should be in the bundled code, since the code is known to be there. You've moved it into the bundled library in multiple place.

I literally just moved it down below the UMD wrapper comment... Are you saying that somehow changed the way it gets bundled?

I'm saying that if you move it into the -- BEGIN MODULE CODE HERE -- part then it WILL get into the bundle, instead of just used when you require the one control.

Wait, so you're telling me that the bundle gets generated in part based on a comment in the code?

Yes, the bundle is created with concat. We couldn't do require or ES6 import, etc.

https://github.com/ExactTarget/fuelux/blob/master/grunt/config/concat.js#L32

interactivellama · 2017-06-27T21:46:05Z

test/checkbox-test.js

+		require('./checkbox-tests/return')(QUnit);
+		require('./checkbox-tests/get-value')(QUnit);
+		require('./checkbox-tests/events')(QUnit);
+		require('./checkbox-tests/visibility')(QUnit);


if we are skipping the visibility tests, then should we comment this line out so no one sees it.

Well, the point of making it .skip is that you alert the person that "there's a thing that maybe ought to be tested, but actually isn't". That way they can either add the test, or go test it themselves. If we comment it out, we might as well just remove it... Any thoughts @futuremint (Dave is the one who told me to add the skip tests)?

How about something like "skipped intentionally, future work" added to the test name.

Looking closer at this, checkbox visibility tests are fully defined. Not sure where you are seeing it is skipped? Tree is the only thing that has skipped tests (and those were added by a different PR).

interactivellama · 2017-06-27T21:49:48Z

index.html should be an example of how to use Fuel UX, not a testing location for console errors. Please move these to another page if in browser testing is the purpose.

Another option is to label the "bad checkboxes" as "THIS MARKUP IS WRONG" or something.

interactivellama · 2017-06-27T22:22:08Z

Please remove skipped tests or add something obvious to tell new folks to the project why they are skipped. Could be in test name?

…at gets bundled into all.js where it would be innapropriate

cmcculloh-kr · 2017-06-28T13:17:38Z

The skipped tests were added in a different PR, but I've documented them in this PR a little in response to your request for them to be removed. In #2000 I fleshed a few of them out more.

It's important for them to be there as an indicator that there is functionality that isn't actually being tested (that in a perfect world would be). That way, if the PR or change someone is working on has anything to do with those things (this PR doesn't...), they'll know that those things need tested more carefully manually by both the dev and the reviewer(s) and/or those tests need fleshed out.

…ssibility-check-for-checkbox

cmcculloh-kr · 2017-06-28T13:19:40Z

Oops! I always forget that index.html is anything other than just a place to do testing. Will fix, thanks.

Christopher McCulloh added 20 commits June 15, 2017 15:10

(GH1972) fixes all eslint errors in checkbox-test in preparation for …

8d48c0b

…adding unit tests

(GH1972) fixes html lint error

86f31db

(GH1972) adds checkbox html to qunit fixture and begins structuring t…

6cd5fee

…ests 'more correctly'

(GH1972) more cleanup of checkbox tests

859c37a

(GH1972) cleans up checkbox qunit tests to utilize fixture, helpers, …

30de167

…and ok/notOk instead of equals

(GH1972) cleans up remaining checkbox tests. Corrects tested checkbox…

82a8723

… markup

(GH1972) renames return functions properly

6517999

(GH1972) stubs out keyboard tests

8b23823

(GH1972) removes attempt to innapropriately test browser functionality

42f2f14

(GH1972) moves keypress factory method into more shareable location

2c3ae6d

(GH1972) puts back non-checkbox unit tests

14e7913

(GH1972) adds check for visibility hidden|collapse and errors out if …

11bbee5

…checkbox is hidden or collapsed.

Merge remote-tracking branch 'origin/master' into GH1972---checkbox-a…

2cdcd78

…nd-radio-do-not-have-tabindex

(GH1972) makes UMD wrapper consistent across Fuel UX

c8a6aa9

(GH1972) fixes eslint errors in checkbox

384d28f

(GH1972) adds line breaks to error messages in case there are more th…

dfe29b0

…an one that show

(GH1972) makes a11y checkbox error non-breaking

0b97844

(CH1972) references correct PR number from error message

13769f2

(CH1972) adds error to radio also

f81f9b9

(CH1972) cleans up eslint error. Moves setDisabledState closer to a f…

a13c5f8

…unctional programming pattern (actually uses passed in var)

cmcculloh-kr changed the title ~~Ch1972 add accessibility check for checkbox~~ Ch1972 add accessibility check for checkbox and radio Jun 21, 2017

cmcculloh-kr added Accessibility checkbox/radio labels Jun 21, 2017

cmcculloh-kr added this to the 3.16.1 milestone Jun 21, 2017

Christopher McCulloh added 6 commits June 21, 2017 10:51

(CH1972) allow options override to ignore visibility check

4ced2a4

(CH1972) checks actual input element visibility instead of label

67dcc06

(CH1972) adds dev tests for visibility checks

f58b649

(CH1972) adds unit tests for visibility error check

28a0256

(CH1972) adds unit test for ignoreVisibilityCheck option

e97f9c4

(CH1972) fixes all eslint errors in radio unittests

deaa53e

Christopher McCulloh added 2 commits June 21, 2017 16:26

(CH1972) makes checkbox test both checked and unchecked change event …

1fced53

…instead of just one of those twice

(CH1972) cleans up radio tests. They were badly copy-pastad versions …

906def4

…of checkbox tests and weren't actually even running in some instances somehow

cmcculloh-kr requested review from interactivellama and futuremint June 21, 2017 20:29

cmcculloh-kr assigned futuremint Jun 21, 2017

cmcculloh-kr mentioned this pull request Jun 24, 2017

Gh1994 keyboard navigation of open empty folder in tree should work as expected #2000

Merged

Merge branch 'master' into CH1972---add-accessibility-check-for-checkbox

4779116

interactivellama assigned interactivellama and unassigned futuremint Jun 27, 2017

interactivellama reviewed Jun 27, 2017

View reviewed changes

Christopher McCulloh added 3 commits June 27, 2017 17:25

(CH1972) removes commented out feature

60598f4

Merge branch 'master' of github.com:ExactTarget/fuelux into CH1972---…

82424b5

…add-accessibility-check-for-checkbox

Merge branch 'CH1972---add-accessibility-check-for-checkbox' of githu…

fe0c272

…b.com:cormacmccarthy/fuelux into CH1972---add-accessibility-check-for-checkbox

interactivellama reviewed Jun 27, 2017

View reviewed changes

Christopher McCulloh added 2 commits June 27, 2017 18:57

(CH1972) places single require error checking code outside of code th…

3a6c720

…at gets bundled into all.js where it would be innapropriate

(CH1972) attempts to clarify skipped tests

eb88b1d

Merge remote-tracking branch 'upstream/master' into CH1972---add-acce…

871c0e8

…ssibility-check-for-checkbox

(CH1972) removes incorrect markup variants

af3961c

interactivellama merged commit bb6c760 into ExactTarget:master Jun 28, 2017

cmcculloh-kr deleted the CH1972---add-accessibility-check-for-checkbox branch July 12, 2017 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ch1972 add accessibility check for checkbox and radio #1996

Ch1972 add accessibility check for checkbox and radio #1996

cmcculloh-kr commented Jun 21, 2017 •

edited

Loading

interactivellama commented Jun 26, 2017

interactivellama Jun 27, 2017

interactivellama Jun 27, 2017

cmcculloh-kr Jun 27, 2017

interactivellama Jun 27, 2017

cmcculloh-kr Jun 27, 2017

interactivellama Jun 27, 2017 •

edited

Loading

interactivellama Jun 27, 2017 •

edited

Loading

cmcculloh-kr Jun 27, 2017

interactivellama Jun 27, 2017

cmcculloh-kr Jun 27, 2017

interactivellama commented Jun 27, 2017 •

edited

Loading

interactivellama commented Jun 27, 2017

cmcculloh-kr commented Jun 28, 2017

cmcculloh-kr commented Jun 28, 2017

Ch1972 add accessibility check for checkbox and radio #1996

Ch1972 add accessibility check for checkbox and radio #1996

Conversation

cmcculloh-kr commented Jun 21, 2017 • edited Loading

interactivellama commented Jun 26, 2017

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

interactivellama Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

interactivellama Jun 27, 2017 • edited Loading

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

interactivellama commented Jun 27, 2017 • edited Loading

interactivellama commented Jun 27, 2017

cmcculloh-kr commented Jun 28, 2017

cmcculloh-kr commented Jun 28, 2017

cmcculloh-kr commented Jun 21, 2017 •

edited

Loading

interactivellama Jun 27, 2017 •

edited

Loading

interactivellama Jun 27, 2017 •

edited

Loading

interactivellama commented Jun 27, 2017 •

edited

Loading