Skip to content

Commit 2f56c8e

Browse files
kfuledead-claudia
kfule
and
dead-claudia
authored
Improve handling of is-elements and Fix tiny bugs of setAttr()/updateStyle() (#2988)
* setAttr/removeAttr: remove `key === "is"`, fixes #2799 This allows the "is" attribute to be set or removed like any other attribute. * swap set and removal order of attributes/style properties * bypass css text This is a leftover from #2811. * Vnode: add vnode.is to handle is-element This seems to give better performance than simply adding a `(old.attrs && old.attrs.is) == (vnode.attrs && vnode.attrs.is)` evaluation to updateNode(). * refactor isEmpty() * move `old.is === vnode.is` evaluation to the top of updateNode() It seems better to evaluate `old.is === vnode.is` before process that assumes node update. * revert isEmpty() back * hasPropertyKey: evaluate `vnode.is` instead of `vnode.attrs.is` * execSelector: use `attrs.is != null` instead of `hasOwn.call(attrs, "is")` * Vnode: initialize `is` with undefined * add manual test * Update hyperscript.js Signed-off-by: Claudia Meadows <[email protected]> --------- Signed-off-by: Claudia Meadows <[email protected]> Co-authored-by: Claudia Meadows <[email protected]>
1 parent f5fc394 commit 2f56c8e

File tree

6 files changed

+247
-26
lines changed

6 files changed

+247
-26
lines changed

‎render/hyperscript.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ function execSelector(state, vnode) {
6262
attrs = Object.assign({type: attrs.type}, attrs)
6363
}
6464

65+
// This reduces the complexity of the evaluation of "is" within the render function.
66+
vnode.is = attrs.is
67+
6568
vnode.attrs = attrs
6669

6770
return vnode

‎render/render.js

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ module.exports = function() {
114114
function createElement(parent, vnode, hooks, ns, nextSibling) {
115115
var tag = vnode.tag
116116
var attrs = vnode.attrs
117-
var is = attrs && attrs.is
117+
var is = vnode.is
118118

119119
ns = getNameSpace(vnode) || ns
120120

@@ -396,7 +396,7 @@ module.exports = function() {
396396
}
397397
function updateNode(parent, old, vnode, hooks, nextSibling, ns) {
398398
var oldTag = old.tag, tag = vnode.tag
399-
if (oldTag === tag) {
399+
if (oldTag === tag && old.is === vnode.is) {
400400
vnode.state = old.state
401401
vnode.events = old.events
402402
if (shouldNotUpdate(vnode, old)) return
@@ -643,7 +643,7 @@ module.exports = function() {
643643
}
644644
}
645645
function setAttr(vnode, key, old, value, ns) {
646-
if (key === "key" || key === "is" || value == null || isLifecycleMethod(key) || (old === value && !isFormAttribute(vnode, key)) && typeof value !== "object") return
646+
if (key === "key" || value == null || isLifecycleMethod(key) || (old === value && !isFormAttribute(vnode, key)) && typeof value !== "object") return
647647
if (key[0] === "o" && key[1] === "n") return updateEvent(vnode, key, value)
648648
if (key.slice(0, 6) === "xlink:") vnode.dom.setAttributeNS("http://www.w3.org/1999/xlink", key.slice(6), value)
649649
else if (key === "style") updateStyle(vnode.dom, old, value)
@@ -676,7 +676,7 @@ module.exports = function() {
676676
}
677677
}
678678
function removeAttr(vnode, key, old, ns) {
679-
if (key === "key" || key === "is" || old == null || isLifecycleMethod(key)) return
679+
if (key === "key" || old == null || isLifecycleMethod(key)) return
680680
if (key[0] === "o" && key[1] === "n") updateEvent(vnode, key, undefined)
681681
else if (key === "style") updateStyle(vnode.dom, old, null)
682682
else if (
@@ -710,22 +710,24 @@ module.exports = function() {
710710
if ("selectedIndex" in attrs) setAttr(vnode, "selectedIndex", null, attrs.selectedIndex, undefined)
711711
}
712712
function updateAttrs(vnode, old, attrs, ns) {
713-
if (old && old === attrs) {
714-
console.warn("Don't reuse attrs object, use new object for every redraw, this will throw in next major")
715-
}
716-
if (attrs != null) {
717-
for (var key in attrs) {
718-
setAttr(vnode, key, old && old[key], attrs[key], ns)
719-
}
720-
}
713+
// Some attributes may NOT be case-sensitive (e.g. data-***),
714+
// so removal should be done first to prevent accidental removal for newly setting values.
721715
var val
722716
if (old != null) {
717+
if (old === attrs) {
718+
console.warn("Don't reuse attrs object, use new object for every redraw, this will throw in next major")
719+
}
723720
for (var key in old) {
724721
if (((val = old[key]) != null) && (attrs == null || attrs[key] == null)) {
725722
removeAttr(vnode, key, val, ns)
726723
}
727724
}
728725
}
726+
if (attrs != null) {
727+
for (var key in attrs) {
728+
setAttr(vnode, key, old && old[key], attrs[key], ns)
729+
}
730+
}
729731
}
730732
function isFormAttribute(vnode, attr) {
731733
return attr === "value" || attr === "checked" || attr === "selectedIndex" || attr === "selected" && vnode.dom === activeElement(vnode.dom) || vnode.tag === "option" && vnode.dom.parentNode === activeElement(vnode.dom)
@@ -737,7 +739,7 @@ module.exports = function() {
737739
// Filter out namespaced keys
738740
return ns === undefined && (
739741
// If it's a custom element, just keep it.
740-
vnode.tag.indexOf("-") > -1 || vnode.attrs != null && vnode.attrs.is ||
742+
vnode.tag.indexOf("-") > -1 || vnode.is ||
741743
// If it's a normal element, let's try to avoid a few browser bugs.
742744
key !== "href" && key !== "list" && key !== "form" && key !== "width" && key !== "height"// && key !== "type"
743745
// Defer the property check until *after* we check everything.
@@ -756,7 +758,7 @@ module.exports = function() {
756758
element.style = style
757759
} else if (old == null || typeof old !== "object") {
758760
// `old` is missing or a string, `style` is an object.
759-
element.style.cssText = ""
761+
element.style = ""
760762
// Add new style properties
761763
for (var key in style) {
762764
var value = style[key]
@@ -767,6 +769,15 @@ module.exports = function() {
767769
}
768770
} else {
769771
// Both old & new are (different) objects.
772+
// Remove style properties that no longer exist
773+
// Style properties may have two cases(dash-case and camelCase),
774+
// so removal should be done first to prevent accidental removal for newly setting values.
775+
for (var key in old) {
776+
if (old[key] != null && style[key] == null) {
777+
if (key.includes("-")) element.style.removeProperty(key)
778+
else element.style[key] = ""
779+
}
780+
}
770781
// Update style properties that have changed
771782
for (var key in style) {
772783
var value = style[key]
@@ -775,13 +786,6 @@ module.exports = function() {
775786
else element.style[key] = value
776787
}
777788
}
778-
// Remove style properties that no longer exist
779-
for (var key in old) {
780-
if (old[key] != null && style[key] == null) {
781-
if (key.includes("-")) element.style.removeProperty(key)
782-
else element.style[key] = ""
783-
}
784-
}
785789
}
786790
}
787791

‎render/tests/manual/case-handling.html

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8">
5+
</head>
6+
<body>
7+
<p>This is a test for special case-handling of attribute and style properties. (#2988).</p>
8+
<p>Open your browser's Developer Console and follow these steps:</p>
9+
<ol>
10+
<li>Check the background color of the "foo" below.</li>
11+
<ul>
12+
<li>If it is light green, it is correct. The style has been updated properly.</li>
13+
<li>If it is red or yellow, the style has not been updated properly.</li>
14+
</ul>
15+
<li>Check the logs displayed in the console.</li>
16+
<ul>
17+
<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>
18+
<li>If "null" is displayed, the attribute has not been updated properly.</li>
19+
</ul>
20+
</ol>
21+
22+
<div id="root" style="background-color: red;"></div>
23+
<script src="../../../mithril.js"></script>
24+
<script>
25+
// data-*** is NOT case-sensitive
26+
// style properties have two cases (camelCase and dash-case)
27+
var a = m("div#a", {"data-sampleId": "If you see this message, something is wrong.", style: {backgroundColor: "yellow"}}, "foo")
28+
var b = m("div#a", {"data-sampleid": "If you see this message, the update process is correct.", style: {"background-color": "lightgreen"}}, "foo")
29+
30+
// background color is yellow
31+
m.render(document.getElementById("root"), a)
32+
33+
// background color is lightgreen?
34+
m.render(document.getElementById("root"), b)
35+
36+
// data-sampleid is "If you see this message, the update process is correct."?
37+
console.log(document.querySelector("#a").getAttribute("data-sampleid"))
38+
</script>
39+
</body>
40+
</html>

‎render/tests/test-attributes.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ o.spec("attributes", function() {
8080
o(spies[0].callCount).equals(0)
8181
o(spies[2].callCount).equals(0)
8282
o(spies[3].calls).deepEquals([{this: spies[3].elem, args: ["custom", "x"]}])
83-
o(spies[4].calls).deepEquals([{this: spies[4].elem, args: ["custom", "x"]}])
84-
o(spies[5].calls).deepEquals([{this: spies[5].elem, args: ["custom", "x"]}])
83+
o(spies[4].calls).deepEquals([{this: spies[4].elem, args: ["is", "something-special"]}, {this: spies[4].elem, args: ["custom", "x"]}])
84+
o(spies[5].calls).deepEquals([{this: spies[5].elem, args: ["is", "something-special"]}, {this: spies[5].elem, args: ["custom", "x"]}])
8585
})
8686

8787
o("when vnode is customElement with property, custom setAttribute not called", function(){
@@ -124,8 +124,8 @@ o.spec("attributes", function() {
124124
o(spies[1].callCount).equals(0)
125125
o(spies[2].callCount).equals(0)
126126
o(spies[3].callCount).equals(0)
127-
o(spies[4].callCount).equals(0)
128-
o(spies[5].callCount).equals(0)
127+
o(spies[4].callCount).equals(1) // setAttribute("is", "something-special") is called
128+
o(spies[5].callCount).equals(1) // setAttribute("is", "something-special") is called
129129
o(getters[0].callCount).equals(0)
130130
o(getters[1].callCount).equals(0)
131131
o(getters[2].callCount).equals(0)

‎render/tests/test-updateElement.js

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,4 +388,178 @@ o.spec("updateElement", function() {
388388
o(root.childNodes.length).equals(3)
389389
o(x).notEquals(y) // this used to be a recycling pool test
390390
})
391+
o.spec("element node with `is` attribute", function() {
392+
o("recreate element node with `is` attribute (set `is`)", function() {
393+
var vnode = m("a")
394+
var updated = m("a", {is: "bar"})
395+
396+
render(root, vnode)
397+
render(root, updated)
398+
399+
o(vnode.dom).notEquals(root.firstChild)
400+
o(updated.dom).equals(root.firstChild)
401+
o(updated.dom.nodeName).equals("A")
402+
o(updated.dom.getAttribute("is")).equals("bar")
403+
})
404+
o("recreate element node without `is` attribute (remove `is`)", function() {
405+
var vnode = m("a", {is: "foo"})
406+
var updated = m("a")
407+
408+
render(root, vnode)
409+
render(root, updated)
410+
411+
o(vnode.dom).notEquals(root.firstChild)
412+
o(updated.dom).equals(root.firstChild)
413+
o(updated.dom.nodeName).equals("A")
414+
o(updated.dom.getAttribute("is")).equals(null)
415+
})
416+
o("recreate element node with `is` attribute (same tag, different `is`)", function() {
417+
var vnode = m("a", {is: "foo"})
418+
var updated = m("a", {is: "bar"})
419+
420+
render(root, vnode)
421+
render(root, updated)
422+
423+
o(vnode.dom).notEquals(root.firstChild)
424+
o(updated.dom).equals(root.firstChild)
425+
o(updated.dom.nodeName).equals("A")
426+
o(updated.dom.getAttribute("is")).equals("bar")
427+
})
428+
o("recreate element node with `is` attribute (different tag, same `is`)", function() {
429+
var vnode = m("a", {is: "foo"})
430+
var updated = m("b", {is: "foo"})
431+
432+
render(root, vnode)
433+
render(root, updated)
434+
435+
o(vnode.dom).notEquals(root.firstChild)
436+
o(updated.dom).equals(root.firstChild)
437+
o(updated.dom.nodeName).equals("B")
438+
o(updated.dom.getAttribute("is")).equals("foo")
439+
})
440+
o("recreate element node with `is` attribute (different tag, different `is`)", function() {
441+
var vnode = m("a", {is: "foo"})
442+
var updated = m("b", {is: "bar"})
443+
444+
render(root, vnode)
445+
render(root, updated)
446+
447+
o(vnode.dom).notEquals(root.firstChild)
448+
o(updated.dom).equals(root.firstChild)
449+
o(updated.dom.nodeName).equals("B")
450+
o(updated.dom.getAttribute("is")).equals("bar")
451+
})
452+
o("keep element node with `is` attribute (same tag, same `is`)", function() {
453+
var vnode = m("a", {is: "foo"})
454+
var updated = m("a", {is: "foo"}, "x")
455+
456+
render(root, vnode)
457+
render(root, updated)
458+
459+
o(vnode.dom).equals(root.firstChild)
460+
o(updated.dom).equals(root.firstChild)
461+
o(updated.dom.nodeName).equals("A")
462+
o(updated.dom.getAttribute("is")).equals("foo")
463+
o(updated.dom.firstChild.nodeValue).equals("x")
464+
})
465+
o("recreate element node with `is` attribute (set `is`, CSS selector)", function() {
466+
var vnode = m("a")
467+
var updated = m("a[is=bar]")
468+
469+
render(root, vnode)
470+
render(root, updated)
471+
472+
o(vnode.dom).notEquals(root.firstChild)
473+
o(updated.dom).equals(root.firstChild)
474+
o(updated.dom.nodeName).equals("A")
475+
o(updated.dom.getAttribute("is")).equals("bar")
476+
})
477+
o("recreate element node without `is` attribute (remove `is`, CSS selector)", function() {
478+
var vnode = m("a[is=foo]")
479+
var updated = m("a")
480+
481+
render(root, vnode)
482+
render(root, updated)
483+
484+
o(vnode.dom).notEquals(root.firstChild)
485+
o(updated.dom).equals(root.firstChild)
486+
o(updated.dom.nodeName).equals("A")
487+
o(updated.dom.getAttribute("is")).equals(null)
488+
})
489+
o("recreate element node with `is` attribute (same tag, different `is`, CSS selector)", function() {
490+
var vnode = m("a[is=foo]")
491+
var updated = m("a[is=bar]")
492+
493+
render(root, vnode)
494+
render(root, updated)
495+
496+
o(vnode.dom).notEquals(root.firstChild)
497+
o(updated.dom).equals(root.firstChild)
498+
o(updated.dom.nodeName).equals("A")
499+
o(updated.dom.getAttribute("is")).equals("bar")
500+
})
501+
o("recreate element node with `is` attribute (different tag, same `is`, CSS selector)", function() {
502+
var vnode = m("a[is=foo]")
503+
var updated = m("b[is=foo]")
504+
505+
render(root, vnode)
506+
render(root, updated)
507+
508+
o(vnode.dom).notEquals(root.firstChild)
509+
o(updated.dom).equals(root.firstChild)
510+
o(updated.dom.nodeName).equals("B")
511+
o(updated.dom.getAttribute("is")).equals("foo")
512+
})
513+
o("recreate element node with `is` attribute (different tag, different `is`, CSS selector)", function() {
514+
var vnode = m("a[is=foo]")
515+
var updated = m("b[is=bar]")
516+
517+
render(root, vnode)
518+
render(root, updated)
519+
520+
o(vnode.dom).notEquals(root.firstChild)
521+
o(updated.dom).equals(root.firstChild)
522+
o(updated.dom.nodeName).equals("B")
523+
o(updated.dom.getAttribute("is")).equals("bar")
524+
})
525+
o("keep element node with `is` attribute (same tag, same `is`, CSS selector)", function() {
526+
var vnode = m("a[is=foo]")
527+
var updated = m("a[is=foo]", "x")
528+
529+
render(root, vnode)
530+
render(root, updated)
531+
532+
o(vnode.dom).equals(root.firstChild)
533+
o(updated.dom).equals(root.firstChild)
534+
o(updated.dom.nodeName).equals("A")
535+
o(updated.dom.getAttribute("is")).equals("foo")
536+
o(updated.dom.firstChild.nodeValue).equals("x")
537+
})
538+
o("keep element node with `is` attribute (same tag, same `is`, from attrs to CSS selector)", function() {
539+
var vnode = m("a", {is: "foo"})
540+
var updated = m("a[is=foo]", "x")
541+
542+
render(root, vnode)
543+
render(root, updated)
544+
545+
o(vnode.dom).equals(root.firstChild)
546+
o(updated.dom).equals(root.firstChild)
547+
o(updated.dom.nodeName).equals("A")
548+
o(updated.dom.getAttribute("is")).equals("foo")
549+
o(updated.dom.firstChild.nodeValue).equals("x")
550+
})
551+
o("keep element node with `is` attribute (same tag, same `is`, from CSS selector to attrs)", function() {
552+
var vnode = m("a[is=foo]")
553+
var updated = m("a", {is: "foo"}, "x")
554+
555+
render(root, vnode)
556+
render(root, updated)
557+
558+
o(vnode.dom).equals(root.firstChild)
559+
o(updated.dom).equals(root.firstChild)
560+
o(updated.dom.nodeName).equals("A")
561+
o(updated.dom.getAttribute("is")).equals("foo")
562+
o(updated.dom.firstChild.nodeValue).equals("x")
563+
})
564+
})
391565
})

‎render/vnode.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"use strict"
22

33
function Vnode(tag, key, attrs, children, text, dom) {
4-
return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, domSize: undefined, state: undefined, events: undefined, instance: undefined}
4+
return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, is: undefined, domSize: undefined, state: undefined, events: undefined, instance: undefined}
55
}
66
Vnode.normalize = function(node) {
77
if (Array.isArray(node)) return Vnode("[", undefined, undefined, Vnode.normalizeChildren(node), undefined, undefined)

0 commit comments

Comments
 (0)