Skip to content

Commit

Permalink
refactor virtualAttrStyle binding to fix #661
Browse files Browse the repository at this point in the history 
Moved to a virtualAttrStyles binding expecting an object of css properties instead of a plain string.
Also changed the main declarations.js code so to do the quoting of the property in the binding serializer when needed.
  • Loading branch information
@bago
bago committed Jun 10, 2022
1 parent 4b9d961 commit 9b60f09
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 36 deletions.
52 changes: 45 additions & 7 deletions spec/declarations-spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ describe('Style declaration processor', function() {
styleSheet = require('../src/js/converter/cssparser.js').parse("#{\n" + 'color: red; -ko-color: @color; background-color: white' + "}");
decls = styleSheet.stylesheet.rules[0].declarations;
previewBindings = declarations.elaborateDeclarationsAndReturnStyleBindings(decls, templateUrlConverter, mockedBindingProvider);
expect(previewBindings).toEqual("virtualAttrStyle: 'color: '+ko.utils.unwrapObservable($color[red])+'; '+'background-color: white;'+''");
expect(previewBindings).toEqual("virtualAttrStyles: { color: $color[red], 'background-color': 'white' }");

styleSheet = require('../src/js/converter/cssparser.js').parse("#{\n" + 'color: red; background-color: white; -ko-color: @color' + "}");
decls = styleSheet.stylesheet.rules[0].declarations;
previewBindings = declarations.elaborateDeclarationsAndReturnStyleBindings(decls, templateUrlConverter, mockedBindingProvider);
expect(previewBindings).toEqual("virtualAttrStyle: 'color: '+ko.utils.unwrapObservable($color[red])+'; '+'background-color: white;'+''");
expect(previewBindings).toEqual("virtualAttrStyles: { color: $color[red], 'background-color': 'white' }");

});

Expand All @@ -40,7 +40,16 @@ describe('Style declaration processor', function() {
styleSheet = require('../src/js/converter/cssparser.js').parse("#{\n" + '-ko-bind-text: @[\'Pulsante\']; -ko-font-family: @face; -ko-color: @color; -ko-font-size: @[size]px; -ko-background-color: @buttonColor; padding-left: 5px; -ko-border-radius: @[radius]px; padding: 5px;' + "}");
decls = styleSheet.stylesheet.rules[0].declarations;
previewBindings = declarations.elaborateDeclarationsAndReturnStyleBindings(decls, templateUrlConverter, mockedBindingProvider);
expect(previewBindings).toEqual("virtualAttrStyle: 'padding-left: 5px; '+'padding: 5px;'+'', text: 'Pulsante', virtualStyle: { fontFamily: $face[undefined], color: $color[undefined], fontSize: $size[undefined]()+'px', backgroundColor: $buttonColor[undefined], borderRadius: $radius[undefined]()+'px' }");
expect(previewBindings).toEqual("virtualAttrStyles: { 'padding-left': '5px', padding: '5px' }, text: 'Pulsante', virtualStyle: { fontFamily: $face[undefined], color: $color[undefined], fontSize: $size[undefined]()+'px', backgroundColor: $buttonColor[undefined], borderRadius: $radius[undefined]()+'px' }");
});

it('should correctly deal with dashed properties', function() {
var result;
result = declarations.elaborateElementStyleDeclarations('background-color: red; -ko-background-color: @color', templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("background-color: red; background-color: <!-- ko text: $color[red] -->red<!-- /ko -->");

result = declarations.elaborateElementStyleDeclarations('-ms-color: red;-ko--ms-color: @color;background-color: white', templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("-ms-color: red;-ms-color: <!-- ko text: $color[red] -->red<!-- /ko -->;background-color: white");
});

it('should mantain spaces and ; when removing/replacing declarations', function() {
Expand Down Expand Up @@ -96,7 +105,7 @@ describe('Style declaration processor', function() {
styleSheet = require('../src/js/converter/cssparser.js').parse("#{\n" + 'color: red; -ko-color: @mycolor; -ko-color-if: mycondition' + "}");
decls = styleSheet.stylesheet.rules[0].declarations;
result = declarations.elaborateDeclarationsAndReturnStyleBindings(decls, templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("virtualAttrStyle: 'color: '+($mycondition[undefined]() ? ko.utils.unwrapObservable($mycolor[red]) : null)+';'+''");
expect(result).toEqual("virtualAttrStyles: { color: $mycondition[undefined]() ? ko.utils.unwrapObservable($mycolor[red]) : null }");

result = declarations.elaborateElementStyleDeclarations('color: red; -ko-color: @mycolor; -ko-color-if: mycondition', templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("color: red; color: <!-- ko text: $mycondition[undefined]() ? ko.utils.unwrapObservable($mycolor[red]) : null -->red<!-- /ko -->; ");
Expand All @@ -107,7 +116,7 @@ describe('Style declaration processor', function() {
styleSheet = require('../src/js/converter/cssparser.js').parse("#{\n" + 'color: red; -ko-color: @mycolor; -ko-color-if: mycondition gt 1 and mycondition lt 3' + "}");
decls = styleSheet.stylesheet.rules[0].declarations;
result = declarations.elaborateDeclarationsAndReturnStyleBindings(decls, templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("virtualAttrStyle: 'color: '+((($mycondition[undefined]() > 1) && ($mycondition[undefined]() < 3)) ? ko.utils.unwrapObservable($mycolor[red]) : null)+';'+''");
expect(result).toEqual("virtualAttrStyles: { color: (($mycondition[undefined]() > 1) && ($mycondition[undefined]() < 3)) ? ko.utils.unwrapObservable($mycolor[red]) : null }");

result = declarations.elaborateElementStyleDeclarations('color: red; -ko-color: @mycolor; -ko-color-if: mycondition gt 1 and mycondition lt 3', templateUrlConverter, mockedBindingProvider);
expect(result).toEqual("color: red; color: <!-- ko text: (($mycondition[undefined]() > 1) && ($mycondition[undefined]() < 3)) ? ko.utils.unwrapObservable($mycolor[red]) : null -->red<!-- /ko -->; ");
Expand Down Expand Up @@ -266,15 +275,44 @@ describe('Style declaration processor', function() {
var cheerio = require('cheerio');
var $ = cheerio.load('<a data-attribute="ciao"></a>');
result = declarations.elaborateElementStyleDeclarations('-ko-attr-data-attribute: @myvalue; background-color: red; -ko-background-color: @mycolor', templateUrlConverter, mockedBindingProvider, $('a')[0]);
expect($('a').attr('data-bind')).toEqual("virtualAttr: { 'data-attribute': $myvalue[ciao] }, virtualAttrStyle: 'background-color: '+ko.utils.unwrapObservable($mycolor[red])+';'+''");
expect($('a').attr('data-bind')).toEqual("virtualAttr: { 'data-attribute': $myvalue[ciao] }, virtualAttrStyles: { 'background-color': $mycolor[red] }");

$ = cheerio.load('<a attribute="ciao"></a>');
result = declarations.elaborateElementStyleDeclarations('-ko-attr-attribute: @myvalue; color: red; -ko-color: @mycolor', templateUrlConverter, mockedBindingProvider, $('a')[0]);
expect($('a').attr('data-bind')).toEqual("virtualAttr: { attribute: $myvalue[ciao] }, virtualAttrStyles: { color: $mycolor[red] }");

});

it('should keep multiple declarations for the same property', function() {
var result;
var cheerio = require('cheerio');
var $ = cheerio.load('<div/>');
result = declarations.elaborateElementStyleDeclarations('color: yellow; color: red; color: blue; -ko-color: @mycolor', templateUrlConverter, mockedBindingProvider, $('div')[0]);
expect($('div').attr('data-bind')).toEqual("virtualAttrStyles: { color$2: 'yellow', color$1: 'red', color: $mycolor[blue] }");

// This is a weird use case, but we have templates using this style and we want to maintain the behaviour
$ = cheerio.load('<div/>');
result = declarations.elaborateElementStyleDeclarations('color: yellow; color: red; color: blue; -ko-color: @mycolor1; -ko-color: @mycolor2', templateUrlConverter, mockedBindingProvider, $('div')[0]);
expect($('div').attr('data-bind')).toEqual("virtualAttrStyles: { color$2: 'yellow', color$1: 'red', color: $mycolor1[blue] }");

// Before mosaico 0.18.6 this used to produce something like "color: yellow, color: $mycolor1[blue], color: blue"
// where the default "blue" value was read from the rightmost element.
// Then we slightly changed the logic so that it should get the default value from the same property it will replace.
$ = cheerio.load('<div/>');
result = declarations.elaborateElementStyleDeclarations('color: yellow; color: red; -ko-color: @mycolor1; -ko-color: @mycolor2; color: blue', templateUrlConverter, mockedBindingProvider, $('div')[0]);
expect($('div').attr('data-bind')).toEqual("virtualAttrStyles: { color$2: 'yellow', color$1: $mycolor1[red], color: 'blue' }");

$ = cheerio.load('<div/>');
result = declarations.elaborateElementStyleDeclarations('color: yellow; color: red; -ko-color: @mycolor1; color: blue; -ko-color: @mycolor2', templateUrlConverter, mockedBindingProvider, $('div')[0]);
expect($('div').attr('data-bind')).toEqual("virtualAttrStyles: { color$2: 'yellow', color$1: $mycolor1[red], color: $mycolor2[blue] }");
});

it('should deal with conditional bindings with correct parentheses', function() {
var result;
var cheerio = require('cheerio');
var $ = cheerio.load('<a data-attribute="ciao"></a>');
result = declarations.elaborateElementStyleDeclarations('background-color: red; -ko-background-color: @color; -ko-background-color-if: visible', templateUrlConverter, mockedBindingProvider, $('a')[0]);
expect($('a').attr('data-bind')).toEqual("virtualAttrStyle: 'background-color: '+($visible[undefined]() ? ko.utils.unwrapObservable($color[red]) : null)+';'+''");
expect($('a').attr('data-bind')).toEqual("virtualAttrStyles: { 'background-color': $visible[undefined]() ? ko.utils.unwrapObservable($color[red]) : null }");
});

afterAll(function() {
Expand Down
28 changes: 25 additions & 3 deletions src/js/bindings/virtuals.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,36 @@ ko.bindingHandlers['virtualAttr'] = {
};
ko.virtualElements.allowedBindings['virtualAttr'] = true;

ko.bindingHandlers['virtualAttrStyle'] = {
var _objectToStyle = function(obj) {
var res = false;
for (var prop in obj) if (obj.hasOwnProperty(prop)) {
var val = ko.utils.unwrapObservable(obj[prop]);
if (val !== null) {
if (res == false) res = ''; else res += '; ';
// ignore everything after the $ (so to support repeated properties)
res += prop.split('$')[0] + ': ' + val;
}
}
// in past our style attrs always ended with ";", so this like would preserve that behaviour.
// if (res) return res+";";
return res;
};

/**
* We use this binding instead of the style binding because we want better control on the specific style attribute output.
* This receive an ordered object of css properties pointing to their values and laso supporting "null" to remove the property
* as if it wasn't in the object.
*
* The property names could be $something suffixed so to be able to declare the same property multiple times.
*/
ko.bindingHandlers['virtualAttrStyles'] = {
update: function(element, valueAccessor, allBindingsAccessor, viewModel, bindingContext) {
if (element.nodeType !== 8) {
// In "preview" we also set "replacedstyle" so to have an attribute to be used by IE (IE breaks the STYLE) to do the export.
var isNotWysiwygMode = (typeof bindingContext.templateMode == 'undefined' || bindingContext.templateMode != 'wysiwyg');
var attrs = ["style"];
if (isNotWysiwygMode) attrs.push("replacedstyle");
var attrValue = ko.utils.unwrapObservable(valueAccessor());
var attrValue = _objectToStyle(valueAccessor());
for (var i = 0; i < attrs.length; i++) {
var attrName = attrs[i];
var toRemove = (attrValue === false) || (attrValue === null) || (attrValue === undefined);
Expand All @@ -32,7 +54,7 @@ ko.bindingHandlers['virtualAttrStyle'] = {
}
}
};
ko.virtualElements.allowedBindings['virtualAttrStyle'] = true;
ko.virtualElements.allowedBindings['virtualAttrStyles'] = true;

ko.bindingHandlers['virtualStyle'] = {
update: function(element, valueAccessor) {
Expand Down
66 changes: 40 additions & 26 deletions src/js/converter/declarations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,19 @@ var cssParser = require("./cssparser.js");
var console = require("console");
var domutils = require("./domutils.js");

var _declarationValueLookup = function(declarations, propertyname, templateUrlConverter) {
for (var i = declarations.length - 1; i >= 0; i--) {
var _declarationValueLookup = function(declarations, propertyname, templateUrlConverter, start, end) {
if (start == undefined) start = declarations.length;
if (end == undefined) end = 0;
for (var i = start - 1; i >= end; i--) {
if (declarations[i].type == 'property' && declarations[i].name == propertyname) {
return converterUtils.declarationValueUrlPrefixer(declarations[i].value, templateUrlConverter);
}
}
// compatibility mode: if we can't find a default value before the current declaration we loop from the end.
if (start < declarations.length) {
return _declarationValueLookup(declarations, propertyname, templateUrlConverter, declarations.length, start)
}

return null;
};

Expand Down Expand Up @@ -104,17 +111,21 @@ var elaborateDeclarations = function(newStyle, declarations, templateUrlConverte
if ((isAttr || isBind) && (typeof element == 'undefined' && newStyle !== null)) throw "Attributes and bind declarations are only allowed in inline styles!";

var needDefaultValue = true;
var bindName = propName;
var bindType;
if (isAttr) {
propDefaultValue = domutils.getAttribute(element, propName);
needDefaultValue = false;
bindType = 'virtualAttr';
} else if (!isBind) {
needDefaultValue = newStyle !== null;
bindType = 'virtualStyle';
bindName = _propToCamelCase(propName);

// in past we didn't read the default value when "needDefaultValue" was false:
// now we try to find it anyway, and simply don't enforce it.
propDefaultValue = _declarationValueLookup(declarations, propName, templateUrlConverter);
bindType = 'virtualStyle';
propDefaultValue = _declarationValueLookup(declarations, propName, templateUrlConverter, i);

} else {
bindType = null;
if (propName == 'text' || propName == 'stylesheet') {
Expand All @@ -138,9 +149,6 @@ var elaborateDeclarations = function(newStyle, declarations, templateUrlConverte
console.error("Cannot find default value for", declarations[i].name, declarations, element, newStyle, declarations[i]);
throw "Cannot find default value for " + declarations[i].name + ": " + declarations[i].value + " in " + element + " (" + typeof newStyle + "/" + propName + ")";
}
var bindDefaultValue = propDefaultValue;

var bindName = !isBind && !isAttr ? _propToCamelCase(propName) : (propName.indexOf('-') != -1 ? '\''+propName+'\'' : propName);

bindValue = _generateBindValue(declarations, bindingProvider, declarations[i].name, declarations[i].value, propDefaultValue, templateUrlConverter);

Expand Down Expand Up @@ -197,25 +205,29 @@ var elaborateDeclarations = function(newStyle, declarations, templateUrlConverte
}

// Style handling by concatenated "style attribute" (worse performance but more stable than direct style handling)
var bind = 'virtualAttrStyles';

if (typeof newBindings[bind] == 'undefined') newBindings[bind] = {};

var bindName2 = _propToCamelCase(declarations[i].name);
var bind = 'virtualAttrStyle';
var bindVal2 = typeof newBindings['virtualStyle'] !== 'undefined' ? newBindings['virtualStyle'][bindName2] : undefined;

var dist = ' ';
if (typeof newBindings[bind] == 'undefined') {
newBindings[bind] = "''";
dist = '';
// make sure to use an unique property name to avoid overriding properties.
// If the input have multiple named properties we want to keep them all and dynamically
// replace only the last one.
// The 'virtualAttrStyles' binding will remove the $i at the end of the property name.
var key = ""+declarations[i].name;
var kk;
while (typeof newBindings[bind][key] !== 'undefined') {
kk = key.split('$');
key = kk[0] + '$' + (kk.length > 1 ? Math.round(kk[1])+1 : 1);
}

if (typeof bindVal2 !== 'undefined') {
// the match is a bit ugly, but we don't want to unwrap things if not needed (performance)
if (bindVal2.match(/^[^' ]*[^' \)]$/)) bindVal2 = 'ko.utils.unwrapObservable(' + bindVal2 + ')';
// make sure we use parentheses for ternary conditional operator
else bindVal2 = '(' + bindVal2 + ')';
newBindings[bind] = "'" + declarations[i].name + ": '+" + bindVal2 + "+';" + dist + "'+" + newBindings[bind];
newBindings[bind][key] = bindVal2;
delete newBindings['virtualStyle'][bindName2];
} else {
newBindings[bind] = "'" + declarations[i].name + ": " + addSlashes(replacedValue) + ";" + dist + "'+" + newBindings[bind];
newBindings[bind][key] = "'" + addSlashes(replacedValue) + "'";
}

}
Expand All @@ -225,7 +237,7 @@ var elaborateDeclarations = function(newStyle, declarations, templateUrlConverte
if (typeof element != 'undefined' && element !== null) {
for (var prop in newBindings['virtualStyle'])
if (newBindings['virtualStyle'].hasOwnProperty(prop)) {
console.log("Unexpected virtualStyle binding after conversion to virtualAttr.style", prop, newBindings['virtualStyle'][prop], newStyle);
console.error("Unexpected virtualStyle binding after conversion to virtualAttr.style", prop, newBindings['virtualStyle'][prop], newStyle, newBindings, declarations);
throw "Unexpected virtualStyle binding after conversion to virtualAttr.style for " + prop;
}
delete newBindings['virtualStyle'];
Expand All @@ -247,11 +259,11 @@ var elaborateDeclarations = function(newStyle, declarations, templateUrlConverte
}
if (!hasVirtualStyle) delete newBindings['virtualStyle'];
else {
// remove and add back virtualAttrStyle so it gets appended BEFORE virtualAttrStyle (_bindingSerializer reverse them...)
if (typeof newBindings['virtualAttrStyle'] !== 'undefined') {
var vs = newBindings['virtualAttrStyle'];
delete newBindings['virtualAttrStyle'];
newBindings['virtualAttrStyle'] = vs;
// remove and add back virtualAttrStyles so it gets appended BEFORE virtualStyle (_bindingSerializer reverse them...)
if (typeof newBindings['virtualAttrStyles'] !== 'undefined') {
var vs = newBindings['virtualAttrStyles'];
delete newBindings['virtualAttrStyles'];
newBindings['virtualAttrStyles'] = vs;
}
}
// returns new serialized bindings
Expand All @@ -265,8 +277,10 @@ var _bindingSerializer = function(val) {
var res = [];
for (var prop in val)
if (val.hasOwnProperty(prop)) {
if (typeof val[prop] == 'object') res.push(prop + ": " + "{ " + _bindingSerializer(val[prop]) + " }");
else res.push(prop + ": " + val[prop]);
res.push(
(prop.indexOf('-') !== -1 ? "'" + addSlashes(prop) + "'" : prop) + ': ' +
(typeof val[prop] == 'object' ? "{ " + _bindingSerializer(val[prop]) + " }" : val[prop])
);
}
return res.reverse().join(', ');
};
Expand Down

0 comments on commit 9b60f09

Please sign in to comment.