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

Selector Engine: fix multiple IDs #39201

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Selector Engine: fix multiple IDs #39201

merged 5 commits into from
Feb 18, 2024

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Sep 17, 2023

Description

Motivation & Context

A draft solution, which supports multiple ids, and usage off CSS.escape at the same time

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Ref: #38989 , #339198

closes: #39189

@GeoSot GeoSot requested a review from a team as a code owner September 17, 2023 22:33
@GeoSot GeoSot marked this pull request as draft September 17, 2023 22:37
@GeoSot GeoSot marked this pull request as ready for review September 18, 2023 12:46
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked everything in detail but I was thinking about the same kind of patch when I tried to analyse the issue.
Your patch fixes the issue, and the multiple target ids system is still working.

Ideas of improvement:

  • Add a new test with a multiple target ids containing special chars; something like the following:
    it('should get elements if several ids with special chars are given', () => {
      fixtureEl.innerHTML = [
        '<div id="test" data-bs-target="#j_id11:exampleModal,#j_id22:exampleModal"></div>',
        '<div class="target" id="j_id11:exampleModal"></div>',
        '<div class="target" id="j_id22:exampleModal"></div>'
      ].join('')

      const testEl = fixtureEl.querySelector('#test')

      expect(SelectorEngine.getMultipleElementsFromSelector(testEl)).toEqual(Array.from(fixtureEl.querySelectorAll('.target')))
    })
  • Add other tests with weird authorized characters as IDs to check that the selector engine escapes the special chars when needed. If I understand well the HTML 5 spec, it seems that almost all characters are authorized except spaces in IDs.
    • Note: not for this PR but maybe in the future we should change the separator for data-bs-target (with multiple targets) to replace , which seems to be authorized as a character in IDs.

FWIW, it doesn't seem that #39198 is linked to this regression and is rather linked to some bad usage + a possible missing escape of characters in scrollspy's JS with anchors. So another topic.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as explained in #39201 (review).

I've added an unit test for SelectorEngine too via e9c649b. Feel free to revert this commit if you don't agree with this approach.

@XhmikosR and/or @GeoSot, I let you double-check this fix in depth and determine if we need to release it in v5.3.3 soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

invalid selector regression in 5.3.2
3 participants