fix(): Fix regex to parse the viewbox attribute to be more strict #10636

asturur · 2025-05-26T13:02:20Z

Description

closes #10635

In Action

codesandbox · 2025-05-26T13:02:22Z

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

github-actions · 2025-05-26T13:04:19Z

Build Stats

file / KB (diff)	bundled	minified
fabric	916.091 (+0.045)	301.478 (+0.017)

Smrtnyk · 2025-05-26T13:07:14Z

does this work with a viewbox like
element.setAttribute('viewBox', '0,0 100,50');

is this even valid?
I think web allows mixed separators
an example test case

it('viewBox with mixed separators', () => {
      const element = document.createElementNS(
        'http://www.w3.org/2000/svg',
        'svg',
      );
      element.setAttribute('viewBox', '0,0 100,50');
      element.setAttribute('width', '200');
      element.setAttribute('height', '100');

      const result = applyViewboxTransform(element);

      expect(result.viewBoxWidth).toBe(100);
      expect(result.viewBoxHeight).toBe(50);
    });

asturur · 2025-05-26T13:11:21Z

does this work with a viewbox like element.setAttribute('viewBox', '0,0 100,50');

is this even valid? I think web allows mixed separators an example test case

it('viewBox with mixed separators', () => {
      const element = document.createElementNS(
        'http://www.w3.org/2000/svg',
        'svg',
      );
      element.setAttribute('viewBox', '0,0 100,50');
      element.setAttribute('width', '200');
      element.setAttribute('height', '100');

      const result = applyViewboxTransform(element);

      expect(result.viewBoxWidth).toBe(100);
      expect(result.viewBoxHeight).toBe(50);
    });

is that on spec? if is on spec it should work. let's add a test case for it.

Smrtnyk · 2025-05-26T13:12:49Z

https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Attribute/viewBox
It has this part

The value of the viewBox attribute is a list of four numbers separated by whitespace and/or a comma: min-x, min-y, width, and height

the part and/or threw me off, meaning it allows mixed stuff

… fix-viewbox-attr

asturur · 2025-05-26T13:14:18Z

0,0 100,50

Yes SVG is crazy specs.
Write numbers however you want as long they are really super wrong and they should work. Not sure why.
But this wasn't working even before my changes, was it?

Smrtnyk · 2025-05-26T13:14:52Z

0,0 100,50
Yes SVG is crazy specs. Write numbers however you want as long they are really super wrong and they should work. Not sure why. But this wasn't working even before my changes, was it?

apparently not
I think we need a really crazy regex here

export const reViewBoxAttrValue = new RegExp(
  '^\\s*' +
  '(' + reNum + ')' +
  '(?:\\s*,?\\s+|\\s*,\\s*)' + 
  '(' + reNum + ')' +
  '(?:\\s*,?\\s+|\\s*,\\s*)' +
  '(' + reNum + ')' +
  '(?:\\s*,?\\s+|\\s*,\\s*)' +
  '(' + reNum + ')' +
  '\\s*$'
);

maybe this

asturur · 2025-05-26T13:16:29Z

i don't get it, the comma is there.

asturur · 2025-05-26T13:20:34Z

I do not understand why the space make it pass.

Smrtnyk · 2025-05-26T13:21:36Z

I do not understand why the space make it pass.

have you tried the one I suggested above?

Smrtnyk · 2025-05-26T13:30:04Z

also I think we should not just test these constants but fully the applyViewBoxTransform file
we can add applyViewboxTransform.spec.ts file and then we test the full pipeline through it

here are some test cases I came up with

