Fix getSelectorFromElement when # is a selector

[Johann-S]

Check if data-target contain # to avoid JQuery error

Close : #21328

[pvdlg]

See this PR #21654 that handle the problem differently and cover more (all) possible cases of selector.

[Johann-S]

Yes I forget one case but I'll update my PR thank you @vanduynslagerp

[pvdlg]

What I meant is that #21654 solve #21328 and other issues, plus it brings other benefits described in the PR. If #21654 is merged this one (#21615) becomes irrelevant as the modified code would been removed.

[Johann-S]

I didn't change how the core of Bootstrap works but I added what you did in your PR by removing the regex because as you said it fix #18641 too 👍

[pvdlg]

Really sorry to insist...but here is a few remarks.

Only the dropdown and the alert component expect to data-target="#" to be valid. This is because the modules handle this cases by selecting the parent of the trigger in this case. See dropdown.js#L212 and alert.js#L97
So as you modification is on Util.js you impact all the other modules using getSelectorFromElement .

Also consider other possible wrong value such data-target="test". The previous code would return null because "test" do not match the regex. With you modification it would set selector to "test" and then execute $('test') not throw an exception and then return selector with the value "test".

Finally you call Jquery on the value selector and then return selector to call JQuery again in each module again on the same selector. That's not really efficient as you have JQuery parsing the dom twice.

For all these reasons and several other I decided to rewrite the function getSelectorFromElement to return the JQuery with the list of matching elements and use that result in each module.
That also allow me to handle different cases differently in each module (for example in dropdown when neither the value of data-target nor href is a selector that returns elements I can determine the target is the parent. In in collapse, in the same situation I just skip the action as it's impossible to determine the target).

The point I was trying to make is that the best way to handle this is to get rid of the regex and use the try catch. This way we can convert all cases and we don't have to add more if elses or a more complicated regex every time we have an issue reported of type "data-target="whatever" doesn't work and should do this or that".
And doing also require the other modifications I made in #21654.

The point of my PR was in the first place to fix #18641 and improve the mechanism by making more robust. I realize later on after posting the PR, that it would also fixe #21328 so I mentioned it here.

[pvdlg]

I also added a unit test in #21654 for the issue described in #21328

[Johann-S]

Yes you're right on this :

Also consider other possible wrong value such data-target="test". The previous code would return null because "test" do not match the regex. With you modification it would set selector to "test" and then execute $('test') not throw an exception and then return selector with the value "test".

So I'll update a bit my PR thank you 👍

In your PR the same problem is present because if the selector is set to test you return
$('test') and after in each modules you check :

const target = targets ? targets[0] : null

But if targets is equal to $('test') it will return targets[0] which is undefined

[pvdlg]

So I'll update a bit my PR thank you 👍

My point was that after I made my PR (without seeing yours before, sorry...) I realized that it also fixes the same problem and covers more ground. There is no point in having both #21654 and this one. Anyway I don't want to insist more.

But if targets is equal to $('test') it will return targets[0] which is undefined

Yes that's on purpose. If there is no matching element that can be used as target then the variable target will be undefined or null and handled according by each module.
If you take the dropdown module as an example it's handled here: dropdown R206. If target is null, undefined or an empty JQuery then we use element.parentNode. This what fix the case explained in #21328.
In collapse for example we need to have a defined target and we cannot figure it out by ourselves (like using element.parentNode in dropdown as the collapse markup is different). So if target is null or undefined here then we skip the action as we don't have anything to collapse

[Johann-S]

I add on my last commit, a check if the selector exist in the DOM or not and with that we don't have to edit all plugins because it works as you described.

Your dropdown unit test is very usefull 👍

[pvdlg]

The more modifications you make, the more your PR is looking like mine.
I spent a day working on #21654 and going through all this cases, details and test. So once you will go through all of them, I imagine the code will be similar to mine.
So I'm not really what we are doing here....
I just wanted to make the point, in my first comment, that I believe #21654 solves #21328 and other reported and non-reported issues in a more thorough way.

[Johann-S]

I finished my PR I wont add anything. My PR only avoid the issue when ``#`` is used in selector that's all⁣​

…

On 11 Jan 2017, 18:33, at 18:33, Pierre-Denis Vanduynslager ***@***.***> wrote: The more modifications you make, the more your PR is looking like mine. I spent a day working on #21654 and going through all this cases, details and test. So once you will go through all of them, I imagine the code will be similar to mine. So I'm not really what we are doing here.... I just wanted to make the point, in my first comment, that I believe #21654 solves #21328 and other reported and non-reported issues in a more thorough way. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #21615 (comment)

[mdo]

Lemme know if we still need to do #22128 as well.

[Johann-S]

#22128 is useless due to the merge of this PR 👍 thank you @mdo