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

Button Element not updated if target ID contains colon #22060

Closed
svenhaag opened this issue Feb 24, 2017 · 10 comments
Closed

Button Element not updated if target ID contains colon #22060

svenhaag opened this issue Feb 24, 2017 · 10 comments
Assignees
Labels

Comments

@svenhaag
Copy link

svenhaag commented Feb 24, 2017

Bootstrap Version 4.0.0-alpha.6 and 3.3.7

Target ID = "collapse:Example"
data-target = "data-target="#collapse\:Example""
aria-controls="collapse:Example"

Only the Target attributes get updated, not the button (toggle element) attributes.
IDs with a colon exist especially in the JSF world.

Bug JS Fiddle

Related Bug

@bkdotcom
Copy link
Contributor

bkdotcom commented Feb 24, 2017

This works

<button role="button" type="button"
        data-toggle="collapse"
        data-target="#collapse\:\:Example"
        aria-expanded="true"
        aria-controls="collapse:Example">
  Button with data-target
</button>
<div class="collapse in" id="collapse::Example">
  bla bla bla
</div>

apparently one needs to escape the colons in the data attribute... but then
$("button").data("target") = "#collapse::Example"
So.. seems to be a bug somewhere that the colons need escaped

edit jQuery issue??
$("#collapse::Example") -> Syntax error, unrecognized expression: #collapse::Example
$("#collapse\:\:Example") -> Syntax error, unrecognized expression: #collapse::Example
$("#collapse\\:\\:Example") -> success

edit #2:
see https://learn.jquery.com/using-jquery-core/faq/how-do-i-select-an-element-by-an-id-that-has-characters-used-in-css-notation/

so... apparently jquery needs to "#collapse\:\:Example" and it also appears that it'd be impossible for bootstrap to reliably to the escaping... so looks like data-target="#collapse\:\:Example" is required

impossible for bootstrap to do it because it would have no way of knowing if "#some.id" is for <div id="#some.id"> or for <div id="some" class="id">

@svenhaag
Copy link
Author

svenhaag commented Feb 24, 2017

Hi, in the JSFiddle the data-target ID is already escaped with a single backslash, i.e data-target="#collapse\:Example". This is an already valid jQuery selector. Though why doubling it to "::" ? This does not work for me in the JSFiddle example. Maybe you can provide a running JSFiddle?
I think bootstrap maybe has to escape backslashes in IDs.
Thanks.

@bkdotcom
Copy link
Contributor

double colon fiddle

@svenhaag
Copy link
Author

Hi bkdotcom, adding a second double-colon is not a solution. JSF will render the ID attribute like "a:b:c".

@bkdotcom
Copy link
Contributor

"second double-colon"

?

JSF will render the ID attribute like "a:​b:c".

?

I inspected my fiddle's html and it's

<div class="collapse in" id="collapse::Example" aria-expanded="true">
  bla bla bla
</div>

Button Element not updated if target ID contains double-colon

Fiddle has working double-colon

Am I missing something?

@svenhaag svenhaag changed the title Button Element not updated if target ID contains double-colon Button Element not updated if target ID contains colon Feb 28, 2017
@svenhaag
Copy link
Author

Hi bkdotcom, sorry for the messup. I did not mean two subsequent colons but instead a single one. I fixed the heading and description of this issue. However the first JSFiddle still shows the bug. The toggling elements aria attributes are not updated if the ID contains a colon. Cheers.

@peterdesmet
Copy link

Maybe related: I noticed that Scrollspy doesn't work either with anchors/ids that include a colon (e.g. #foo:bar). This was already brought up in this discussion and decided to postpone to v4. Anyone has an idea if this still on the roadmap? /cc @hnrch02

@Johann-S
Copy link
Member

Johann-S commented Nov 7, 2017

I made a Codepen with our latest release (beta 2) see : https://codepen.io/Johann-S/pen/Ebgmgg
And aria-expanded is correctly updated

The only thing we can fix here it's to avoid the thing folks must escape manually their ID

@Johann-S
Copy link
Member

Johann-S commented Nov 7, 2017

#24700 is merged so now Bootstrap will escape your ID

@Johann-S Johann-S closed this as completed Nov 7, 2017
@meeque
Copy link
Contributor

meeque commented Jan 5, 2018

Hm, maybe too late but, here's how I see it:

  • Assuming an element with id foo:bar the correct CSS selector is in fact #foo\:bar
    (When writing this selector in JS, you also need to escape the backslash, but that's not relevant here.)
  • Per documentation bootstrap's data-target holds a CSS selector
  • In contrast, the aria-controls attribute does not hold a CSS selector but a verbatim id

And here's my conclusions:

  • It is not bootstraps job to fix broken CSS selectors
  • If application authors rely on bogus ids, the should learn to write the corresponding selectors
  • There is actually a reason why CSS selectors treat an unescaped colon specially: colons are used to express pseudo classes or pseudo elements
  • There may be valid reasons for using colons inside data-target as intended by CSS, e.g. #myfield:enabled
  • If bootstrap introduces magic fixes, it will most likely break other stuff.
  • In the very least this should be clearly documented

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors

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