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

fix: Teleport SVG elements #2648

Merged
merged 6 commits into from
Nov 30, 2020
Merged

fix: Teleport SVG elements #2648

merged 6 commits into from
Nov 30, 2020

Conversation

yassilah
Copy link
Contributor

Currently, when using Teleport to move an SVGElement or into an SVGElement, the element is not rendered . The issue is due to the fact that the Teleport "process" method is not checking whether the target element is itself an SVGElement and therefore uses createElement instead of createElementNS. This PR fixes that issue.

@yassilah
Copy link
Contributor Author

PS: I don't know if it's necessary to add a test or even how I could test that, since the element is in fact added to the DOM, it is simply not being rendered.

@posva
Copy link
Member

posva commented Nov 20, 2020

Can you provide a repro using one of the templates at https://new-issue.vuejs.org/?repo=vuejs/vue-next

@yassilah
Copy link
Contributor Author

Sorry about that, i just created the issue: #2652 !

Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR.

Can you add a test?

@LinusBorg LinusBorg added the need test The PR has missing test cases. label Nov 20, 2020
@@ -80,6 +80,13 @@ export const TeleportImpl = {

const disabled = isTeleportDisabled(n2.props)
const { shapeFlag, children } = n2
// Verify whether the target is an SVGElement or not.
const targetIsSVG = (target: RendererElement) => {
Copy link
Member

@edison1105 edison1105 Nov 21, 2020

Choose a reason for hiding this comment

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

use isSVGTag(target.tag) instead of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@yassilah
Copy link
Contributor Author

@LinusBorg I'm not quite sure how to add my test, I would either need a visual test or at least test if the element has the right namespace, but from what I saw, there is no namespaceURI on the TestElement class.

@posva
Copy link
Member

posva commented Nov 21, 2020

One thing to note is that this only work if the target is an svg but not if the target is inside the SVG

@yassilah
Copy link
Contributor Author

@posva what do you mean? i think this should work both if the target has an SVG tag or if the destination is an SVG element, or did i misunderstand how the Teleport.process method works?

@yassilah
Copy link
Contributor Author

I've added a test but still i'm a bit confused about the way Teleport works because:

  • in my tests, i needed to add a v-if or disabled prop to the Teleport component because i otherwise get an error saying the destination is invalid; that's not the behaviour i get when trying it out outside of a test;
  • it seems that somehow the "target" can sometimes be an HTMLorSVGElement meaning the "tag" property does not always exist and should be tagName;
  • i'm also confused about the fact that n1.props.to/n2.props.to don't match with the target when it is reactive (at least from the tests i ran); in my case even when n2.props.to is an SVGElement, the target is null.

This might all be related to the fact that i'm missing something about how to test the Teleport component.

@@ -9,7 +9,7 @@ import {
traverseStaticChildren
} from '../renderer'
import { VNode, VNodeArrayChildren, VNodeProps } from '../vnode'
import { isString, ShapeFlags } from '@vue/shared'
import { isString, isSVGTag, ShapeFlags } from '@vue/shared'
Copy link
Member

Choose a reason for hiding this comment

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

FYI isSVGTag is for compiler only and should be avoided in runtime code since it bloats the bundle size.

@yyx990803
Copy link
Member

I've added a test but still i'm a bit confused about the way Teleport works because:

  • in my tests, i needed to add a v-if or disabled prop to the Teleport component because i otherwise get an error saying the destination is invalid; that's not the behaviour i get when trying it out outside of a test;
  • it seems that somehow the "target" can sometimes be an HTMLorSVGElement meaning the "tag" property does not always exist and should be tagName;
  • i'm also confused about the fact that n1.props.to/n2.props.to don't match with the target when it is reactive (at least from the tests i ran); in my case even when n2.props.to is an SVGElement, the target is null.

This might all be related to the fact that i'm missing something about how to test the Teleport component.

  • The v-if is expected because a template ref could be null on the initial mount (which will trigger an update immediately when it's set).
  • target should always be a resolved native element. It's not the to prop.
  • Note sure what you mean about the props.to not matching (a test case might help)

@yyx990803
Copy link
Member

Made some updates:

  • isSVGTag is a compiler only helper. The original instanceof SVGElement check is actually better.
  • However, Teleport is technically platform agnostic so we must check for typeof SVGElement !== 'undefined' first.
  • ownerSVGElement is not necessary since all svg elements (not just <svg> itself) are instances of SVGElement.

@yyx990803 yyx990803 merged commit cd92836 into vuejs:master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need test The PR has missing test cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants