Skip to content

fix(prefixIds): does not handle SMIL timing offsets#2208

Open
yangxu52 wants to merge 2 commits into
svg:mainfrom
yangxu52:fix/prefixIds-not-handle-SMIL-offsets
Open

fix(prefixIds): does not handle SMIL timing offsets#2208
yangxu52 wants to merge 2 commits into
svg:mainfrom
yangxu52:fix/prefixIds-not-handle-SMIL-offsets

Conversation

@yangxu52

Copy link
Copy Markdown

This PR improves handling of begin and end attributes in the prefixIds plugin.

Problem

The current implementation only prefixes values ending with .end or .start, but valid SMIL syntax also includes offsets such as:

  • a.end-0.5s
  • b.begin+0.1s

These cases are currently ignored, leading to broken animation references after ID prefixing.

Solution

Replace the suffix check with a regex that correctly parses:
<id>.(begin|end)(+/-offset)?
This ensures all valid SMIL timing references are properly prefixed.

Impact

  • Fixes broken animation chaining
  • Maintains backward compatibility
  • No effect on non-SMIL attributes

Example

Before:

begin="a.end-0.5s"

After

begin="prefix-a.end-0.5s"

Test

  1. without offsets, like x.end, OK
  2. with offsets, like b.end-0.5s, OK
  3. mix and multiple, like x.begin;x.end-0.5s, OK

Test code e.g:

it('should prefix SMIL begin/end with offsets', () => {
  const svg = `
    <svg>
      <animate id="a" begin="b.end-0.5s" />
      <animate id="b" begin="a.begin+0.1s" />
    </svg>
  `

  const result = optimize(svg, {
    plugins: [
      'cleanupIds',
      {
        name: 'prefixIds',
        params: { prefix: 'p' },
      },
    ],
  })
  expect(result.data).toContain('p-b.end-0.5s')
  expect(result.data).toContain('p-a.begin+0.1s')
})
image

Notice
This PR intentionally keeps a minimal regex-based approach consistent with existing implementation, while extending coverage to valid SMIL offset syntax instead of all.

Linked Issue
Fixes: #2207 #2073

- Using the `replace` callback function to handle the offset and else, replacing the original `endsWith` matching
- adjust the RegExp of split to adapt multiple value
@yangxu52

Copy link
Copy Markdown
Author

Hi, @SethFalco

This PR has been submitted for some time, and both CI tests have passed.
It is also associated with Issue #2207 #2073
I wonder if it would be convenient for you to help review it?
If there are any necessary modifications/additions, I will follow up at any time.

Thanks!

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.

prefixIds does not correctly handle SMIL begin/end with offsets (e.g. ".end-0.5s")

1 participant