add feature detect and fix for oldIE to normalize order of border style #2405

Merged
merged 1 commit into from Aug 16, 2012

Projects

None yet

3 participants

@gonchuki

fixes #2391

let me know if the coding style is not up to par, I reused the same el element for the feature detect to save some bytes and runtime resources.

@gonchuki gonchuki and 2 others commented on an outdated diff Aug 13, 2012
Source/Element/Element.Style.js
@@ -131,6 +135,11 @@ Element.implement({
if ((/^border(.+)Width|margin|padding/).test(property) && isNaN(parseFloat(result))){
return '0px';
}
+ //<ltIE9>
+ if (returnsBordersInWrongOrder && /^border(Top|Right|Bottom|Left)?$/.test(property) && /^#.+/.test(result)) {
@gonchuki
gonchuki Aug 13, 2012

just a note here, the last check (/^#.+/) is there because if you only set 2 components of a border (say, borderLeftWidth and borderLeftColor) IE would actually return the styles in the correct order. It's only after you set the 3 components that you get the color as the first component.

@arian
arian Aug 13, 2012

i'm not a regex expert, but how does this regex check if it are 2 or 3 components?

@arian
arian Aug 13, 2012

it just checks if there is a # right.. so that would be for the color.. does it always return a hex code, even if it's set with rgb?

@gonchuki
gonchuki Aug 13, 2012

previous code already converts rgb to hex: https://github.com/mootools/mootools-core/blob/master/Source/Element/Element.Style.js#L121
also, the border* styles always return 3 components (unset values just return the browser defaults).

@ibolmo
ibolmo Aug 13, 2012
if (... /^#/.test(result)){

Not the lack of need for .+ and removal of extra space after ) {

@ibolmo
ibolmo Aug 13, 2012

or

result.chartAt(0) == '#'
@ibolmo ibolmo and 1 other commented on an outdated diff Aug 13, 2012
Source/Element/Element.Style.js
@@ -24,6 +24,10 @@ var el = document.createElement('div');
el.style.color = 'red';
el.style.color = null;
var doesNotRemoveStyles = el.style.color == 'red';
+
+// check for oldIE, which returns border* shorthand styles in the wrong order (color-width-style instead of width-style-color)
+el.style.border = '1px solid #123abc';
@ibolmo
ibolmo Aug 13, 2012
var border = '1px solid #123abc'; // allows js compressors to do their job
el.style.border = border;
var returnsBorderInWrongOrder = (el.style.border != border);
@gonchuki
gonchuki Aug 13, 2012

right, did it on the spec and forgot to port it back here where it really matters.

@ibolmo ibolmo commented on an outdated diff Aug 13, 2012
Specs/1.4client/Element/Element.Style.js
@@ -158,5 +158,20 @@ describe('Element.Style', function(){
});
});
+
+ describe('getStyle border after setStyle', function(){
+
+ it('should have same order when getting a previously set border', function(){
+ var border = '2px solid #123abc',
+ element = new Element('div', {
+ styles: {
+ 'border': border
+ }
+ });
+
+ expect(element.getStyle('border')).toEqual(border);
@ibolmo
ibolmo Aug 13, 2012

This is no big deal, but extra poitnts if you add a element = null; so that IE gc can collect the element.

@ibolmo
ibolmo Aug 13, 2012

Oh and just fyi, this could have worked without worry of IE gc:

var border = '2px solid #123abc';
expect(new Element('div').setStyle('border', border).getStyle('border')).toEqual(border);
@gonchuki

I can create a new pull request with a rebased version of this if you want to keep a cleaner story on the main repo :)

@arian
MooTools member

I think you can rebase / squash this stuff too. iirc GitHub has become smarter with this stuff.

@gonchuki

interesting feature @arian, I remember in the past it just used to lose the entire pull request and comment history when you rebased a set of already pull-requested commits :)

@arian
MooTools member

yep, much better. I'll pull this if everybody agrees 👍

@arian arian merged commit 85017cf into mootools:master Aug 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment