Skip to content

Commit 7bfaa91

Browse files
fourcolorsfourcolors
committed
fix canvas modals accessibility issues
fixes CNVS-25910 When ReactModal was upgraded, it introduced a new API and changes some behavior with setAppElement. Now, react modal accepts a property, appElement and needs to be set. By default, it attaches to body, however in canvas this is really bad because it will aria-hide the entire application whenever a modal opens. This commit makes the default attachment #application with an option to pass in a different element if you'd like. Test Plan Make sure opening modals do NOT attach aria-hidden to the <body> element. There are many places where canvas modal is used. Here are a few know places. If they work, then its safe to assume its working everywhere else. - File Rename Modal (on the files page) - File Move Dialog (on the files page) - File Zip/Unzip Dialog (on the files page) Test thoses spots, ensure aria-hidden is not on the body, instead it should be attached on the div with an id of #application. Change-Id: I606e46524b385da85f8b9cce4f41fd7449c9dec2 Reviewed-on: https://gerrit.instructure.com/68964 Tested-by: Jenkins Reviewed-by: Andrew Butterfield <abutterfield@instructure.com> Product-Review: Sterling Cobb <sterling@instructure.com> QA-Review: Jahnavi Yetukuri <jyetukuri@instructure.com>
1 parent 93d2e25 commit 7bfaa91

2 files changed

Lines changed: 70 additions & 3 deletions

File tree

‎app/jsx/shared/modal.jsx‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ define([
88
'./modal-buttons',
99
], function (React, $, _, preventDefault, ReactModal, ModalContent, ModalButtons) {
1010

11-
ReactModal.setAppElement(document.body)
12-
1311
var Modal = React.createClass({
1412

1513
getInitialState() {
@@ -32,6 +30,7 @@ define([
3230
closeModal() {
3331
this.setState({modalIsOpen: false}, function(){
3432
this.props.onRequestClose();
33+
$(this.getAppElement()).removeAttr('aria-hidden');
3534
});
3635
},
3736
closeWithX() {
@@ -43,6 +42,10 @@ define([
4342
var promise = this.props.onSubmit();
4443
$(this.refs.modal.getDOMNode()).disableWhileLoading(promise);
4544
},
45+
getAppElement () {
46+
// Need to wait for the dom to load before we can get the default #application dom element
47+
return this.props.appElement || document.getElementById('application');
48+
},
4649
processMultipleChildren(props){
4750
var content = null;
4851
var buttons = null;
@@ -84,7 +87,9 @@ define([
8487
isOpen={this.state.modalIsOpen}
8588
onRequestClose={this.closeModal}
8689
className={this.props.className}
87-
overlayClassName={this.props.overlayClassName}>
90+
overlayClassName={this.props.overlayClassName}
91+
appElement={this.getAppElement()}
92+
>
8893
<div ref="modal" className="ReactModal__Layout">
8994

9095
<div className="ReactModal__Header">

‎spec/coffeescripts/jsx/shared/modalSpec.coffee‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@ define [
99
TestUtils = React.addons.TestUtils
1010

1111
module 'Modal',
12+
setup: ->
13+
$('body').append("<div id=application />")
1214
teardown: ->
1315
React.unmountComponentAtNode(@component.getDOMNode().parentNode)
16+
$('#application').remove()
1417

1518
test 'has a default class of, "ReactModal__Content--canvas"', ->
1619
ModalElement = React.createElement(Modal, {isOpen: true, title: "Hello"},
@@ -110,3 +113,62 @@ define [
110113
@component.closeModal()
111114
equal @component.state.modalIsOpen, false, "closes modal"
112115
ok calledOnRequestClose, "calls on request close"
116+
117+
test "doesn't default to attaching to body", ->
118+
@component = TestUtils.renderIntoDocument(React.createElement(Modal,
119+
isOpen: true,
120+
className: 'custom_class_name'
121+
title: "Hello",
122+
"Inner content"
123+
))
124+
125+
equal $('body').attr('aria-hidden'), undefined, "doesn't attach aria-hidden to body"
126+
127+
test "defaults to attaching to #application", ->
128+
@component = TestUtils.renderIntoDocument(React.createElement(Modal,
129+
isOpen: true,
130+
className: 'custom_class_name'
131+
title: "Hello",
132+
"Inner content"
133+
))
134+
135+
equal $('#application').attr('aria-hidden'), 'true', "attaches to application"
136+
137+
test 'removes aria-hidden from #application when closed', ->
138+
@component = TestUtils.renderIntoDocument(React.createElement(Modal,
139+
onRequestClose: -> console.log('closed'),
140+
isOpen: true,
141+
className: 'custom_class_name'
142+
title: "Hello",
143+
"Inner content"
144+
))
145+
146+
@component.closeModal()
147+
148+
equal $('#application').attr('aria-hidden'), undefined, "removes aria-hidden attribute"
149+
150+
151+
test "appElement sets react modals app element", ->
152+
@component = TestUtils.renderIntoDocument(React.createElement(Modal,
153+
appElement: $('#fixtures')[0],
154+
isOpen: true,
155+
className: 'custom_class_name'
156+
title: "Hello",
157+
"Inner content"
158+
))
159+
160+
equal $('#fixtures').attr('aria-hidden'), 'true', "attaches to the specified dom element"
161+
162+
test "removes aria-hidden from custom setElement property when closed", ->
163+
@component = TestUtils.renderIntoDocument(React.createElement(Modal,
164+
onRequestClose: -> console.log('closed'),
165+
appElement: $('#fixtures')[0],
166+
isOpen: true,
167+
className: 'custom_class_name'
168+
title: "Hello",
169+
"Inner content"
170+
))
171+
172+
@component.closeModal()
173+
174+
equal $('#fixtures').attr('aria-hidden'), undefined, "no aria-hidden attribute on element"

0 commit comments

Comments
 (0)