describe('applyViewboxTransform()', () => {
  it('return empty object for non-viewBox elements', () => {
    const element = document.createElement('rect');
    const result = applyViewboxTransform(element);
    expect(result).toEqual({});
  });

  it('return empty object for unsupported elements', () => {
    const element = document.createElement('circle');
    const result = applyViewboxTransform(element);
    expect(result).toEqual({});
  });

  it('handle missing viewBox with valid dimensions', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('width', '100');
    element.setAttribute('height', '200');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 200,
    });
  });

  it('handle missing viewBox and missing dimensions', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 0,
      height: 0,
    });
  });

  it('handle missing viewBox with percentage dimensions', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('width', '100%');
    element.setAttribute('height', '100%');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 0,
      height: 0,
    });
  });

  it('handle x/y translation for missing viewBox', () => {
    const parent = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'symbol',
    );
    parent.appendChild(element);

    element.setAttribute('x', '10');
    element.setAttribute('y', '20');
    element.setAttribute('width', '100');
    element.setAttribute('height', '200');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 200,
    });
    expect(element.getAttribute('transform')).toContain('translate(10 20)');
    expect(element.hasAttribute('x')).toBe(false);
    expect(element.hasAttribute('y')).toBe(false);
  });

  it('handles invalid viewBox with 3 values', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0 0 408');
    element.setAttribute('width', '100');
    element.setAttribute('height', '200');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 200,
    });
  });

  it('handle invalid viewBox with malformed values', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', 'invalid values here');
    element.setAttribute('width', '100');
    element.setAttribute('height', '200');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 200,
    });
  });

  it('handle empty viewBox', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '');
    element.setAttribute('width', '100');
    element.setAttribute('height', '200');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 200,
    });
  });

  it('parse valid viewBox with dimensions', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 200,
      height: 100,
      minX: 0,
      minY: 0,
      viewBoxWidth: 100,
      viewBoxHeight: 50,
    });
  });

  it('parse valid viewBox without dimensions', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '10 20 100 50');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 50,
      minX: -10,
      minY: -20,
      viewBoxWidth: 100,
      viewBoxHeight: 50,
    });
  });

  it('handle negative viewBox values', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '-10 -20 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 200,
      height: 100,
      minX: 10,
      minY: 20,
      viewBoxWidth: 100,
      viewBoxHeight: 50,
    });
  });

  it('handle decimal viewBox values', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0.5 1.5 100.5 50.5');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 200,
      height: 100,
      minX: -0.5,
      minY: -1.5,
      viewBoxWidth: 100.5,
      viewBoxHeight: 50.5,
    });
  });

  it('not generate transform for identity case', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '100');
    element.setAttribute('height', '50');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 100,
      height: 50,
      minX: 0,
      minY: 0,
      viewBoxWidth: 100,
      viewBoxHeight: 50,
    });
    expect(element.getAttribute('transform')).toBeFalsy();
  });

  it('generate transform matrix for scaling', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    applyViewboxTransform(element);

    const gElement = element.querySelector('g');
    expect(gElement).toBeTruthy();
    expect(gElement?.getAttribute('transform')).toContain(
      'matrix(2 0 0 2 0 0)',
    );
  });

  it('generate transform matrix with translation', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '10 20 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    applyViewboxTransform(element);

    const gElement = element.querySelector('g');
    expect(gElement?.getAttribute('transform')).toContain(
      'matrix(2 0 0 2 -20 -40)',
    );
  });

  it('handle x/y attributes with viewBox', () => {
    const parent = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'symbol',
    );
    parent.appendChild(element);

    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');
    element.setAttribute('x', '5');
    element.setAttribute('y', '10');

    applyViewboxTransform(element);

    expect(element.getAttribute('transform')).toContain('translate(5 10)');
    expect(element.getAttribute('transform')).toContain('matrix(2 0 0 2 0 0)');
  });

  it('create g wrapper for svg elements', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const child = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'rect',
    );
    element.appendChild(child);

    applyViewboxTransform(element);

    const gElement = element.querySelector('g');
    expect(gElement).toBeTruthy();
    expect(gElement?.contains(child)).toBe(true);
    expect(element.children.length).toBe(1);
    expect(element.children[0]).toBe(gElement);
  });

  it('apply transform directly to non-svg elements', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'symbol',
    );
    element.setAttribute('viewBox', '0 0 100 50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');
    element.setAttribute('transform', 'rotate(45)');

    applyViewboxTransform(element);

    expect(element.getAttribute('transform')).toContain('rotate(45)');
    expect(element.getAttribute('transform')).toContain('matrix(2 0 0 2 0 0)');
    expect(element.querySelector('g')).toBeFalsy();
  });

  it('handle scientific notation in viewBox', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '1e-2 2e-3 1e2 5e1');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result).toEqual({
      width: 200,
      height: 100,
      minX: -0.01,
      minY: -0.002,
      viewBoxWidth: 100,
      viewBoxHeight: 50,
    });
  });

  it('viewBox with mixed separators', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '0,0 100,50');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result.viewBoxWidth).toBe(100);
    expect(result.viewBoxHeight).toBe(50);
  });

  it('handle whitespace in viewBox', () => {
    const element = document.createElementNS(
      'http://www.w3.org/2000/svg',
      'svg',
    );
    element.setAttribute('viewBox', '  0   0   100   50  ');
    element.setAttribute('width', '200');
    element.setAttribute('height', '100');

    const result = applyViewboxTransform(element);

    expect(result.viewBoxWidth).toBe(100);
    expect(result.viewBoxHeight).toBe(50);
  });
});

