search

salesforce / kagekiri

Shadow DOM-piercing query APIs for the browser.
BSD 3-Clause "New" or "Revised" License
86 stars 9 forks source link

fix: traverse all possible ancestors when selector is a space #98

Closed shivamothkuri closed 5 months ago

shivamothkuri commented 6 months ago

PR for this issue: https://github.com/salesforce/kagekiri/issues/95

salesforce-cla[bot] commented 6 months ago

Thanks for the contribution! Before we can merge this, we need @shivamothkuri to sign the Salesforce Inc. Contributor License Agreement.

nolanlawson commented 6 months ago

Hi @shivamothkuri this looks reasonable to me. Could you please add a test that confirms the new behavior? You will also need to run npm run lint:fix to fix formatting issues. Thanks!

shivamothkuri commented 6 months ago

Hi @shivamothkuri this looks reasonable to me. Could you please add a test that confirms the new behavior? You will also need to run npm run lint:fix to fix formatting issues. Thanks!

Thanks for the review @nolanlawson Added a test case as suggested.

I found something which I want to clarify with you. I think Kagekiri will be the drop-in replacement for document.querySelector - meaning Kagekiri should take care of vanilla light dom, shadow dom and mixture of both. So, we should be testing kagekiri functionality for light dom as well. But I found light dom test cases were being tested using context.querySelector which is internally document.querySelector

I think we are missing to test lightdom functionality using kagekiri querySelector and querySelectorAll. Please correct my if am wrong.

https://github.com/salesforce/kagekiri/blob/c10bd235944f2cf80750ea95a37db07a0c773c64/test/querySelector.spec.js#L58-L68

nolanlawson commented 6 months ago

@shivamothkuri

I think we are missing to test lightdom functionality using kagekiri querySelector and querySelectorAll

That's a good point. The existing document.querySelector* tests are designed as a sanity-check to make sure that kagekiri has the same behavior as the browser implementation. As you say, lightdom+kagekiri would be a great test suite to add.

shivamothkuri commented 6 months ago

@nolanlawson added few more tests as suggested. Please check

nolanlawson commented 5 months ago

Thank you!!