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

Collapse multi-target fails with "#id1, #id2" data-target selector #25273

Closed
bpierson opened this issue Jan 11, 2018 · 11 comments
Closed

Collapse multi-target fails with "#id1, #id2" data-target selector #25273

bpierson opened this issue Jan 11, 2018 · 11 comments
Labels

Comments

@bpierson
Copy link

Fiddle

With a comma-separated CSS selector, the targeted collapsible elements are not toggled, for example:
data-target="#coll1, #coll2"

No error is raised.

@MartijnCuppens
Copy link
Member

An aria-expanded attribute is added to the toggle button to indicate if the collapsible element is collapsed or not. If you target multiple collapsible elements which have different states (collapsed and shown), it's impossible to determine the aria-expanded state of the toggle button.

I don't know if there are (accessible) solutions for this situation.

@patrickhlauke
Copy link
Member

this is a valid bug related to http://getbootstrap.com/docs/4.0/components/collapse/#multiple-targets

@MartijnCuppens as for the aria-expanded, you're right that if the multiple targets aren't all either expanded or collapsed, the aria-expanded will become meaningless. interesting edge case, as i'm not sure it's intended. might be worth filing a separate issue for it.

@bpierson
Copy link
Author

@MartijnCuppens , @patrickhlauke that's true about the aria-expanded, but wouldn't it be true in any case using the example from the docs that takes a class selector (and works correctly)? Thanks.

@patrickhlauke
Copy link
Member

@bpierson yes, which is why i'm saying that particular thing should be a separate issue. the problem you reported is valid as per your description (the valid jquery selector you provide, with the two ids, isn't working as it should)

@bpierson
Copy link
Author

Won't SR's honor the display style of the collapsible element in question, irrespective of which element triggered it? Maybe aria-expanded on the trigger is overkill here. Don't want to get too far into the weeds and away from the OP, but using aria-expanded seems to be a non-starter if the "any selector" feature stays in place since the value can only be true or false.

@XhmikosR
Copy link
Member

Ping @Johann-S

@Johann-S
Copy link
Member

Possibly related to this line https://github.com/twbs/bootstrap/blob/v4-dev/js/src/util.js#L109

@bpierson
Copy link
Author

@Johann-S I just confirmed that to be the case by just changing the value to " #coll1, #coll2", which worked as expected (note the space at index 0).

I understand the motivation in trying to sanitize the id, but it seems like in the case of a selector it's a losing battle. Presumably the coder supplied both the id value and the data-target value, so it should really be up to them to make sure id values (and their corresponding href or attribute values) are pre-sanitized.

I might humbly suggest that to simplify your code base, js libraries should just take what the coder supplies for attribute values - even ids.

@Johann-S
Copy link
Member

Yep it seems they are a lot of use cases which are difficult to handle in our ends 😟

Maybe we should add a warning in our documentation and remove that code

@bpierson
Copy link
Author

@Johann-S most systems that generate or template html have methods on the server side to sanitize id and attribute values before it ever gets to the browser. While it would technically be a breaking change, I doubt that it would have much impact. I've been programming about 30 years and in my experience, many issues are created by changing the value supplied by the user (side-effects). I've even run into problems just trimming off whitespace!

@miken32
Copy link

miken32 commented Jan 18, 2018

I've run into this problem also, not with multiple targets but just using a more complicated target spec that started with #. I agree with the idea that the value should be passed to jQuery as-is, with no preprocessing.

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

No branches or pull requests

6 participants