Open krisdages opened 6 years ago
3cp
commented
6 years ago From my understanding, I don't think your css.bind is a correct Aurelia usage.
We use
css="width: ${width}px; height: ${height}px;"to do string interpolation,
use
<div style.bind="styleString"></div>
<div style.bind="styleObject"></div>to do binding.
krisdages
commented
6 years ago Thanks for the tip... I had been using css.bind for a long time because I thought it was equivalent and avoided some browser-specific quirk with the word "style".
I tried modifying my demo gist to use style.bind to the object, and the issue still occurs identically.
3cp
commented
6 years ago Instead of using {color: undefined}, use a proper css value like {color: 'inherit'} or {color: 'unset'} can clean up the old value. That can normalise your problem on browsers.
krisdages
commented
6 years ago I'm not really worried about being able to work around the problem, I've already implemented something similar to fix my specific issue...
I'm just concerned that there is a difference in behavior (that violates the principle of least astonishment) between changing the bound object to one that lacks the property entirely and one that has the property set to undefined. The property is removed as expected when binding to an object without the property, but left as-is when the object has the property set to undefined.
And it can make it tough to leverage type checking with TypeScript. A typescript interface for my style object where the properties are optional can't guarantee that I don't set the properties to undefined. But the behavior would be different.
interface MyStyleProperties {
color?: string;
borderColor?: string;
... others ...
}is equivalent to
interface MyStyleProperties {
color?: string | undefined;
borderColor?: string | undefined;
... others ...
}BTW, the code that this affected has been working properly with every version of Aurelia for over a year... I just want it to keep working the same for everyone :) I'm pretty sure it was a change in the browser behavior.
The behavior in Chrome now doesn't match the description in the docs on MDN that says an unprovided value should be treated as the empty string: https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty#Parameters
3cp
commented
6 years ago I understand your point. My argument is that undefined is not a proper css value, all css values should be string. I think it's fragile to rely on Aurelia to delete the key with undefined value, since Aurelia never had official document to support that behaviour.
krisdages
commented
6 years ago True, but if you bind the style property to an object, don't you expect the style properties to reflect the object (and only the object, unless you are mucking around doing manual changes outside of your binding)?
If I bind style to { color: "green"; }, then change it to { border: "1px red solid" }, I'd expect the color to be unset and the border to be set, and vice versa.
And I think it'd be more intuitive to do styleObject.border = undefined (which used to work) to remove the border rather than have to delete styleObject.border, which wouldn't even work if the property's value was inherited for some reason.
krisdages
commented
6 years ago I agree about undocumented behavior though; but maybe the more intuitive behavior should be documented rather than having a more surprising undocumented behavior remaining.
I could be wrong about what is intuitive and what's not, though :) Thanks for taking the time to discuss w/ me.
krisdages
commented
6 years ago Also, I just realized... I don't think using "unset" or "inherit" would actually do the right thing in the case where that property is also defined in the CSS class.
If I just want to remove the property from the style attribute, so that the element reverts to the value defined in the class, it needs to be removed from the style attribute. Looks like passing empty string (or null) as a value does work for this.
fkleuver
commented
6 years ago @krisdages @huochunpeng I just want to point out here that there is a big difference between how null and undefined are treated by the DOM. undefined is interpreted as "not a value", e.g. it doesn't set the value (not even to empty string). null on the other hand does translate to empty string. This is behaving as expected in that sense. Just a little thing you got to know about.. :)
krisdages
commented
6 years ago @fkleuver Agreed on the way the DOM treats undefined. That's actually why I believe this ISN'T behaving as expected.
The binding of the style attribute to an object should make the style attribute equivalent to the object, meaning the style attribute should contain the properties defined on the object and not contain those that don't. Since undefined is an UNSET value, the style attribute should not contain the value for that property if it is present on the object. Unfortunately, since style.setProperty(propName, undefined) now does nothing to the style attribute, it's not valid to call it that way to remove the property.
StyleObserver tracks which styles are set through versions of bound objects and removes those that are no longer set.
If we add a check for undefined value when tracking the style here in StyleObserver.setValue(),
src/element-observation.js:88,
if (newValue instanceof Object) {
let value;
for (style in newValue) {
if (newValue.hasOwnProperty(style)) {
value = newValue[style];
/* Add Undefined Check Here */
if (value === undefined) {
continue;
}
/* End Undefined Check */
style = style.replace(/([A-Z])/g, m => '-' + m.toLowerCase());
styles[style] = version;
this._setProperty(style, value);
}
}
}then the style property would not be tracked in that binding version as having a value when it doesn't. The property would be removed from the attribute along with other unset properties at the end of the method: src/element-observation.js:118,
/* Unchanged */
version -= 1;
for (style in styles) {
if (!styles.hasOwnProperty(style) || styles[style] !== version) {
continue;
}
this.element.style.removeProperty(style);
}And we wouldn't be calling element.style.setProperty(style, undefined) with the expectation that it would have some effect, when that is no longer the case.
fkleuver
commented
6 years ago I think you misunderstood me a little:
undefined is interpreted as "not a value", e.g. it doesn't set the value (not even to empty string).
What I mean to say here is that undefined does not do anything. It neither sets nor unsets the value. null is how you unset something.
To illustrate:
var el = document.createElement('div')
el.style.color = 'green';
console.log(el.style.color); // 'green'
el.style.color = undefined;
console.log(el.style.color); // 'green' <- you are expecting it to be '' here
el.style.color = null;
console.log(el.style.color); // ''null is treated as an empty string by the HTMLElement interface for some properties as can be seen in the w3 spec. At least for textContent. It works like this in all major browsers. There is more vagueness around how undefined behaves which could explain why this may have changed across browser versions, but I don't know that for sure.
In any case, using null is closer to the "officially supported" way of unsetting a DOM string property on an HTMLElement than undefined.
krisdages
commented
6 years ago No, we are on the same page about what undefined does. I am suggesting that because setting undefined neither sets nor unsets the value, an undefined value should not be tracked as a set property by the style observer.
Binding to this object: { color: "red", zIndex: undefined }
should result in the same style attribute as binding to this object: { color: "red" },
which should be "color:red"
Under the current implementation, this is not always the case.
If I bind to { color: "red", zIndex: "100" }, then update the binding to { color: "red", zIndex: undefined },
the result style attribute string is "color: red; z-index: 100"
This means the binding is not deterministic.
The intention is not to suggest that a value of undefined is intended to explicitly unset the property, it is that a value of undefined should be treated as if it was nonexistent. (Meaning it gets removed if it was previously added by the binding, just like if the property was not present on the object)
fkleuver
commented
6 years ago Sorry for nitpicking, but we're really not on the same page about what undefined does x)
If I bind to { color: "red", zIndex: "100" }, then update the binding to { color: "red", zIndex: undefined }, the result style attribute string is "color: red; z-index: 100" This means the binding is not deterministic.
This is exactly as you would expect it to behave, and it is very much deterministic.
Setting a style property to undefined is equivalent to not performing any operation on it.
So if you first set the zIndex property to some non-undefined value, followed by setting it to undefined, it stays on the value that you initially set it to.
el.style.zIndex = '100';
el.style.zIndex = undefined; // it's still 100The intention is not to suggest that a value of undefined is intended to explicitly unset the property, it is that a value of undefined should be treated as if it was nonexistent.
Correct, and this is how it works with respect to the style property of the element (how the underlying mechanism works in the observer is an implementation detail that's not relevant here)
(Meaning it gets removed if it was previously added by the binding, just like if the property was not present on the object)
No! :) that's not how style works, as I demonstrated on my earlier comment:
You have to set it to null, empty string, or delete the property, just like you would have to if you were working directly with the element
krisdages
commented
6 years ago When I say the binding is not deterministic, what I mean is that I don't naturally consider a binding to be a series of applied instructions; rather I would expect any particular style object to have a corresponding string representation as a style attribute. And this is not the case when the same object (with some property set to undefined) bound to style results in two different styles (as you can see in the demo) depending on whether it is the first object bound to style or whether it is bound after a different object that has that property set to a value.
But maybe I just have an incorrect metaphor for what the style binding actually means.
I do understand how the Element.style property works, I'm just saying that the way that the style binding works currently can result in unexpected behavior if anyone happens to use an undefined value in their style object. Which can happen by accident even with TypeScript, since as far as I know, in strict null mode, you can't declare an optional property that is not allowed to have undefined as the value.
I can understand if the behavior of the binding will stay as is, since it's strictly not incorrect in that it sets the properties on the style object to the element's style, even if setting undefined to the style property will do nothing. I just think the fact that the binding may also REMOVE properties from the style attribute that are not present on the object could introduce confusion and potential for bugs when two situations where styleObject.property === undefined are handled differently.
I'm trying to think of a use case where someone would actually want or expect the current behavior... do you know of one?
I think it might help to at least put a caveat in the documentation to avoid using undefined and using delete or a null value in order to have the property removed from the attribute.
Sorry if there are difficulties in communicating what I mean, I'm not trying to be argumentative. Hopefully this explanation helps a little more.
fkleuver
commented
6 years ago I'm not trying to be argumentative.
All is good. Sometimes it takes some back-and-forth with nit picking to get to the crux of an issue.
I have read your reply and I hope I understood it correctly, but what I'm getting from it is that we appear to disagree on the expected behavior.
Put aside for a moment the current behavior, and let's talk first about the expected behavior. The two might not necessarily be aligned, but we need to agree on the expected before we can agree on whether a fix (in either code or docs) is needed.
In my opinion (as this is the general philosophy behind Aurelia), as close to the native behavior as possible. Mutating a bound source object should have the same effect on the bound DOM target as if you were performing the mutation directly on the bound DOM target.
How native html behaves, assume you're first setting element.style.backgroundColor to a value 1 which is a valid CSS property (let's say "green"), and then you set it to some value 2 (applies to points 1-4) If value 2 is:
undefined, then the property stays "green"null, then the property becomes """", then the property becomes """red", then the property becomes "red"element.style.removeProperty, then that property becomes ""See the image below. This corresponds to points 1-5 as follows:
The corresponding gist: https://gist.run/?id=3c50e01adabff686bd1bfd33bb5c3d5d
Again, I draw the parallels here in points 1-5 between how HTML natively works and how the observer should work.
The fact that native HTML behaves a bit weirdly (and therefore unexpected) is a different story, and one that might make it worth putting a note in the docs. But binding generally shouldn't be responsible for "correcting" wrongs in HTML. This is what value converters and binding behaviors are for. For example this could solve your problem:
export class NoUndefinedValueConverter {
toView(obj) {
for (const prop in obj) {
if (obj[prop] === undefined) {
delete obj[prop]
}
}
return obj;
}
}<div style.bind="style | noUndefined">With this in mind, I believe (with respect to the example in the gist) that the actual behavior is consistent with the expected behavior.
Now I know you disagree with this, and I understand why. I'm guessing you are expecting the observer to treat the binding in a more javascript-esque manner, where an undefined (but existing) evaluates to the same thing as a non-existing property. But we're not evaluating, we're using Object.hasOwnProperty and doing so very intentionally: this is the only way we can ensure that undefined is distinguished from non-existing. And for how the value itself affects the DOM, we're leaving that to the DOM.
Anyway, can you tell me which boxes you expect to be different from the picture?
krisdages
commented
6 years ago Very good points about philosophy ... and using a value converter would be a good solution (though I'd probably not mutate the object in that converter).
I guess part of my concern was my sense that what the binding does is transform the object to an equivalent style string -- it's binding an object to a string attribute after all, and using an object is presented as an alternative to string interpolation. I knew that DOM style objects were used to achieve this, but hypothetically this could even be done naively without them. With this conception, it didn't seem right that the result of the binding would have any relation to the state of the element.
If the docs were a bit more explicit about how the binding actually works (mutating the element's style property, with a warning about undefined and a suggestion for a value converter), I think that would help avoid the potential pitfalls of the mechanism.
BTW, the approach I was using that led to noticing this issue didn't have to do with mutating a bound object, it had to do with replacing the bound object; I think in the case of mutation it's conceptually a little more straightforward that whatever value you set gets applied to the element.
Just curious, what happens if you try to two-way bind the style attribute?
//The actual bugged behavior in my app was caused by something like this:
type Prop3Colors = "red" | "green";
interface StyleObject {
prop?: string;
prop2?: string;
prop3?: Prop3Colors;
}
function getStyleObject(): StyleObject {
const { value, value2 } = getSomeStyles();
return {
prop: value,
prop2: value2,
prop3: someCondition ? getComputedValue() : undefined
};
}
//A frustrating way to have to work around it, for which a value converter would eliminate the awkwardness, thx:
function getStyleObject2(): StyleObject {
const { value, value2 } = getSomeStyles();
const obj = {
prop: value,
prop2: value2
};
if (someCondition) {
obj.prop3 = getComputedValue();
}
return obj;
}Just curious, what happens if you try to two-way bind the style attribute?
StyleObserver.subscribe will throw an error. It doesn't support two-way binding (same goes for any other element attribute that does not dispatch an event when it changes). This will be supported in vNext however (via MutationObservers).
I guess part of my concern was my sense that what the binding does is transform the object to an equivalent style string
It doesn't. It's literally as if you're working directly with the style property of an element in javascript:
const el = document.createElement('div'); // el = HTMLDivElement
const style = el.style; // style = CSSStyleDeclaration
style.backgroundColor = 'green';
style.backgroundColor = undefined; // still greenThis is identical to:
<div style.bind="myStyle"></div>this.myStyle = { backgroundColor: 'green' };
this.taskQueue.queueMicroTask(() => {
this.myStyle = { backgroundColor: undefined }; // still green
});The difference is that the individual properties on the object are not observed, so you must always assign a new object if you want to change any property. Furthermore changes to the DOM are queued, so setting the style multiple times in a row from behind an observer is equivalent to setting it only once to the last value directly on the element (this is a general Aurelia thing that's consistent throughout binding).
So to reiterate: an object is not translated to a string. Just pretend it's a CSSStyleDeclaration. You can easily test stuff in the browser console and whatever happens there, you can expect to happen in binding.
Same thing applies to when you set it to a string. This is just as if you assigned that string to the .style property in javascript.
bigopon
commented
6 years ago ---- Advertisement plug start -----
https://github.com/bigopon/aurelia-style-binding-command-plugin for two way binding with [style]:
<div background-color.style="bg"></div>
<!-- optinally, if needed, can be made to work: -->
<div style.background-color="bg"></div>---- Advertisement plug end -----
krisdages
commented
6 years ago So to reiterate: an object is not translated to a string. Just pretend it's a CSSStyleDeclaration. You can easily test stuff in the browser console and whatever happens there, you can expect to happen in binding.
Same thing applies to when you set it to a string. This is just as if you assigned that string to the .style property in javascript.
I think this would be great to have in the docs to clarify the string vs object bindings. A caveat about undefined values for the object mode would help too.
Thanks again for the discussion. :)
krisdages
commented
6 years ago @fkleuver Is there a mechanism using DI or anything like that to wrap existing binding components by default, for example, if I wanted to append my value converter to all instances of style.bind without explicitly putting it in the template?
fkleuver
commented
6 years ago In vCurrent there's no true pluggable way of doing that (there will be in vNext), but it's easy nonetheless: either monkey patch the StyleObserver or, if you want a more OO approach, derive a new implementation from it and the observerlocator and return your own version instead. I generally can't think of a cleaner way to globally modify the behavior of an observer other than to simply modify that observer.. :)
const originalSetValue = StyleObserver.prototype.setValue;
StyleObserver.prototype.setValue = function(value) {
// magic stuff
originalSetValue.call(this, value);
// more magic stuff
}Something among those lines
I'm submitting a bug report
Please tell us about your environment:
Operating System: Linux (Ubuntu Bionic)
Node Version: Electron 3.0
NPM Version: 6.4.1
JSPM OR Webpack AND Version JSPM 0.17.0-beta.48
Browser: Electron 3.0 (Chrome 66)
Language: Typescript 3.0
Current behavior: Using style.bind="{ property: undefined }" does not unset property if the previous binding had a value for that property.
Using style.bind="{ }" does unset the property if the previous binding had a value for that property.
Expected/desired behavior: It should unset the property the same as if binding to an object without the property key.
In Electron 2.0, this was the behavior. I assume there was a change to style.setProperty() in Chromium where an undefined value no longer removes the style.
Can we call style.removeProperty (or replace undefined with null or "" when calling setProperty) instead to restore the previous behavior?
https://gist.run/?id=4044775d2efdb1733b55a0b9d92de378&sha=3acf777a662b49c4266c6449c2bf2d8413b250e6