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.

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

Fix power-select with closeOnSelect=false nested within basic-dropdown #1512

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamescdavis
Copy link

@jamescdavis jamescdavis commented Mar 2, 2022

Overriding aria-controls on the dropdown trigger causes issues when a power-select is nested within a basic-dropdown and @closeOnSelect={{false}} is set on the power-select. Specifically, the parent dropdown is erroneously closed when an option is selected. This change was introduced in bb1b8e9 as part of #1481.

cibernox/ember-basic-dropdown#633 is the ember-basic-dropdown PR that made it not ok to override aria-controls since it uses this to build a relationship between the trigger and content.

Repro: https://github.com/jamescdavis/power-select-nested-dropdown-repro/blob/main/app/templates/application.hbs
(this repro uses PowerSelectMultiple but the behavior is the same for PowerSelect)

This PR adds a failing test for the issue along with a couple of smoke tests for nested power-select. It also includes a commit that simply removes the aria-controls override, which makes the failing tests pass and resolves the bug, but I'm sure is not the desired resolution (and obviously makes a11y tests fail). I am as of yet uncertain how to fix this properly.

@jamescdavis jamescdavis force-pushed the fix_nested_in_dropdown_with_closeOnSelect_false branch from f60a821 to a323c19 Compare March 2, 2022 15:54
@jamescdavis
Copy link
Author

@calvin-fb any ideas on a proper fix?

@jamescdavis
Copy link
Author

@matthew-robertson tagging you as well

@calvin-fb
Copy link

@jamescdavis Interesting. I just played around with it a bit and I found that if I pass @renderInPlace={{true}} for the power-select, it also fixes the issue. If you are able to do that with your app, that would be a quick workaround until this gets fixed properly. I'm going to do some digging to find out what is actually going on and if there's a cleaner fix.

@calvin-fb
Copy link

So the issue is with this block in ember-basic-dropdown:
https://github.com/cibernox/ember-basic-dropdown/blob/master/addon/components/basic-dropdown-content.ts#L390-L392
It expects aria-controls to be matched up with ember-basic-dropdown-content. With the accessibility fixes that I made, that's no longer the case. aria-controls is now connected to ember-power-select-options instead of ember-basic-dropdown-content.
Here's another relevant line:
https://github.com/cibernox/ember-basic-dropdown/blob/master/addon/components/basic-dropdown-content.ts#L352

In terms of fixing it... one hacky fix would be adding ember-basic-dropdown-content at the same level as ember-power-select-options. This is pretty gross though because the ember-basic-dropdown-content class would also be on the parent and if there any styling associated with it, it would look pretty gross.
The other option would be making a change in the ember-basic-dropdown repo to make it aware of the differences in ember-power-select (not ideal). The closestContent function would have to be changed to look for either ember-basic-dropdown-content or ember-power-select-options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants