-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
Changes from all commits
e4b4791
1e8850d
f944a7e
e8534b0
0ce4634
6e9e11b
ae6e60f
c290f0a
9d4068b
473ee08
65f0066
c72556c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -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) | ||
|
|
@@ -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 ( | ||
|
|
@@ -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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what the inner 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 + swap (without console.warn move) original + swap like this PR (with console.warn move)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you add the test case to this PR (if you don't have similar already)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would a manual test case (like in #3002) be acceptable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
|
@@ -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. | ||
|
|
@@ -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] | ||
|
|
@@ -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. | ||
dead-claudia marked this conversation as resolved.
Show resolved
Hide 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] | ||
|
|
@@ -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] = "" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| 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> |
Uh oh!
There was an error while loading. Please reload this page.