Skip to content

Improve handling of is-elements and Fix tiny bugs of setAttr()/updateStyle() #2988

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.

Sign up for GitHub

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

Merged
dead-claudia merged 12 commits into from
Feb 8, 2025
Merged

Improve handling of is-elements and Fix tiny bugs of setAttr()/updateStyle() #2988

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e4b4791
setAttr/removeAttr: remove `key === "is"`, fixes #2799
kfule Feb 4, 2025
1e8850d
swap set and removal order of attributes/style properties
kfule Feb 4, 2025
f944a7e
bypass css text
kfule Feb 4, 2025
e8534b0
Vnode: add vnode.is to handle is-element
kfule Feb 5, 2025
0ce4634
refactor isEmpty()
kfule Feb 5, 2025
6e9e11b
move `old.is === vnode.is` evaluation to the top of updateNode()
kfule Feb 5, 2025
ae6e60f
revert isEmpty() back
kfule Feb 6, 2025
c290f0a
hasPropertyKey: evaluate `vnode.is` instead of `vnode.attrs.is`
kfule Feb 6, 2025
9d4068b
execSelector: use `attrs.is != null` instead of `hasOwn.call(attrs, "…
kfule Feb 8, 2025
473ee08
Vnode: initialize `is` with undefined
kfule Feb 8, 2025
65f0066
add manual test
kfule Feb 8, 2025
c72556c
Update hyperscript.js
dead-claudia Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions render/hyperscript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ function execSelector(state, vnode) {
attrs = Object.assign({type: attrs.type}, attrs)
}

// This reduces the complexity of the evaluation of "is" within the render function.
if (attrs.is != null) vnode.is = attrs.is
Comment thread
dead-claudia marked this conversation as resolved.
Outdated

vnode.attrs = attrs

return vnode
Expand Down
46 changes: 25 additions & 21 deletions render/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ module.exports = function() {
function createElement(parent, vnode, hooks, ns, nextSibling) {
var tag = vnode.tag
var attrs = vnode.attrs
var is = attrs && attrs.is
var is = vnode.is

ns = getNameSpace(vnode) || ns

Expand Down Expand Up @@ -396,7 +396,7 @@ module.exports = function() {
}
function updateNode(parent, old, vnode, hooks, nextSibling, ns) {
var oldTag = old.tag, tag = vnode.tag
if (oldTag === tag) {
if (oldTag === tag && old.is === vnode.is) {
vnode.state = old.state
vnode.events = old.events
if (shouldNotUpdate(vnode, old)) return
Expand Down Expand Up @@ -643,7 +643,7 @@ module.exports = function() {
}
}
function setAttr(vnode, key, old, value, ns) {
if (key === "key" || key === "is" || value == null || isLifecycleMethod(key) || (old === value && !isFormAttribute(vnode, key)) && typeof value !== "object") return
if (key === "key" || value == null || isLifecycleMethod(key) || (old === value && !isFormAttribute(vnode, key)) && typeof value !== "object") return
if (key[0] === "o" && key[1] === "n") return updateEvent(vnode, key, value)
if (key.slice(0, 6) === "xlink:") vnode.dom.setAttributeNS("http://www.w3.org/1999/xlink", key.slice(6), value)
else if (key === "style") updateStyle(vnode.dom, old, value)
Expand Down Expand Up @@ -676,7 +676,7 @@ module.exports = function() {
}
}
function removeAttr(vnode, key, old, ns) {
if (key === "key" || key === "is" || old == null || isLifecycleMethod(key)) return
Comment thread
dead-claudia marked this conversation as resolved.
if (key === "key" || old == null || isLifecycleMethod(key)) return
if (key[0] === "o" && key[1] === "n") updateEvent(vnode, key, undefined)
else if (key === "style") updateStyle(vnode.dom, old, null)
else if (
Expand Down Expand Up @@ -710,22 +710,24 @@ module.exports = function() {
if ("selectedIndex" in attrs) setAttr(vnode, "selectedIndex", null, attrs.selectedIndex, undefined)
}
function updateAttrs(vnode, old, attrs, ns) {
if (old && old === attrs) {
console.warn("Don't reuse attrs object, use new object for every redraw, this will throw in next major")
}
if (attrs != null) {
for (var key in attrs) {
setAttr(vnode, key, old && old[key], attrs[key], ns)
}
}
// Some attributes may NOT be case-sensitive (e.g. data-***),
// so removal should be done first to prevent accidental removal for newly setting values.
Copy link
Copy Markdown
Member

@dead-claudia dead-claudia Feb 8, 2025

Choose a reason for hiding this comment

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

That's what the inner attrs[key] == null condition is for in the remove loop.

Is there a test case that failed with the old code? If not, I'd like to either see one, a benchmark result, or a bundle size reduction to justify the restructuring here.

Copy link
Copy Markdown
Contributor Author

@kfule kfule Feb 8, 2025

Choose a reason for hiding this comment

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

That's what the inner attrs[key] == null condition is for in the remove loop.

Yes, that condition is the cause of the deletion issue.

An example of a test case that does not work well with the old mithril code is the flems described in the description at the top of this PR.

The performance does not seem to be affected by this code swap.
As for the bundle size, the bundle size is reduced by 1 byte gzipped due to the console.warn move (the code old && is removed).

Copy link
Copy Markdown
Contributor Author

@kfule kfule Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I changed only this part and measured the bundle size.

original (current main branch)
Original size: 20,075 bytes gzipped (67,460 bytes uncompressed)
Compiled size: 8,960 bytes gzipped (24,405 bytes uncompressed)

original + swap (without console.warn move)
Original size: 20,082 bytes gzipped (67,460 bytes uncompressed)
Compiled size: 8,963 bytes gzipped (24,405 bytes uncompressed)

original + swap like this PR (with console.warn move)
Original size: 20,081 bytes gzipped (67,456 bytes uncompressed)
Compiled size: 8,959 bytes gzipped (24,402 bytes uncompressed)

Copy link
Copy Markdown
Member

@dead-claudia dead-claudia Feb 8, 2025

Choose a reason for hiding this comment

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

An example of a test case that does not work well with the old mithril code is the flems described in the description at the top of this PR.

Could you add the test case to this PR (if you don't have similar already)?

Copy link
Copy Markdown
Contributor Author

@kfule kfule Feb 8, 2025

Choose a reason for hiding this comment

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

Would a manual test case (like in #3002) be acceptable?
I think the dom mock does not emulate the special case handling of such cases.

Copy link
Copy Markdown
Contributor Author

@kfule kfule Feb 8, 2025

Choose a reason for hiding this comment

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

I added manual test similar to flems.

var val
if (old != null) {
if (old === attrs) {
console.warn("Don't reuse attrs object, use new object for every redraw, this will throw in next major")
}
for (var key in old) {
if (((val = old[key]) != null) && (attrs == null || attrs[key] == null)) {
removeAttr(vnode, key, val, ns)
}
}
}
if (attrs != null) {
for (var key in attrs) {
setAttr(vnode, key, old && old[key], attrs[key], ns)
}
}
}
function isFormAttribute(vnode, attr) {
return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected" && vnode.dom === activeElement(vnode.dom) || vnode.tag === "option" && vnode.dom.parentNode === activeElement(vnode.dom)
Expand All @@ -737,7 +739,7 @@ module.exports = function() {
// Filter out namespaced keys
return ns === undefined && (
// If it's a custom element, just keep it.
vnode.tag.indexOf("-") > -1 || vnode.attrs != null && vnode.attrs.is ||
vnode.tag.indexOf("-") > -1 || vnode.is ||
// If it's a normal element, let's try to avoid a few browser bugs.
key !== "href" && key !== "list" && key !== "form" && key !== "width" && key !== "height"// && key !== "type"
// Defer the property check until *after* we check everything.
Expand All @@ -756,7 +758,7 @@ module.exports = function() {
element.style = style
} else if (old == null || typeof old !== "object") {
// `old` is missing or a string, `style` is an object.
element.style.cssText = ""
element.style = ""
// Add new style properties
for (var key in style) {
var value = style[key]
Expand All @@ -767,6 +769,15 @@ module.exports = function() {
}
} else {
// Both old & new are (different) objects.
// Remove style properties that no longer exist
// Style properties may have two cases(dash-case and camelCase),
// so removal should be done first to prevent accidental removal for newly setting values.
Comment thread
dead-claudia marked this conversation as resolved.
for (var key in old) {
if (old[key] != null && style[key] == null) {
if (key.includes("-")) element.style.removeProperty(key)
else element.style[key] = ""
}
}
// Update style properties that have changed
for (var key in style) {
var value = style[key]
Expand All @@ -775,13 +786,6 @@ module.exports = function() {
else element.style[key] = value
}
}
// Remove style properties that no longer exist
for (var key in old) {
if (old[key] != null && style[key] == null) {
if (key.includes("-")) element.style.removeProperty(key)
else element.style[key] = ""
}
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions render/tests/test-attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ o.spec("attributes", function() {
o(spies[0].callCount).equals(0)
o(spies[2].callCount).equals(0)
o(spies[3].calls).deepEquals([{this: spies[3].elem, args: ["custom", "x"]}])
o(spies[4].calls).deepEquals([{this: spies[4].elem, args: ["custom", "x"]}])
o(spies[5].calls).deepEquals([{this: spies[5].elem, args: ["custom", "x"]}])
o(spies[4].calls).deepEquals([{this: spies[4].elem, args: ["is", "something-special"]}, {this: spies[4].elem, args: ["custom", "x"]}])
o(spies[5].calls).deepEquals([{this: spies[5].elem, args: ["is", "something-special"]}, {this: spies[5].elem, args: ["custom", "x"]}])
})

o("when vnode is customElement with property, custom setAttribute not called", function(){
Expand Down Expand Up @@ -124,8 +124,8 @@ o.spec("attributes", function() {
o(spies[1].callCount).equals(0)
o(spies[2].callCount).equals(0)
o(spies[3].callCount).equals(0)
o(spies[4].callCount).equals(0)
o(spies[5].callCount).equals(0)
o(spies[4].callCount).equals(1) // setAttribute("is", "something-special") is called
o(spies[5].callCount).equals(1) // setAttribute("is", "something-special") is called
o(getters[0].callCount).equals(0)
o(getters[1].callCount).equals(0)
o(getters[2].callCount).equals(0)
Expand Down
174 changes: 174 additions & 0 deletions render/tests/test-updateElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,4 +388,178 @@ o.spec("updateElement", function() {
o(root.childNodes.length).equals(3)
o(x).notEquals(y) // this used to be a recycling pool test
})
o.spec("element node with `is` attribute", function() {
o("recreate element node with `is` attribute (set `is`)", function() {
var vnode = m("a")
var updated = m("a", {is: "bar"})

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("recreate element node without `is` attribute (remove `is`)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("a")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals(null)
})
o("recreate element node with `is` attribute (same tag, different `is`)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("a", {is: "bar"})

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("recreate element node with `is` attribute (different tag, same `is`)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("b", {is: "foo"})

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("B")
o(updated.dom.getAttribute("is")).equals("foo")
})
o("recreate element node with `is` attribute (different tag, different `is`)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("b", {is: "bar"})

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("B")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("keep element node with `is` attribute (same tag, same `is`)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("a", {is: "foo"}, "x")

render(root, vnode)
render(root, updated)

o(vnode.dom).equals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("foo")
o(updated.dom.firstChild.nodeValue).equals("x")
})
o("recreate element node with `is` attribute (set `is`, CSS selector)", function() {
var vnode = m("a")
var updated = m("a[is=bar]")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("recreate element node without `is` attribute (remove `is`, CSS selector)", function() {
var vnode = m("a[is=foo]")
var updated = m("a")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals(null)
})
o("recreate element node with `is` attribute (same tag, different `is`, CSS selector)", function() {
var vnode = m("a[is=foo]")
var updated = m("a[is=bar]")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("recreate element node with `is` attribute (different tag, same `is`, CSS selector)", function() {
var vnode = m("a[is=foo]")
var updated = m("b[is=foo]")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("B")
o(updated.dom.getAttribute("is")).equals("foo")
})
o("recreate element node with `is` attribute (different tag, different `is`, CSS selector)", function() {
var vnode = m("a[is=foo]")
var updated = m("b[is=bar]")

render(root, vnode)
render(root, updated)

o(vnode.dom).notEquals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("B")
o(updated.dom.getAttribute("is")).equals("bar")
})
o("keep element node with `is` attribute (same tag, same `is`, CSS selector)", function() {
var vnode = m("a[is=foo]")
var updated = m("a[is=foo]", "x")

render(root, vnode)
render(root, updated)

o(vnode.dom).equals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("foo")
o(updated.dom.firstChild.nodeValue).equals("x")
})
o("keep element node with `is` attribute (same tag, same `is`, from attrs to CSS selector)", function() {
var vnode = m("a", {is: "foo"})
var updated = m("a[is=foo]", "x")

render(root, vnode)
render(root, updated)

o(vnode.dom).equals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("foo")
o(updated.dom.firstChild.nodeValue).equals("x")
})
o("keep element node with `is` attribute (same tag, same `is`, from CSS selector to attrs)", function() {
var vnode = m("a[is=foo]")
var updated = m("a", {is: "foo"}, "x")

render(root, vnode)
render(root, updated)

o(vnode.dom).equals(root.firstChild)
o(updated.dom).equals(root.firstChild)
o(updated.dom.nodeName).equals("A")
o(updated.dom.getAttribute("is")).equals("foo")
o(updated.dom.firstChild.nodeValue).equals("x")
})
})
})
2 changes: 1 addition & 1 deletion render/vnode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use strict"

function Vnode(tag, key, attrs, children, text, dom) {
return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, domSize: undefined, state: undefined, events: undefined, instance: undefined}
return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, is: undefined, domSize: undefined, state: undefined, events: undefined, instance: undefined}
}
Vnode.normalize = function(node) {
if (Array.isArray(node)) return Vnode("[", undefined, undefined, Vnode.normalizeChildren(node), undefined, undefined)
Expand Down