Skip to content

Commit

Permalink
[added] Add contentLabel prop to put aria-label on modal content
Browse files Browse the repository at this point in the history
This makes the modal better follow aria guidelines for dialogs.
It also gives screenreader users some identity for the modal after
it opens.

closes #236
  • Loading branch information
claydiffrient committed Oct 15, 2016
1 parent 11c1fd6 commit 3d8e5a0
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 4 deletions.
9 changes: 7 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ npm install --save react-modal
onRequestClose={requestCloseFn}
closeTimeoutMS={n}
style={customStyle}
contentLabel="Modal"
>
<h1>Modal Content</h1>
<p>Etc.</p>
Expand Down Expand Up @@ -136,7 +137,9 @@ var App = React.createClass({
isOpen={this.state.modalIsOpen}
onAfterOpen={this.afterOpenModal}
onRequestClose={this.closeModal}
style={customStyles} >
style={customStyles}
contentLabel="Example Modal"
>

<h2 ref="subtitle">Hello</h2>
<button onClick={this.closeModal}>close</button>
Expand Down Expand Up @@ -171,7 +174,9 @@ pass the 'shouldCloseOnOverlayClick' prop with 'false' value.
onRequestClose={requestCloseFn}
closeTimeoutMS={n}
shouldCloseOnOverlayClick={false}
style={customStyle}>
style={customStyle}
contentLabel="No Overlay Click Modal"
>

<h1>Force Modal</h1>
<p>Modal cannot be closed when clicking the overlay area</p>
Expand Down
3 changes: 2 additions & 1 deletion lib/components/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ var Modal = React.createClass({
closeTimeoutMS: React.PropTypes.number,
ariaHideApp: React.PropTypes.bool,
shouldCloseOnOverlayClick: React.PropTypes.bool,
role: React.PropTypes.string
role: React.PropTypes.string,
contentLabel: React.PropTypes.string.isRequired

This comment has been minimized.

Copy link
@camwest

camwest Dec 9, 2016

This is a breaking change? Why is this property now required?

This comment has been minimized.

Copy link
@claydiffrient

claydiffrient Dec 10, 2016

Author Contributor

It's not really a breaking change. I mean, you can always not provide the property. If you don't provide it you will get a warning about missing a required propType. It adds something to the API for the component, thus was released as a minor version. Because the addition of this property shouldn't have caused any user facing errors for the component I didn't feel it merited a major version bump. I'm sorry if it's caused you any issues.

The reason that this is now here, is that it dramatically improves the accessibility story for users with screenreaders. This modal's primary objective is to be accessible. This furthers that objective.

This comment has been minimized.

Copy link
@linmic

linmic Dec 11, 2016

Contributor

Even if this is to improve the a11y, contentLabel can still be optional yes? Or is there any reason should you make this a required prop?

This comment has been minimized.

Copy link
@claydiffrient

claydiffrient Dec 11, 2016

Author Contributor

@linmic In theory yes it could be optional. I'm not comfortable with that though because you should be putting a label there for any modals.

While we don't follow the ARIA guidelines completely sometimes in an effort to do what works best in various browser/screenreader combinations (i.e., we don't apply role="dialog" by default) , this is a "best practice" regardless for this particular item. (see https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal)

This comment has been minimized.

Copy link
@linmic

linmic Dec 12, 2016

Contributor

@claydiffrient Make sense. I'd recommend to add the required props info to README so that people can have it handled before they have to see the unexpected warnings. PR made: #274

This comment has been minimized.

Copy link
@camwest

camwest Dec 13, 2016

Thank you for the explanation.

This comment has been minimized.

Copy link
@drosen0

drosen0 Dec 20, 2016

With semantic versioning, a breaking change must correspond to a major version bump.

This comment has been minimized.

Copy link
@avanderhoorn

avanderhoorn Dec 20, 2016

Ya I just ran into this problem... the typescript definitions hadn't been updated to include this.

},

getDefaultProps: function () {
Expand Down
3 changes: 2 additions & 1 deletion lib/components/ModalPortal.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ var ModalPortal = module.exports = React.createClass({
onKeyDown: this.handleKeyDown,
onMouseDown: this.handleContentMouseDown,
onMouseUp: this.handleContentMouseUp,
role: this.props.role
role: this.props.role,
"aria-label": this.props.contentLabel
},
this.props.children
)
Expand Down
7 changes: 7 additions & 0 deletions specs/Modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ describe('Modal', function () {
unmountModal();
});

it('renders the modal with a aria-label based on the contentLabel prop', function () {
var child = 'I am a child of Modal, and he has sent me here...';
var component = renderModal({isOpen: true, contentLabel: 'Special Modal'}, child);
equal(component.portal.refs.content.getAttribute('aria-label'), 'Special Modal');
unmountModal();
});

it('has default props', function() {
var node = document.createElement('div');
Modal.setAppElement(document.createElement('div'));
Expand Down

0 comments on commit 3d8e5a0

Please sign in to comment.