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

Fixes removing the namespace from an event name #2115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eydamos
Copy link

@Eydamos Eydamos commented Sep 1, 2022

Bootstrap uses events which need to include a namespace. E.g. the event name to bind to showing a modal is show.bs.modal. Just setting the event on show will not work.

Example:

$(document).on('show', '.modal', function() { console.log('this will never get fired')});
$(document).on('show.bs.modal', '.modal', function() { console.log('this will be fired')})

The issue when using jquery-UI's _on method is that the namespace gets cut off. So this will not work:

$.widget('mywidget', {
    _create: function() {
        this._on({
            'show.bs.modal .modal': function() {
                alert('this will not be fired');
            }
        });
    }
});

The issue is the regex in the _on method. It does not allow a dot in the event name and also it searches for zero to unlimited amount of spaces to split of the selector.
This results into the following:

var match = event.match( /^([\w:-]*)\s*(.*)$/ );
var eventName = match[ 1 ] + instance.eventNamespace; // "show.mywidget"
var selector = match[ 2 ]; // ".bs.modal .modal"

To prevent this I changed the regex to also allow dots so namespaces will not be broken apart and also to require at least one whitespace.

var match = event.match( /^([\w:.-]*)\s+(.*)$/ );
var eventName = match[ 1 ] + instance.eventNamespace; // "show.bs.modal.mywidget"
var selector = match[ 2 ]; // ".modal"

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 1, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: TimWerdin / name: Tim Werdin (cc6a600)

@mgol
Copy link
Member

mgol commented Oct 8, 2022

Thanks for the PR. All changes require tests, though. I’m not sure why existing tests didn’t run for this PR but maybe that would happen on the next update to the PR.

@fnagel
Copy link
Member

fnagel commented Oct 10, 2022

Closing and reopening in order to run the tests.

@fnagel fnagel closed this Oct 10, 2022
@fnagel fnagel reopened this Oct 10, 2022
@mgol
Copy link
Member

mgol commented Oct 10, 2022

@TimWerdin in addition to writing new tests, you need to ensure existing ones pass. You can now see that's not the case right now.

@Eydamos
Copy link
Author

Eydamos commented Oct 17, 2022

I was on vacation until yesterday. I will have a look this week

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

Successfully merging this pull request may close these issues.

4 participants