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

Don't use localName to identify tags, if it's not populated #498

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

djjeck
Copy link
Collaborator

@djjeck djjeck commented Mar 24, 2023

Don't use localName to identify tags, if it's not populated.

In browsers that don't populate localName, idom couldn't identify the tag name during patch, and would always think that a different tag was being re-rendered, always recreating DOM nodes rather than reusing them (you can't change the tag name of an existing node).

I verified this issue on Cobalt 9, but this may also affect IE <=8 and other old browsers. https://caniuse.com/?search=localName

Quick links:

Don't use localName to identify tags, if it's not populated.

I verified this issue on Cobalt 9, but this may also affect IE <=8. https://caniuse.com/?search=localName

In browsers that don't populate localName, idom couldn't identify the tag name during patch, and would always think that a different tag was being re-rendered, always recreating DOM nodes rather than reusing them (you can't change the tag name of an existing node).

Quick links:
Element.localName https://developer.mozilla.org/en-US/docs/Web/API/Element/localName
Node.nodeName https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName
Element.tagName https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName
@djjeck djjeck changed the title Update node_data.ts Don't use localName to identify tags, if it's not populated Mar 27, 2023
@djjeck djjeck marked this pull request as draft March 27, 2023 14:07
Remove runtime Element check and force TypeScript to access `localName`.

Changed check to use `??` after I verified that localName is not the empty string
@djjeck djjeck requested a review from iteriani March 27, 2023 14:25
@djjeck djjeck marked this pull request as ready for review March 27, 2023 14:26
@jridgewell
Copy link
Contributor

nodeName has the incorrect name casing for checks: "foreignObject" !== "FOREIGNOBJECT". You'll need to fix everywhere that uses NodeData.nameOrCtor.

@djjeck
Copy link
Collaborator Author

djjeck commented Mar 27, 2023

nodeName has the incorrect name casing for checks: "foreignObject" !== "FOREIGNOBJECT". You'll need to fix everywhere that uses NodeData.nameOrCtor.

I'm sorry, I'm not familiar with the idom implementation. I don't understand what I would need to fix.
If the tag name only comes from this line, isn't it self consistent within the algorithm? I don't see any other reference in the codebase to nodeName, localName, or tagName. Are you talking about wiz checks?
Could we just normalize to uppercase here?

@jridgewell
Copy link
Contributor

nodeName is always capitalized, while localName gives the correct casing:

const d = document.createElement('div');
assert.equal(d.nodeName, 'DIV');
assert.equal(d.localName, 'div');

This is particularly important for SVG, which has case-sensitive element names. Eg, document.createElementNs('http://www.w3.org/2000/svg', 'clippath') will not inherit from SVGClipPathElement, only 'clipPath' will.

In short, everywhere that reads NodeData.nameOrCtor will need to be updated to handle the fact that we can read either a correclty-cased localName or an uppercase nodeName, eg:

If you were delete the localName branch in your PR, you'd see every test case fail.

@djjeck djjeck marked this pull request as draft March 28, 2023 20:22
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.

2 participants