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.

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
merged 12 commits into from
Feb 8, 2025
3 changes: 3 additions & 0 deletions render/hyperscript.js
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.
vnode.is = attrs.is

vnode.attrs = attrs

return vnode
Expand Down
46 changes: 25 additions & 21 deletions render/render.js
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
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
Member

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
Contributor Author

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
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 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
Member

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
Contributor Author

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
Contributor Author

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.
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
40 changes: 40 additions & 0 deletions render/tests/manual/case-handling.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
</head>
<body>
<p>This is a test for special case-handling of attribute and style properties. (#2988).</p>
<p>Open your browser's Developer Console and follow these steps:</p>
<ol>
<li>Check the background color of the "foo" below.</li>
<ul>
<li>If it is light green, it is correct. The style has been updated properly.</li>
<li>If it is red or yellow, the style has not been updated properly.</li>
</ul>
<li>Check the logs displayed in the console.</li>
<ul>
<li>If the attribute has been updated correctly, you should see the following message: "If you see this message, the update process is correct."</li>
<li>If "null" is displayed, the attribute has not been updated properly.</li>
</ul>
</ol>

<div id="root" style="background-color: red;"></div>
<script src="../../../mithril.js"></script>
<script>
// data-*** is NOT case-sensitive
// style properties have two cases (camelCase and dash-case)
var a = m("div#a", {"data-sampleId": "If you see this message, something is wrong.", style: {backgroundColor: "yellow"}}, "foo")
var b = m("div#a", {"data-sampleid": "If you see this message, the update process is correct.", style: {"background-color": "lightgreen"}}, "foo")

// background color is yellow
m.render(document.getElementById("root"), a)

// background color is lightgreen?
m.render(document.getElementById("root"), b)

// data-sampleid is "If you see this message, the update process is correct."?
console.log(document.querySelector("#a").getAttribute("data-sampleid"))
</script>
</body>
</html>
8 changes: 4 additions & 4 deletions render/tests/test-attributes.js
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
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
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