Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix aria-owns attribute - Issue #1521 #1556

Merged
merged 1 commit into from
May 17, 2017
Merged

Fix aria-owns attribute - Issue #1521 #1556

merged 1 commit into from
May 17, 2017

Conversation

mhubenthal
Copy link
Contributor

@mhubenthal mhubenthal commented Feb 15, 2017 •
edited
Loading

(Addresses issue #1521)

It appears that Firefox is getting stuck in some kind of infinite loop due to incorrect use of aria-owns attribute. Here's the W3C spec on aria-owns:

https://www.w3.org/TR/wai-aria/states_and_properties#aria-owns

In Select.js, line 879, when isOpen is false, the component assigns this._instancePrefix + '-value' to aria-owns. That value corresponds to a DOM node which is a parent, and thus has a hierarchical relationship, breaking the W3C spec.

If you visit the demo in Chrome (https://jedwatson.github.io/react-select/) and inspect the HTML, you'll see that in the situation when the component would be breaking Firefox, Chrome has intelligently removed the offending aria-owns value.

(This PR uses the same "classNames" approach to generating aria-owns already used on line 841.)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 38e3e1b on mhubenthal:master into ** on JedWatson:master**.

@mhubenthal mhubenthal mentioned this pull request Feb 15, 2017
Closed
omgaz
omgaz approved these changes Feb 15, 2017
View reviewed changes
Copy link
Contributor

@omgaz omgaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, should there be a unit test for this?

@mhubenthal
Copy link
Contributor Author

mhubenthal commented Feb 16, 2017 •
edited
Loading

@omgaz I had the same question. In this case the string value assigned to aria-owns was invalid because it referred to a parent node. I guess that's an open question to @JedWatson if there should be tests for valid HTML structure? And on a side note, Firefox probably shouldn't blow up because of invalid HTML...

@mhubenthal
Copy link
Contributor Author

mhubenthal commented Feb 16, 2017 •
edited
Loading

@omgaz after poking around a little more this morning I have a bit more to add to the issue. It appears that the invalid use of aria-owns breaks Firefox when the parent node is a <span> but not when it is a <div>. I still think this PR is valid for correcting use of aria-owns, but changing the parent node to a <div> would also fix the issue (but would be more of a workaround than a true fix).

cc: @JedWatson

@mhubenthal
Copy link
Contributor Author

@omgaz @JedWatson any more thoughts on this PR?

@omgaz
Copy link
Contributor

omgaz commented Feb 22, 2017

Seeing as how aria-owns applies to "All elements of the base markup", changing from a span to a div, like you mention, would feel like a workaround and not a fix.

I think this PR looks good.

@dalehurwitz
Copy link

Hi @JedWatson any headway on this?

@abhishekpillai
Copy link

Hi @JedWatson, let me know if there's anything I can do to help get this PR merged! Looking forward to this fix. Thanks! 👍

@ekwonye-richard
Copy link

I had the same issue, my fix was to use readOnly rather than searchable... Use this props

inputProps={{readOnly: bolean}}

I was lucky to quickly effect it across my app because i wrapped react-select inside a component i wrote.

@lxerxa
Copy link

lxerxa commented Mar 30, 2017

looking fotward to this fix.

@rnadrag rnadrag mentioned this pull request Apr 12, 2017
Open
@broadhu2 broadhu2 mentioned this pull request May 8, 2017
Closed
@broadhu2
Copy link

broadhu2 commented May 8, 2017

When will this pull request be approved?

@Kadrian
Copy link

Kadrian commented May 10, 2017

@JedWatson Could you take another look / merge it?

@rnadrag
Copy link

rnadrag commented May 17, 2017

👍 on this to merge/release as soon as possible

@agirton
Copy link
Collaborator

agirton commented May 17, 2017

Hi @mhubenthal this looks good to me. I agree this is the right approach.

@agirton agirton merged commit 196c7bb into JedWatson:master May 17, 2017
@rnadrag
Copy link

rnadrag commented May 17, 2017

any timelines on getting this released?

@agirton
Copy link
Collaborator

agirton commented May 17, 2017

@rnadrag I would like to get the new Async focus bug fixed and then can get a new release out soon.

@rnadrag
Copy link

rnadrag commented May 17, 2017

sweet!

@benelog
Copy link

benelog commented May 23, 2017

@agirton
Thanks for merging it.
The next release will be very helpful to our project, too.

billyvg pushed a commit to billyvg/react-select that referenced this pull request May 25, 2017
@mhubenthal
Fix aria owns value (JedWatson#1556)
ed9f879
echenley pushed a commit to helpdotcom/react-select that referenced this pull request Jun 13, 2017
Squashed commit of the following:
86eb32c 
commit 07dc061
Author: cbergmiller <[email protected]>
Date:   Tue Jun 6 07:04:20 2017 +0200

    [ADD] Clarified the onInputChange prop signature in the docs (JedWatson#1773)

    * [ADD] Clarified the onInputChange prop signature in the docs

    * [ADD] requested changes

commit 40db517
Author: cbergmiller <[email protected]>
Date:   Sat Jun 3 15:53:45 2017 +0200

    [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps (JedWatson#1765)

    * [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps

    * [ADD] test for option prop sync by componentWillReceiveProps

commit 1d15068
Author: Jed Watson <[email protected]>
Date:   Thu May 25 00:25:19 2017 +1000

    v1.0.0-rc.5

commit e06ea57
Author: Jed Watson <[email protected]>
Date:   Thu May 25 00:25:09 2017 +1000

    Updating build

commit 7013005
Merge: 9943711 8239201
Author: Jed Watson <[email protected]>
Date:   Thu May 25 00:19:49 2017 +1000

    Merge pull request JedWatson#1738 from agirton/chore/HISTORY-update

    Update change log for rc5 release.

commit 8239201
Author: Adam Girton <[email protected]>
Date:   Sat May 20 10:37:18 2017 -0700

    Update changelog

commit 9943711
Author: Benjamin Piouffle <[email protected]>
Date:   Fri May 19 07:21:40 2017 +1100

    Fix selected option focus when valueKey is not "value" (JedWatson#1733)

    * Fix selected option focus when valueKey is not "value"

    * Revert "Fix selected option focus when valueKey is not "value""

    This reverts commit 45f5665.

    * Fix on src/ instead of lib/

commit 196c7bb
Author: Max Hubenthal <[email protected]>
Date:   Wed May 17 12:47:29 2017 -0400

    Fix aria owns value (JedWatson#1556)

commit fd19b91
Author: Simon Gaestel <[email protected]>
Date:   Wed May 17 18:22:34 2017 +0200

    Fix JedWatson#1394 (JedWatson#1395)

    * Fix JedWatson#1394

    * Updates following review

    * Updates following review by @JedWatson

commit 549d20a
Author: Jed Watson <[email protected]>
Date:   Mon May 15 00:00:05 2017 +1000

    v1.0.0-rc.4

commit 5269a85
Merge: af8f58d 75d6372
Author: Jed Watson <[email protected]>
Date:   Sun May 14 23:31:30 2017 +1000

    Merge pull request JedWatson#1652 from dan-diaz/isClearable

    Include a className of 'is-clearable' if clearable is true

commit af8f58d
Merge: 8ff4596 9150107
Author: Jed Watson <[email protected]>
Date:   Sun May 14 23:27:19 2017 +1000

    Merge pull request JedWatson#1659 from lagun4ik/master

    Added variables for shadow styles

commit 8ff4596
Merge: a0e5855 03b3da2
Author: Jed Watson <[email protected]>
Date:   Sun May 14 23:25:48 2017 +1000

    Merge pull request JedWatson#1658 from agirton/migrate-to-external-pkgs

    Migrate to prop-types and create-react-class packages

commit 03b3da2
Author: Adam Girton <[email protected]>
Date:   Sun Apr 30 20:41:54 2017 -0700

    Bumping create-react-class and prop-types

commit 387f241
Author: Adam Girton <[email protected]>
Date:   Sun Apr 30 20:35:55 2017 -0700

    Update react-input-autosize dep

commit 9150107
Author: lagun4ik <[email protected]>
Date:   Mon Apr 10 21:55:48 2017 +0300

    added variables for shadow styles

commit d7d2a79
Author: Adam Girton <[email protected]>
Date:   Mon Apr 10 09:55:46 2017 -0700

    Move over examples to use new pkgs

commit feabc4d
Author: Adam Girton <[email protected]>
Date:   Mon Apr 10 09:55:17 2017 -0700

    Migrate rest of src overto createClass pkg

commit ace3e08
Author: Adam Girton <[email protected]>
Date:   Mon Apr 10 09:44:54 2017 -0700

    Migrate src over to creatClass package

commit 16ff8e9
Author: Adam Girton <[email protected]>
Date:   Mon Apr 10 09:38:37 2017 -0700

    Use new prop-types package

commit 75d6372
Author: DDiaz <[email protected]>
Date:   Thu Apr 6 10:39:10 2017 -0400

    spaces to tabs

commit c81b9c9
Author: DDiaz <[email protected]>
Date:   Thu Apr 6 10:37:15 2017 -0400

    Include a className of 'is-clearable' if clearable is true
ryanabooth added a commit to entelo/react-select that referenced this pull request Jun 29, 2017
@ryanabooth
Merge commit '26169305a721ec3099a912cea2f6ed38e6dc9c4c' into ryan/upd…
bcc482c 
…ate_and_add_creatable_options

* commit '26169305a721ec3099a912cea2f6ed38e6dc9c4c': (36 commits)
  Fix Usage Docs Example (JedWatson#1799)
  Hide create option after closing menu (JedWatson#1306)
  Update README.md to include 'valueComponent' prop (JedWatson#1803)
  Adding merged changes to changelog
  Fix backspace handling for non-multi select controls. Fixes JedWatson#638 (JedWatson#773)
  Update Select.js
  [ADD] Clarified the onInputChange prop signature in the docs (JedWatson#1773)
  [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps (JedWatson#1765)
  v1.0.0-rc.5
  Updating build
  Update changelog
  Fix selected option focus when valueKey is not "value" (JedWatson#1733)
  Fix aria owns value (JedWatson#1556)
  Fix JedWatson#1394 (JedWatson#1395)
  v1.0.0-rc.4
  Bumping create-react-class and prop-types
  Update react-input-autosize dep
  added variables for shadow styles
  Move over examples to use new pkgs
  Migrate rest of src overto createClass pkg
  ...

# Conflicts:
#	dist/react-select.js
#	dist/react-select.min.js
#	examples/dist/bundle.js
#	examples/dist/standalone.js
#	lib/Async.js
#	lib/Value.js
@nnmn nnmn mentioned this pull request Jan 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omgaz omgaz omgaz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.