Skip to content

Enhancement refactor element.style #2414

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.

Sign up for GitHub

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

Closed
ibolmo wants to merge 3 commits into from
Closed

Enhancement refactor element.style #2414

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7197436
Unrolled Element.Styles and Element.ShortStyles for ease.
ibolmo Aug 21, 2012
d8c0495
setStyle uses setters and mappers inside Element.Styles. Added opacit…
ibolmo Aug 21, 2012
b687daa
Major refactoring. Cleaned up getStyles by moving each special case t…
ibolmo Aug 22, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
324 changes: 260 additions & 64 deletions Source/Element/Element.Style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

/*
---

Expand Down Expand Up @@ -75,6 +76,16 @@ var floatName = (html.style.cssFloat == null) ? 'styleFloat' : 'cssFloat',
namedPositions = {left: '0%', top: '0%', center: '50%', right: '100%', bottom: '100%'},
hasBackgroundPositionXY = (html.style.backgroundPositionX != null);

function getSetter(property){
property = Element.Styles[property];
return property && property.set;
}

function getMap(property){
property = Element.Styles[property];
return property && property.map || '@';
}

Element.implement({

getComputedStyle: function(property){
Expand All @@ -85,22 +96,23 @@ Element.implement({
},

setStyle: function(property, value){
if (property == 'opacity'){
if (value != null) value = parseFloat(value);
setOpacity(this, value);
return this;
}
property = (property == 'float' ? floatName : property).camelCase();

property = (property == 'float') ? floatName : property.camelCase();

if (typeOf(value) != 'string'){
var map = (Element.Styles[property] || '@').split(' ');
var map = getMap(property).split(' ');
value = Array.from(value).map(function(val, i){
if (!map[i]) return '';
return (typeOf(val) == 'number') ? map[i].replace('@', Math.round(val)) : val;
}).join(' ');
} else if (value == String(Number(value))){
value = Math.round(value);
}
this.style[property] = value;

var setter = getSetter(property);
if (setter) setter(element, value);
else this.style[property] = value;

//<ltIE9>
if ((value == '' || value == null) && doesNotRemoveStyles && this.style.removeAttribute){
this.style.removeAttribute(property);
Expand All @@ -110,45 +122,29 @@ Element.implement({
},

getStyle: function(property){
if (property == 'opacity') return getOpacity(this);
property = (property == 'float' ? floatName : property).camelCase();

property = (property == 'float') ? floatName : property.camelCase();

var getter = getGetter(property);
Copy link
Member

@arian arian Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there isn't a getGetter function yet.

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe good call. Must have forgotten. I'll fix soon

if (getter) return getter(property);

var result = this.style[property];
if (!result || property == 'zIndex'){
if (Element.ShortStyles.hasOwnProperty(property)){
result = [];
for (var s in Element.ShortStyles[property]) result.push(this.getStyle(s));
return result.join(' ');
}

if (!result && Element.ShortStyles.hasOwnProperty(property)){
result = [];
for (var s in Element.ShortStyles[property]) result.push(this.getStyle(s));
return result.join(' ');
} else {
Copy link
Member

@gonchuki gonchuki Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code will wrongly enter here when result && !Element.ShortStyles.hasOwnProperty(property), you only need to run the getComputedStyle part if we got no result from the this.style[property]

Even better, I'm also not sure if doing: var result = this.style[property] || this.getComputedStyle(property); would break anything, current specs pass but that doesn't say much. Any reason to give ShortStyles precedence over trying to getComputedStyle on the element?

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right the change is not 1:1 with the previous logic. I'll modify soon.

Regards to your suggest change, again you're right. It's not clear why we can't default to getComputedStyle. If style getter doesn't work.

My other thought is, why can't we always use getComputedStyle? Was the original idea used to save CPU cycles?

Copy link
Member

@gonchuki gonchuki Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saving CPU cycles is a sure thing, remember than only IE has .currentStyle so other browsers will go though the longer function-call-heavy path. Considering that getStyle is used a lot on animations, we should always have the fastest possible path here to avoid jerkiness.

That being said, if it's safe to do style || getComputedStyle on this part of the code then we could integrate this default behavior right inside getComputedStyle if (this.style[property]) return this.style[property]; etc. as the rest of the uses of getComputedStyle are always as a default when the element.style cannot be retrieved (getOpacity, setFilter on #2411, etc)

Copy link
Member

@gonchuki gonchuki Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

investigated this, it's not safe to just integrate the element style inside getComputedStyle. Several tests break on different browsers so it seems it is like that for a reason. Removing the element.style querying altogether is neither an option, once again several tests break. Keeping in line with the comment I left on #2417 I think this part of the code should stay with the same logic flow unless we get way more specs to cover all bases.

edit: also a small correction to my comment above: seems that Opera also has currentStyle and it has a serious bug on getComputedStyle where if you set something as a %, the getComputedStyle call rounds the result to the nearest pixel integer instead of returning the exact sub-pixel value as Firefox/IE9/IE10 does (this unit conversion from % to px was addressed on #2160, some browsers convert from % to px, others don't).

Take this as misc info to reinforce the fact that current logic flow is mostly ok, even though I would still like to have getComputedStyle used if available instead of currentStyle as that's the standard and I will probably get up a PR later with these changes. I still think that doing small incremental changes gets us in a position where we can ship code now without requiring a ton of new specs.

result = this.getComputedStyle(property);
}
if (hasBackgroundPositionXY && /^backgroundPosition[XY]?$/.test(property)){
return result.replace(/(top|right|bottom|left)/g, function(position){
return namedPositions[position];
}) || '0px';
}
if (!result && property == 'backgroundPosition') return '0px 0px';

// todo(ibolmo): normalization is needed for which properties? and when
if (result){
result = String(result);
var color = result.match(/rgba?\([\d\s,]+\)/);
if (color) result = result.replace(color[0], color[0].rgbToHex());
}
if (Browser.opera || Browser.ie){
if ((/^(height|width)$/).test(property) && !(/px$/.test(result))){
var values = (property == 'width') ? ['left', 'right'] : ['top', 'bottom'], size = 0;
values.each(function(value){
size += this.getStyle('border-' + value + '-width').toInt() + this.getStyle('padding-' + value).toInt();
}, this);
return this['offset' + property.capitalize()] - size + 'px';
}
if ((/^border(.+)Width|margin|padding/).test(property) && isNaN(parseFloat(result))){
return '0px';
}
//<ltIE9>
if (returnsBordersInWrongOrder && /^border(Top|Right|Bottom|Left)?$/.test(property) && /^#/.test(result)){
return result.replace(/^(.+)\s(.+)\s(.+)$/, '$2 $3 $1');
}
//</ltIE9>
}

return result;
},

Expand All @@ -167,15 +163,6 @@ Element.implement({

});

Element.Styles = {
left: '@px', top: '@px', bottom: '@px', right: '@px',
width: '@px', height: '@px', maxWidth: '@px', maxHeight: '@px', minWidth: '@px', minHeight: '@px',
backgroundColor: 'rgb(@, @, @)', backgroundPosition: '@px @px', color: 'rgb(@, @, @)',
fontSize: '@px', letterSpacing: '@px', lineHeight: '@px', clip: 'rect(@px @px @px @px)',
margin: '@px @px @px @px', padding: '@px @px @px @px', border: '@px @ rgb(@, @, @) @px @ rgb(@, @, @) @px @ rgb(@, @, @)',
borderWidth: '@px @px @px @px', borderStyle: '@ @ @ @', borderColor: 'rgb(@, @, @) rgb(@, @, @) rgb(@, @, @) rgb(@, @, @)',
zIndex: '@', 'zoom': '@', fontWeight: '@', textIndent: '@px', opacity: '@'
};

//<1.3compat>

Expand Down Expand Up @@ -213,23 +200,232 @@ Element.Styles = new Hash(Element.Styles);

//</1.2compat>

Element.ShortStyles = {margin: {}, padding: {}, border: {}, borderWidth: {}, borderStyle: {}, borderColor: {}};
Element.Styles = {
left: {map: '@px'},
Copy link
Member

@arian arian Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not keep left: '@px' by default, only for special cases use an object. Is smaller and probably better for backward compat.

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could reduce this by adding more logic to the getGetter, getMap, and getStyle, but I'm also trying to encourage a simple hook mechanism. See the following comment.

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh and there is a simple byte reduction I had thought of, but I'm not implementing yet. give me your 2c:

'left,top,bottom,right,width,height,letterSpacing,lineHeight,...'.split(',').forEach(function(property){
    Element.Styles[property] = {map: '@px'};
});

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even further:

{
  '@px': 'left,top,bottom,right,width,height,letterSpacing,lineHeight,...',
  '@': 'zIndex',
  ...
}

And just have the initializer code once.

top: {map: '@px'},
bottom: {map: '@px'},
right: {map: '@px'},
width: {map: '@px'},
height: {map: '@px'},
letterSpacing: {map: '@px'},
lineHeight: {map: '@px'},
clip: {map: 'rect(@px @px @px @px)'},
zIndex: {
map: '@',
get: function(element){
return element.getComputedStyle('zIndex');
}
},
'zoom': {map: '@'},
textIndent: {map: '@px'},
opacity: {
map: '@',
get: function(element){
Copy link
Member

@arian arian Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

get: getOpacity,
set: setOpacity

should be able to handle it..

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I'm considering moving that logic inside the get and set. Just haven't gotten to it

return getOpacity(this);
Copy link
Member

@arian arian Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also you're using this here, instead of element.

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, forgot to update. nice catch

},
set: function(element, value){
if (value != null) value = parseFloat(value);
setOpacity(this, value);
return this;
}
},
color: {map: 'rgb(@, @, @)'},

maxWidth: {map: '@px'},
maxHeight: {map: '@px'},

['Top', 'Right', 'Bottom', 'Left'].each(function(direction){
var Short = Element.ShortStyles;
var All = Element.Styles;
['margin', 'padding'].each(function(style){
var sd = style + direction;
Short[style][sd] = All[sd] = '@px';
minWidth: {map: '@px'},
minHeight: {map: '@px'},

backgroundColor: {map: 'rgb(@, @, @)'},
backgroundPosition: {
map: '@px @px',
get: function(element){
return element.getComputedStyle('backgroundPosition') || '0px 0px';
}
},

fontWeight: {map: '@'},
fontSize: {map: '@px'},

margin: {map: '@px @px @px @px'},
marginTop: {map: '@px'},
marginRight: {map: '@px'},
marginBottom: {map: '@px'},
marginLeft: {map: '@px'},

padding: {map: '@px @px @px @px'},
paddingTop: {map: '@px'},
paddingRight: {map: '@px'},
paddingBottom: {map: '@px'},
paddingLeft: {map: '@px'},

border: {map: '@px @ rgb(@, @, @) @px @ rgb(@, @, @) @px @ rgb(@, @, @)'},
borderWidth: {map: '@px @px @px @px'},
borderStyle: {map: '@ @ @ @'},
borderColor: {map: 'rgb(@, @, @) rgb(@, @, @) rgb(@, @, @) rgb(@, @, @)'},

borderTop: {map: '@px @ rgb(@, @, @)'},
borderTopWidth: {map: '@px'},
borderTopColor: {map: '@px'},
borderTopStyle: {map: '@px'},

borderRight: {map: '@px @ rgb(@, @, @)'},
borderRightWidth: {map: '@px'},
borderRightStyle: {map: '@px'},
borderRightColor: {map: '@px'},

borderBottom: {map: '@px @ rgb(@, @, @)'},
borderBottomStyle: {map: '@px'},
borderBottomColor: {map: '@px'},
borderBottomWidth: {map: '@px'},

borderLeft: {map: '@px @ rgb(@, @, @)'},
borderLeftWidth: {map: '@px'},
borderLeftStyle: {map: '@px'},
borderLeftColor: {map: '@px'}
};

Element.ShortStyles = {
margin: {
marginTop: '@px',
marginRight: '@px',
marginBottom: '@px',
marginLeft: '@px'
},
padding: {
paddingTop: '@px',
paddingRight: '@px',
paddingBottom: '@px',
paddingLeft: '@px'
},
border: {
borderTop: '@px @ rgb(@, @, @)',
borderRight: '@px @ rgb(@, @, @)',
borderBottom: '@px @ rgb(@, @, @)',
borderLeft: '@px @ rgb(@, @, @)'
},
borderTop: {
borderTopWidth: '@px',
borderTopColor: '@px',
borderTopStyle: '@px',
borderRightWidth: '@px',
borderRightStyle: '@px',
borderRightColor: '@px',
borderBottomStyle: '@px',
borderBottomColor: '@px',
borderBottomWidth: '@px',
borderLeftWidth: '@px',
borderLeftStyle: '@px',
borderLeftColor: '@px'
},
borderRight: {
borderTopWidth: '@px',
borderTopColor: '@px',
borderTopStyle: '@px',
borderRightWidth: '@px',
borderRightStyle: '@px',
borderRightColor: '@px',
borderBottomStyle: '@px',
borderBottomColor: '@px',
borderBottomWidth: '@px',
borderLeftWidth: '@px',
borderLeftStyle: '@px',
borderLeftColor: '@px'
},
borderBottom: {
borderTopWidth: '@px',
borderTopColor: '@px',
borderTopStyle: '@px',
borderRightWidth: '@px',
borderRightStyle: '@px',
borderRightColor: '@px',
borderBottomStyle: '@px',
borderBottomColor: '@px',
borderBottomWidth: '@px',
borderLeftWidth: '@px',
borderLeftStyle: '@px',
borderLeftColor: '@px'
},
borderLeft: {
borderTopWidth: '@px',
borderTopColor: '@px',
borderTopStyle: '@px',
borderRightWidth: '@px',
borderRightStyle: '@px',
borderRightColor: '@px',
borderBottomStyle: '@px',
borderBottomColor: '@px',
borderBottomWidth: '@px',
borderLeftWidth: '@px',
borderLeftStyle: '@px',
borderLeftColor: '@px'
},
borderWidth: {
borderTopWidth: '@px',
borderRightWidth: '@px',
borderBottomWidth: '@px',
borderLeftWidth: '@px'
},
borderStyle: {
borderTopStyle: '@px', q
borderRightStyle: '@px',
borderBottomStyle: '@px',
borderLeftStyle: '@px'
},
borderColor: {
borderTopColor: '@px',
borderRightColor: '@px',
borderBottomColor: '@px',
borderLeftColor: '@px'
}
};


// todo(ibolmo): collisions?

if (hasBackgroundPositionXY){
Copy link
Member

@arian arian Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also move the feature detections here, closer where it's actually used.

Copy link
Member Author

@ibolmo ibolmo Aug 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

['backgroundPositionX', 'backgroundPositionY'].forEach(function(property){
Element.Styles[property] = {get: function(element){
return element.getComputedStyle(property).replace(/(top|right|bottom|left)/g, function(position){
return namedPosition[position];
}) || '0px';
}};
});
var bd = 'border' + direction;
Short.border[bd] = All[bd] = '@px @ rgb(@, @, @)';
var bdw = bd + 'Width', bds = bd + 'Style', bdc = bd + 'Color';
Short[bd] = {};
Short.borderWidth[bdw] = Short[bd][bdw] = All[bdw] = '@px';
Short.borderStyle[bds] = Short[bd][bds] = All[bds] = '@';
Short.borderColor[bdc] = Short[bd][bdc] = All[bdc] = 'rgb(@, @, @)';
});
Element.ShortStyles.backgroundPosition = {backgroundPositionX: '@', backgroundPositionY: '@'};
}

if (Browser.opera || Browser.ie){
Object.each({
width: ['borderTopWidth', 'borderBottomWidth', 'paddingTop', 'paddingBottom'],
height: ['borderLeftWidth', 'borderRightWidth', 'paddingLeft', 'paddingRight']
}, function(styles, property){
Element.Styles[property].get = function(element){
var result = element.getComputedStyle(property), size = 0;
if (result.substr(-2) == 'px') return result;

Object.forEach(element.getStyles(styles), function(value){ size += value; });
return (element['offset' + property.capitalize()] - size) + 'px';
};
});

for (var property in Element.Styles) if (/^border(.+)Width|margin|padding/.test(property)){
Element.Styles[property].get = function(element){
var result = element.getComputedStyle(property);
return isNaN(parseFloat(result)) ? '0px' : result;
}
}
}

//<ltIE9>
if (returnsBordersInWrongOrder){
for (var property in Element.Styles) if (/^border(Top|Right|Bottom|Left)?$/.test(property)){
Element.Styles[property].get = function(element){
var result = element.getComputedStyle(property);
return /^#/.test(result) ? result.replace(/^(.+)\s(.+)\s(.+)$/, '$2 $3 $1') : result;
};
}
}
//</ltIE9>

if (hasBackgroundPositionXY) Element.ShortStyles.backgroundPosition = {backgroundPositionX: '@', backgroundPositionY: '@'};
})();