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

Support jQuery selectors #29

Open
chocolateboy opened this issue Apr 11, 2018 · 17 comments
Open

Support jQuery selectors #29

chocolateboy opened this issue Apr 11, 2018 · 17 comments

Comments

@chocolateboy
Copy link

I'm trying to migrate away from another jQuery plugin, which has several bugs, to this, which doesn't have either of those bugs. 👍

But, looking through the code, there appears to a fair bit of code devoted to parsing the selector, which seems fragile. More importantly, nodes are matched with Element.matches, which means that jQuery-specific features like :visible won't work, and jQuery extensions like :not only partially work.

Since it's a jQuery plugin, why not just use $(node).is(selector) instead of Element.matches + custom parsing?

@bezborodow
Copy link
Collaborator

bezborodow commented Apr 12, 2018

Hi @chocolateboy, thanks for your interest.

The plugin only parses the selector to learn about the nature of the selector. This allows some small performance tweaks that are admittedly probably not of huge benefit in most cases, but could be useful in certain circumstances. For example, if the selector does not contain attribute selectors, then the plugin will not ask the MutationObserver to observe mutations to the target's attributes. See:

  • attributes | Set to true if mutations to target's attributes are to be observed.

Please note that actual CSS selector parsing is handled by native JavaScript (querySelectorAll), not by any custom parsing. The plugin only parses the selector to extract meta data, not scan the document. By the way, there was a bug with using jQuery's selector, which is why this plugin uses the native function querySelectorAll.

If you can identify any practical bugs as a consequence of this approach, please let us know.

@bezborodow
Copy link
Collaborator

bezborodow commented Apr 12, 2018

You might notice that jQuery-onMutate observes a specific node in the DOM. By default, jQuery.initialise observes the entire document. You can get the same behaviour as per below.

$('#observe-this-element').onCreate('div.foo', callbackFunction, true);

Is equivalent to:

$.initialize('div.foo', callbackFunction, { target: document.getElementById('observe-this-element') });

Also note that this plugin does not have a concept of multi = boolean. The plugin always matches multiple times. There is a feature request to support this under #24.

@chocolateboy
Copy link
Author

Thanks. I haven't spotted any bugs.

I suppose my question was more about why jQuery isn't being used to process the selector rather than why it's being manually parsed. Neither qSA nor Element.matches support jQuery additions like :visible or extensions like :not, both of which I use and would be reluctant to sacrifice for a hypothetical performance gain.

@chocolateboy
Copy link
Author

chocolateboy commented Apr 12, 2018

By default, jQuery.initialise observes the entire document.

That's fine. I usually call its as $.onCreate, which is equivalent to $(document).onCreate, so I wouldn't miss that feature greatly by migrating away from jQuery-onMutate. Likewise, I always set multi = true on jQuery-onMutate (and add a comment to explain the mysterious boolean), so that wouldn't be a loss either.

@bezborodow
Copy link
Collaborator

bezborodow commented Apr 12, 2018

I must reiterate, there is no manual parsing of the selector except for extracting meta data about the selector (the grok method.) The meta data is used to alter the behaviour of how the plugin works depending on what kind of selector is passed to it, mainly for performance reasons. This is a completely separate logical part of the code and is not involved in scanning the DOM.

The actual scanning of the document uses querySelectorAll. The reason why I chose to use querySelectorAll was not based on performance considerations. There was some sort of recursive bug when using jQuery's .find() method every time a mutation is observed that caused the browser to hang/crash.

@chocolateboy
Copy link
Author

OK, I think there's a misunderstanding, and I guess I'm not making myself clear. It's specifically the jQuery functionality I'm requesting. CSS (i.e. querySelectorAll) doesn't support jQuery extensions. I'll try to come up with a new wording for this issue, or feel free to close it if there's no interest in supporting these extensions.

@bezborodow
Copy link
Collaborator

Right. I had to use querySelectorAll because of a bug when using .find() that caused the browser to crash. The bug seemed recursive in nature, almost as if using .find() triggered a mutation, which then caused a recursive loop of mutation -> find -> mutation -> find ... ad infinitum.

@chocolateboy chocolateboy changed the title Why parse the selector? Support jQuery selectors Apr 12, 2018
@bezborodow
Copy link
Collaborator

Hi @chocolateboy , I quickly implemented this on feature/support-jquery-selectors. The bug I mentioned doesn't seem to be apparent. Please test or suggest changes if you are interested.

@bezborodow
Copy link
Collaborator

So, I reproduced the bug that originally made it impossible to support jquery selectors.

On the feature/support-jquery-selectors branch, go to test2.html, and click on "Just add .initialize-me to .wrong-class." The browser will then crash.

@shamel67
Copy link

I'm also switching from onMutate to your library. Regarding the apparent looping when using .find(), you might have to disconnect your observer just before doing .find(), and reconnect it after. At least that's what I had to do in one of my projects to avoid infinitely looping because I was creating a DOM element within my observer callback.

@jvosloo
Copy link

jvosloo commented Oct 26, 2018

I think my issue is related to this issue.

This used to work with v1.0.0:

$("input:submit,input:button,a.button,button:submit").initialize( function(){    
    $(this).button();   
});

but breaks with v1.3.0 ?

@bezborodow
Copy link
Collaborator

bezborodow commented Oct 27, 2018

Yes, your issue is related to this ticket. You may want to use button, input[type='button'] for :button and input[type="submit"], button[type="submit"] for :submit as per documentation:

https://api.jquery.com/button-selector/
https://api.jquery.com/submit-selector/

Also, note that the API has changed, so you should use the following:

$.initialize("button,a.button,input[type='submit'],input[type='button']", function() {    
    $(this).button();   
});

@bezborodow
Copy link
Collaborator

Possibly related jquery/jquery#4332?

@bezborodow
Copy link
Collaborator

I have something on feature/support-jquery-selectors2 that seems to work with jQuery 3.7.0. However, I will not release this until I have made it backward-compatible. Older versions will hang.

@bezborodow
Copy link
Collaborator

If anybody would like to test this on feature/support-jquery-selectors2 with jQuery 3.7.0, and also check older versions less than 3.7.0, then we can think about releasing an update to resolve this issue.

@bezborodow bezborodow added the testing Needs testing label May 26, 2023
@bezborodow
Copy link
Collaborator

If this fails testing, our next chance might be jQuery 4.0, which "inlines & simplifies Sizzle."

@bezborodow
Copy link
Collaborator

This is on hold for now.

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

Successfully merging a pull request may close this issue.

4 participants