From 1ecc30a0641d31c9b5f38227270c8f4b9b1cf54d Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Fri, 31 Aug 2018 22:54:21 -0400 Subject: [PATCH] Fix custom elements attribute application, improve key checking - Fix custom elements attribute application to acknowledge that not all custom elements operate purely based on attributes. (Plus, those blasted things are verbose as heck when you're working with them in raw form. It's also not that uncommon for functionality to be exposed via property and *not* attribute.) - Don't memoize the normalized value when we 1. only use it once in each branch, and 2. only use it for a few special cases. - Centralize the "has property key" code, so it's easier to tune and read. I also inlined a couple functions while I was at it since they were small and only used once. - Actually test for how attributes are applied to raw DOM elements vs when we choose to use keys. When I first developed the patch, it silently worked, when I should've been breaking things. --- docs/change-log.md | 1 + docs/hyperscript.md | 72 ++++++++++++++++++++++++- ospec/change-log.md | 6 +-- render/render.js | 30 ++++++----- render/tests/test-attributes.js | 92 ++++++++++++++++++++++++-------- render/tests/test-hyperscript.js | 57 ++++++++++++++++++++ 6 files changed, 217 insertions(+), 41 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index 9ec18373..b4c750b6 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -28,6 +28,7 @@ - hyperscript: when an attribute is defined on both the first and second argument (as a CSS selector and an `attrs` field, respectively), the latter takes precedence, except for `class` attributes that are still added together. [#2172](https://github.com/MithrilJS/mithril.js/issues/2172) ([#2174](https://github.com/MithrilJS/mithril.js/pull/2174)) - stream: when a stream conditionally returns HALT, dependant stream will also end ([#2200](https://github.com/MithrilJS/mithril.js/pull/2200)) - render: remove some redundancy within the component initialization code ([#2213](https://github.com/MithrilJS/mithril.js/pull/2213)) +- render: Align custom elements to work like normal elements, minus all the HTML-specific magic. ([#2221](https://github.com/MithrilJS/mithril.js/pull/2221)) #### News diff --git a/docs/hyperscript.md b/docs/hyperscript.md index 26b0a27d..d0ac2767 100644 --- a/docs/hyperscript.md +++ b/docs/hyperscript.md @@ -178,6 +178,76 @@ m("input[readonly]") m("input[readOnly]") ``` +This even includes custom elements. For example, you can use [A-Frame](https://aframe.io/docs/0.8.0/introduction/) within Mithril, no problem! + +```javascript +m("a-scene", [ + m("a-box", { + position: "-1 0.5 -3", + rotation: "0 45 0", + color: "#4CC3D9", + }), + + m("a-sphere", { + position: "0 1.25 -5", + radius: "1.25", + color: "#EF2D5E", + }), + + m("a-cylinder", { + position: "1 0.75 -3", + radius: "0.5", + height: "1.5", + color: "#FFC65D", + }), + + m("a-plane", { + position: "0 0 -4", + rotation: "-90 0 0", + width: "4", + height: "4", + color: "#7BC8A4", + }), + + m("a-sky", { + color: "#ECECEC", + }), +]) +``` + +And yes, this translates to both attributes and properties, and it works just like they would in the DOM. Using [Brick's `brick-deck`](http://brick.mozilla.io/docs/brick-deck) as an example, they have a `selected-index` attribute with a corresponding `selectedIndex` getter/setter property. + +```javascript +m("brick-deck[selected-index=0]", [/* ... */]) // lowercase +m("brick-deck[selectedIndex=0]", [/* ... */]) // uppercase +// I know these look odd, but `brick-deck`'s `selectedIndex` property is a +// string, not a number. +m("brick-deck", {"selected-index": "0"}, [/* ... */]) +m("brick-deck", {"selectedIndex": "0"}, [/* ... */]) +``` + +For custom elements, it doesn't auto-stringify properties, in case they are objects, numbers, or some other non-string value. So assuming you had some custom element `my-special-element` that has an `elem.whitelist` array getter/setter property, you could do this, and it'd work as you'd expect: + +```javascript +m("my-special-element", { + whitelist: [ + "https://example.com", + "http://neverssl.com", + "https://google.com", + ], +}) +``` + +If you have classes or IDs for those elements, the shorthands still work as you would expect. To pull another A-Frame example: + +```javascript +// These two are equivalent +m("a-entity#player") +m("a-entity", {id: "player"}) +``` + +Do note that all the properties with magic semantics, like lifecycle attributes, `onevent` handlers, `key`s, `class`, and `style`, those are still treated the same way they are for normal HTML elements. + --- ### Style attribute @@ -192,7 +262,7 @@ m("div[style=background:red]") Using a string as a `style` would overwrite all inline styles in the element if it is redrawn, and not only CSS rules whose values have changed. -Mithril does not attempt to add units to number values. +Mithril does not attempt to add units to number values. It simply stringifies them. --- diff --git a/ospec/change-log.md b/ospec/change-log.md index 58763d96..04884e99 100644 --- a/ospec/change-log.md +++ b/ospec/change-log.md @@ -4,6 +4,7 @@ ## Upcoming... _2018-xx-yy_ +- Add `spy.calls` array property to get the `this` and `arguments` values for any arbitrary call. ## 3.0.1 _2018-06-30_ @@ -70,10 +71,7 @@ _2017-12-01_ -## 1.3 and earlier +## 1.3 and earlier - Log using util.inspect to show object content instead of "[object Object]" ([#1661](https://github.com/MithrilJS/mithril.js/issues/1661), [@porsager](https://github.com/porsager)) - Shell command: Ignore hidden directories and files ([#1855](https://github.com/MithrilJS/mithril.js/pull/1855) [@pdfernhout)](https://github.com/pdfernhout)) - Library: Add the possibility to name new test suites ([#1529](https://github.com/MithrilJS/mithril.js/pull/1529)) - - - diff --git a/render/render.js b/render/render.js index 552d3194..c05cfe1b 100644 --- a/render/render.js +++ b/render/render.js @@ -686,15 +686,17 @@ module.exports = function($window) { 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) - else if (key in vnode.dom && !isAttribute(key) && ns === undefined && !isCustomElement(vnode.tag, vnode.attrs)) { + else if (hasPropertyKey(vnode, key, ns)) { if (key === "value") { - var normalized = "" + value // eslint-disable-line no-implicit-coercion + // Only do the coercion if we're actually going to check the value. + /* eslint-disable no-implicit-coercion */ //setting input[value] to same value by typing on focused element moves cursor to end in Chrome - if ((vnode.tag === "input" || vnode.tag === "textarea") && vnode.dom.value === normalized && vnode.dom === $doc.activeElement) return + if ((vnode.tag === "input" || vnode.tag === "textarea") && vnode.dom.value === "" + value && vnode.dom === $doc.activeElement) return //setting select[value] to same value while having select open blinks select dropdown in Chrome - if (vnode.tag === "select" && old !== null && vnode.dom.value === normalized) return + if (vnode.tag === "select" && old !== null && vnode.dom.value === "" + value) return //setting option[value] to same value while having select open blinks select dropdown in Chrome - if (vnode.tag === "option" && old !== null && vnode.dom.value === normalized) return + if (vnode.tag === "option" && old !== null && vnode.dom.value === "" + value) return + /* eslint-enable no-implicit-coercion */ } // If you assign an input type that is not supported by IE 11 with an assignment expression, an error will occur. if (vnode.tag === "input" && key === "type") vnode.dom.setAttribute(key, value) @@ -712,12 +714,10 @@ module.exports = function($window) { if (key[0] === "o" && key[1] === "n" && !isLifecycleMethod(key)) updateEvent(vnode, key, undefined) else if (key === "style") updateStyle(vnode.dom, old, null) else if ( - key in vnode.dom && !isAttribute(key) + hasPropertyKey(vnode, key, ns) && key !== "className" && !(vnode.tag === "option" && key === "value") && !(vnode.tag === "input" && key === "type") - && ns === undefined - && !isCustomElement(vnode.tag, vnode.attrs || {}) ) { vnode.dom[key] = null } else { @@ -760,11 +760,15 @@ module.exports = function($window) { function isLifecycleMethod(attr) { return attr === "oninit" || attr === "oncreate" || attr === "onupdate" || attr === "onremove" || attr === "onbeforeremove" || attr === "onbeforeupdate" } - function isAttribute(attr) { - return attr === "href" || attr === "list" || attr === "form" || attr === "width" || attr === "height"// || attr === "type" - } - function isCustomElement(tag, attrs){ - return attrs.is || tag.indexOf("-") > -1 + function hasPropertyKey(vnode, key, ns) { + // 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 || + // 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. + ) && key in vnode.dom } //style diff --git a/render/tests/test-attributes.js b/render/tests/test-attributes.js index 00b91f1d..9b6f2ba3 100644 --- a/render/tests/test-attributes.js +++ b/render/tests/test-attributes.js @@ -52,38 +52,84 @@ o.spec("attributes", function() { }) o.spec("customElements", function(){ - o("when vnode is customElement, custom setAttribute called", function(){ - - var normal = [ - {tag: "input", attrs: {value: "hello"}}, - {tag: "input", attrs: {value: "hello"}}, - {tag: "input", attrs: {value: "hello"}} - ] - - var custom = [ - {tag: "custom-element", attrs: {custom: "x"}}, - {tag: "input", attrs: {is: "something-special", custom: "x"}}, - {tag: "custom-element", attrs: {is: "something-special", custom: "x"}} - ] - - var view = normal.concat(custom) - + o("when vnode is customElement without property, custom setAttribute called", function(){ var f = $window.document.createElement - var spy + var spies = [] $window.document.createElement = function(tag, is){ var el = f(tag, is) - if(!spy){ - spy = o.spy(el.setAttribute) - } + var spy = o.spy(el.setAttribute) el.setAttribute = spy - + spies.push(spy) + spy.elem = el return el } - render(root, view) + render(root, [ + {tag: "input", attrs: {value: "hello"}}, + {tag: "input", attrs: {value: "hello"}}, + {tag: "input", attrs: {value: "hello"}}, + {tag: "custom-element", attrs: {custom: "x"}}, + {tag: "input", attrs: {is: "something-special", custom: "x"}}, + {tag: "custom-element", attrs: {is: "something-special", custom: "x"}} + ]) - o(spy.callCount).equals(custom.length) + o(spies[0].callCount).equals(0) + o(spies[1].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("when vnode is customElement with property, custom setAttribute not called", function(){ + var f = $window.document.createElement + var spies = [] + var getters = [] + var setters = [] + + $window.document.createElement = function(tag, is){ + var el = f(tag, is) + var spy = o.spy(el.setAttribute) + el.setAttribute = spy + spies.push(spy) + spy.elem = el + if (tag === "custom-element" || is && is.is === "something-special") { + var custom = "foo" + var getter, setter + Object.defineProperty(el, "custom", { + configurable: true, + enumerable: true, + get: getter = o.spy(function () { return custom }), + set: setter = o.spy(function (value) { custom = value }) + }) + getters.push(getter) + setters.push(setter) + } + return el + } + + render(root, [ + {tag: "input", attrs: {value: "hello"}}, + {tag: "input", attrs: {value: "hello"}}, + {tag: "input", attrs: {value: "hello"}}, + {tag: "custom-element", attrs: {custom: "x"}}, + {tag: "input", attrs: {is: "something-special", custom: "x"}}, + {tag: "custom-element", attrs: {is: "something-special", custom: "x"}} + ]) + + o(spies[0].callCount).equals(0) + 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(getters[0].callCount).equals(0) + o(getters[1].callCount).equals(0) + o(getters[2].callCount).equals(0) + o(setters[0].calls).deepEquals([{this: spies[3].elem, args: ["x"]}]) + o(setters[1].calls).deepEquals([{this: spies[4].elem, args: ["x"]}]) + o(setters[2].calls).deepEquals([{this: spies[5].elem, args: ["x"]}]) }) }) diff --git a/render/tests/test-hyperscript.js b/render/tests/test-hyperscript.js index 21924dee..bb4a0f62 100644 --- a/render/tests/test-hyperscript.js +++ b/render/tests/test-hyperscript.js @@ -302,6 +302,63 @@ o.spec("hyperscript", function() { o(vnode.attrs.className).equals("a b") }) }) + o.spec("custom element attrs", function() { + o("handles string attr", function() { + var vnode = m("custom-element", {a: "b"}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals("b") + }) + o("handles falsy string attr", function() { + var vnode = m("custom-element", {a: ""}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals("") + }) + o("handles number attr", function() { + var vnode = m("custom-element", {a: 1}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals(1) + }) + o("handles falsy number attr", function() { + var vnode = m("custom-element", {a: 0}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals(0) + }) + o("handles boolean attr", function() { + var vnode = m("custom-element", {a: true}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals(true) + }) + o("handles falsy boolean attr", function() { + var vnode = m("custom-element", {a: false}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals(false) + }) + o("handles only key in attrs", function() { + var vnode = m("custom-element", {key:"a"}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs).equals(null) + o(vnode.key).equals("a") + }) + o("handles many attrs", function() { + var vnode = m("custom-element", {a: "b", c: "d"}) + + o(vnode.tag).equals("custom-element") + o(vnode.attrs.a).equals("b") + o(vnode.attrs.c).equals("d") + }) + o("handles className attrs property", function() { + var vnode = m("custom-element", {className: "a"}) + + o(vnode.attrs.className).equals("a") + }) + }) o.spec("children", function() { o("handles string single child", function() { var vnode = m("div", {}, ["a"])