not sure if these all are valid test cases but we could pick some out

asturur · 2025-05-26T13:55:05Z

yeah i could test higher level, but now i m changing the regex, i test the regex. I can copy paste those tests somewhere else but when i do those changes i have to hope other unit tests would catch regressions. visual or unit

asturur · 2025-05-26T13:55:42Z

oh i see the issue, some spaces are optional, other are mandatory. that is wrong. all spaces are optional

Smrtnyk · 2025-05-26T13:57:50Z

yeah i could test higher level, but now i m changing the regex, i test the regex. I can copy paste those tests somewhere else but when i do those changes i have to hope other unit tests would catch regressions. visual or unit

Yeah the tests i proposed are for a new file where apply viewbox lives
They all pass for me with the regex i proposed above

asturur · 2025-05-26T14:29:03Z

apply viewbox transform is such a confusing function :(
it change the element and returns data, it was my early js coding and trying to save any byte where possible

Smrtnyk · 2025-05-26T14:40:40Z

apply viewbox transform is such a confusing function :( it change the element and returns data, it was my early js coding and trying to save any byte where possible

Yes, that is why i proposed more compound tests, because i wasnt sure if these return values were actually corect, mainly these with -0

asturur · 2025-05-27T18:54:17Z

Yeah no sure why the minus. i forgot.

…0636)

asturur added 2 commits May 26, 2025 15:01

add tests

a88ed4d

add file

bade67d

asturur mentioned this pull request May 26, 2025

[Bug]: "viewbox" attribute regular expression parsing issue #10635

Closed

7 tasks

github-actions bot and others added 2 commits May 26, 2025 13:03

update CHANGELOG.md

a08b171

no dist

de5af0b

Merge branch 'master' into fix-viewbox-attr

dd1a1ea

asturur added 2 commits May 26, 2025 15:13

it doesnt work

6e19947

Merge branch 'fix-viewbox-attr' of github.com:fabricjs/fabric.js into…

5f0cef3

… fix-viewbox-attr

asturur added 4 commits May 26, 2025 15:59

this will make everything explode

0b90ea1

this will make everything explode

1139451

i got the difference now

2f721be

why minus zero

a5c401b

see if it fixes lint

c6d35fe

asturur merged commit 6a60f29 into master May 26, 2025
13 checks passed

asturur deleted the fix-viewbox-attr branch May 26, 2025 14:40

asturur added a commit that referenced this pull request May 30, 2025

fix(): Fix regex to parse the viewbox attribute to be more strict (#1…

02d6539

…0636)

Uh oh!

fix(): Fix regex to parse the viewbox attribute to be more strict #10636

fix(): Fix regex to parse the viewbox attribute to be more strict #10636

Uh oh!

Conversation

asturur commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

In Action

Uh oh!

codesandbox bot commented May 26, 2025

Review or Edit in CodeSandbox

Uh oh!

github-actions bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Smrtnyk commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented May 26, 2025

Uh oh!

Smrtnyk commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented May 26, 2025

Uh oh!

Smrtnyk commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented May 26, 2025

Uh oh!

asturur commented May 26, 2025

Uh oh!

Smrtnyk commented May 26, 2025

Uh oh!

Smrtnyk commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented May 26, 2025

Uh oh!

asturur commented May 26, 2025

Uh oh!

Smrtnyk commented May 26, 2025

Uh oh!

asturur commented May 26, 2025

Uh oh!

Uh oh!

Smrtnyk commented May 26, 2025

Uh oh!

asturur commented May 27, 2025

Uh oh!

Uh oh!

asturur commented May 26, 2025 •

edited

Loading

github-actions bot commented May 26, 2025 •

edited

Loading

Smrtnyk commented May 26, 2025 •

edited

Loading

Smrtnyk commented May 26, 2025 •

edited

Loading

Smrtnyk commented May 26, 2025 •

edited

Loading

Smrtnyk commented May 26, 2025 •

edited

Loading