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

Use getAttribute when soft matching ids. #131

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

Conversation

dombesz
Copy link

@dombesz dombesz commented Mar 11, 2025

Some javascript frameworks like angular can overwrite the tag's id getter, and comparison can turn into a false negative. Newly added elements don't have the framework's code initialised yet, and calling id on those elements can differ from the id assigned via angular. Since there is no way to have frameworks initialised on the new elements, calling the id can be inconsistent. Using getAttribute("id") ensures that we always compare the html id attributes.

See this issue for more details: #130

@botandrose
Copy link
Collaborator

Hey @dombesz , thanks for the PR! A couple of notes before I review:

  1. Don't modify the files in dist/, we rebuild these only just before a release.
  2. Can you get CI green? You can do a simulation locally with npm test:ci. More details in TESTING.md

@dombesz dombesz force-pushed the fix-node-matching branch from ae36057 to 25001df Compare March 11, 2025 21:02
@dombesz
Copy link
Author

dombesz commented Mar 11, 2025

Hi @botandrose , thanks for the quick feedback, the CI should be green now.

    Some javascript frameworks like angular can overwrite the tag's id
    getter, and comparison can turn into a false negative. Newly added
    elements don't have the framework's code initialised yet, and calling id
    on those elements can differ from the id assigned via angular. Since
    there is no way to have frameworks initialized on the new elements,
    calling the uid=501(dombesz) gid=20(staff) groups=20(staff),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),702(com.apple.sharepoint.group.2),701(com.apple.sharepoint.group.1),33(_appstore),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae) can be inconsistent. Using getAttribute("id") ensures
    that we always compare the html id attributes.

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

Successfully merging this pull request may close these issues.

2 participants