feat(mock-doc): add matrix and tspan props for svgelement #3408

marlon-ionic · 2022-06-06T16:34:33Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Some methods and properties of SVGElement and its children elements are missing from mock-doc.

What is the new behavior?

Per request from ZenDesk ticket 41657:

Added following members were added to SVGElement: createSVGPoint(), getBBox(), getComputedTextLength(), getCTM(), getScreenCTM()
Added focus() and blur() to MockElement

Does this introduce a breaking change?

Yes
No

Testing

Added test suites for the new svg members, one for the matrix-based values and another for getComputedTextLength() since that's related to SVG text elements. The thought was that if additional mockdoc updates are needed in the future, separating the suites would help with code organization.

Other information

rwaskiewicz · 2022-06-06T18:57:51Z

src/mock-doc/node.ts

@@ -316,6 +320,10 @@ export class MockElement extends MockNode {
    return this.children[0] || null;
  }

+  focus() {


focus accepts an optional parameter, which is an object with a single optional field, preventScroll.

Although folks won't use it in this implementation (since we have an empty implementation here), it may be nice to provide folks the ability to conform to the function's definition. Can we add an unused, optional parameter like so?

Suggested change

focus() {

focus(_options: { preventScroll?: boolean }) {

Suggested change

focus() {

focus(_options?: { preventScroll?: boolean }) {

Since options is optional too right?

Yep! Exactly

src/mock-doc/test/html-parse.spec.ts

rwaskiewicz · 2022-06-07T13:00:19Z

src/mock-doc/element.ts

@@ -267,6 +267,27 @@ export class MockStyleElement extends MockHTMLElement {
  }
 }

+//Based on deprecated SVGMatrix - http://html5index.org/SVG%20-%20SVGMatrix.html
+export const defaultSVGMatrix = {
+  a: 1,


For my own understanding, why do a and d get assigned a value of 1, but the other single letter properties of the matrix are assigned a value of 0?

src/mock-doc/element.ts

rwaskiewicz · 2022-06-07T13:07:10Z

src/mock-doc/element.ts

+  getBBox() {
+    return { x: 0, y: 0, width: 10, height: 10 };
+  }
+  getCTM() {
+    return defaultSVGMatrix;
+  }
+  getScreenCTM() {
+    return defaultSVGMatrix;
+  }


These methods appear to not exist on SVGElement, but rather it's child class SVGGraphicsElement. Can we please move these to a (new) MockSVGGraphicsElement class that extends MockSVGElement? The reason being is that these methods shouldn't be available on MockSVGElement (and by extension, any of its child classes except MockSVGGraphicsElement)

rwaskiewicz · 2022-06-07T13:39:37Z

src/mock-doc/element.ts

+  createSVGPoint() {
+    return {
+      matrixTransform: defaultSVGMatrix,
+      x: 0,
+      y: 0,
+    };
+  }
+  getBBox() {
+    return { x: 0, y: 0, width: 10, height: 10 };
+  }
+  getCTM() {
+    return defaultSVGMatrix;
+  }
+  getScreenCTM() {
+    return defaultSVGMatrix;
+  }


Can we add return types to the function signatures added here please, since they don't return undefined/void?

Sure but can I use named types like DOMMatrix, or DOMRect? or should the return types be anonymous? I'm not sure what's available while running in the testing context.

I think you can, there's prior art here in this file for using types like DOMPoint and SVGSVGElement

rwaskiewicz · 2022-06-07T13:43:30Z

src/mock-doc/element.ts

+  getComputedTextLength(): number {
+    return 0;
+  }
+  createSVGPoint() {


This method appears to not exist on SVGElement, but rather on SVGPoint. Can we please move these to a (new) SVGTextContentElement class that extends (a new) MockSSVGPoint? The reason being is that these methods shouldn't be available on MockSVGElement (and by extension, any of its child classes)

It looks like createSVGPoint() exists in SVGSVGElement so I created that instead

rwaskiewicz · 2022-06-07T13:45:39Z

src/mock-doc/element.ts

+  }
+  createSVGPoint() {
+    return {
+      matrixTransform: defaultSVGMatrix,


Looking at https://www.w3.org/TR/SVG11/coords.html#InterfaceSVGPoint, should this be a callable function rather than an attribute on the object?

Suggested change

matrixTransform: defaultSVGMatrix,

matrixTransform: () => defaultSVGMatrix,

rwaskiewicz · 2022-06-07T13:56:15Z

src/mock-doc/element.ts

+    return { x: 0, y: 0, width: 10, height: 10 };
+  }
+  getCTM() {
+    return defaultSVGMatrix;


I think the return value of getCTM needs to be a DOMMatrix. Although it's similar to an SVGMatrix, it looks like there are different properties/methods on the two

rwaskiewicz · 2022-06-07T13:56:24Z

src/mock-doc/element.ts

+    return defaultSVGMatrix;
+  }
+  getScreenCTM() {
+    return defaultSVGMatrix;


I think the return value of getScreenCTM needs to be a DOMMatrix. Although it's similar to an SVGMatrix, it looks like there are different properties/methods on the two

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

rwaskiewicz

Looks good! One outstanding question/comment

rwaskiewicz · 2022-06-13T15:31:22Z

src/mock-doc/element.ts

+  a: number = 1;
+  b: number = 0;
+  c: number = 0;
+  d: number = 1;


I had this question in the original implementation that I think still stands

For my own understanding, why do a and d get assigned a value of 1, but the other single letter properties of the matrix are assigned a value of 0?

Oh sorry, I missed responding to this one! I don't know. I simply used the default values returned when a DOMMatrix is instantiated (see screenshot).

rwaskiewicz

Thanks @marlon-ionic! All in all this looks good to me.

The last thing to do is to is resolve a few strictNullChecks errors that this PR is introducing in our test suite. They're a bit hard to find at the moment, as it looks like our CI check for these isn't working correctly against forks. I took the time to find/fix the violations that our tests are introducing, which you can see in the diff below. Can you apply the changes to your fork please? (I've also included a patch file so that you can apply all of the changes all at once), see 'rm-null-errors.txt' attached to this comment)

diff --git a/src/mock-doc/test/html-parse.spec.ts b/src/mock-doc/test/html-parse.spec.ts
index e015311d8..eb3286ae9 100644
--- a/src/mock-doc/test/html-parse.spec.ts
+++ b/src/mock-doc/test/html-parse.spec.ts
@@ -96,7 +96,8 @@ describe('parseHtml', () => {
         </svg>
       </svg>
     `);
-    const svgElem: MockSVGSVGElement = doc.body.firstElementChild.firstElementChild as MockSVGSVGElement;
+    const svgElem: MockSVGSVGElement = doc.body.firstElementChild?.firstElementChild as MockSVGSVGElement;
+    expect(svgElem).toBeDefined();
     expect(svgElem.getBBox()).toEqual(new MockSVGRect());
     expect(svgElem.createSVGPoint()).toEqual(new MockDOMPoint());
     expect(svgElem.getScreenCTM()).toEqual(new MockDOMMatrix());
@@ -112,9 +113,12 @@ describe('parseHtml', () => {
         </text>
       </svg>
     `);
-    const tspan: MockSVGTextContentElement = doc.body.firstElementChild.firstElementChild
-      .firstElementChild as MockSVGTextContentElement;
-    expect(doc.body.firstElementChild.firstElementChild.tagName).toEqual('text');
+    const text: MockSVGTextContentElement = doc.body.firstElementChild?.firstElementChild as MockSVGTextContentElement;
+    expect(text).toBeDefined();
+    expect(text.tagName).toEqual('text');
+
+    const tspan: MockSVGTextContentElement = text.firstElementChild as MockSVGTextContentElement;
+    expect(tspan).toBeDefined();
     expect(tspan.getComputedTextLength()).toEqual(0);
   });

Patch File:
rm-null-errors.txt

…/stencil into mh/test-mockdoc-svg

rwaskiewicz · 2022-06-13T21:53:56Z

I'm merging this as an admin - the tech debt burndown (download error files and report) has a bug in it where it doesn't work against forks of the repo. All other CI checks are passing, and no new strictNullChecks violations nor unused exports are being introduced in this PR.

marlon-ionic added 4 commits June 3, 2022 16:08

feat(mock-doc): add matrix and tspan props for svg

5dc7c14

added blur(), focus() to MockElement

796f66e

rm focus() from SVGElement, rename defaultMatrix

ff86348

update svg testing

6acc410

marlon-ionic requested a review from a team June 6, 2022 16:34

Merge branch 'main' into mh/test-mockdoc-svg

060a939

rwaskiewicz requested changes Jun 7, 2022

View reviewed changes

marlon-ionic and others added 4 commits June 8, 2022 15:12

Update src/mock-doc/test/html-parse.spec.ts

46a3b86

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update src/mock-doc/element.ts

dc878cc

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

added subest of svg child elements

8c870de

update MockDOMPoint

587b349

marlon-ionic requested a review from rwaskiewicz June 10, 2022 21:59

rwaskiewicz reviewed Jun 13, 2022

View reviewed changes

Merge branch 'main' into mh/test-mockdoc-svg

b6804a2

rwaskiewicz requested changes Jun 13, 2022

View reviewed changes

marlon-ionic added 2 commits June 13, 2022 15:00

apply strictNullChecks patch to html-parse.spect.ts

d9e2ea0

Merge branch 'mh/test-mockdoc-svg' of https://github.com/marlon-ionic…

29f49bf

…/stencil into mh/test-mockdoc-svg

rwaskiewicz approved these changes Jun 13, 2022

View reviewed changes

rwaskiewicz merged commit d3b93c1 into stenciljs:main Jun 13, 2022

rwaskiewicz added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mock-doc): add matrix and tspan props for svgelement #3408

feat(mock-doc): add matrix and tspan props for svgelement #3408

marlon-ionic commented Jun 6, 2022

rwaskiewicz Jun 6, 2022

marlon-ionic Jun 8, 2022

rwaskiewicz Jun 8, 2022

rwaskiewicz Jun 7, 2022

rwaskiewicz Jun 7, 2022

rwaskiewicz Jun 7, 2022

marlon-ionic Jun 9, 2022

rwaskiewicz Jun 10, 2022

rwaskiewicz Jun 7, 2022

marlon-ionic Jun 10, 2022

rwaskiewicz Jun 7, 2022

rwaskiewicz Jun 7, 2022

rwaskiewicz Jun 7, 2022

rwaskiewicz left a comment

rwaskiewicz Jun 13, 2022

marlon-ionic Jun 13, 2022

rwaskiewicz left a comment •

edited

Loading

rwaskiewicz commented Jun 13, 2022

	matrixTransform: defaultSVGMatrix,
	matrixTransform: () => defaultSVGMatrix,

feat(mock-doc): add matrix and tspan props for svgelement #3408

feat(mock-doc): add matrix and tspan props for svgelement #3408

Conversation

marlon-ionic commented Jun 6, 2022

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment • edited Loading

Choose a reason for hiding this comment

rwaskiewicz commented Jun 13, 2022

rwaskiewicz left a comment •

edited

Loading