Normalize backgroundPosition, fixes #2213 and #2295 #2409

Merged
merged 2 commits into from Aug 20, 2012

Projects

None yet

4 participants

@gonchuki
Collaborator

several issues solved here:

  • IE7 & 8 fail to return the backgroundPosition of an element when it was set on the stylesheet
  • IE 7-8-9 (not sure about 10) return the top|right|bottom|left keywords when doing a getStyle if that keyword was used (7 and 8 only do it for backgroundPosition[XY], they correctly normalize backgroundPosition)
  • IE7, IE8, Chrome and Opera return an empty string on a getStyle over an element that has no backgroundPosition set

this patch normalizes all those scenarios so that all browsers return the same values (using the suggested normalization hash on #2213, plus other fixes).
If someone can test on IE10 it would be great, the fiddle I added to #2295 works fine after this patch is applied.

@gonchuki gonchuki Normalize backgroundPosition and ensure that backgroundPosition[XY] a…
…lways

returns something on browsers that support that extension.
Fixes #2213 and #2295
1e400ff
@ibolmo
Member
ibolmo commented Aug 16, 2012

Great work. I'll review carefully soon, but with a quick look I'm concerned with getStyle it's starting to become a monster.

I'd like for us invest some time to figure out a better (mutation? e.g. hooks) pattern for us to use.

@gonchuki
Collaborator

yeah I noticed it's getting big. There's one small change/optimization I didn't do as part of this patch, the if statement on line 116 could be reduced to this:

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(' ');
    }
    result = this.getComputedStyle(property);
}

I really have no idea why there's an iteration with a continue statement there. I know it's not much of an improvement but it's a first step on to the right direction. Let me know if you want me to get another pull request on that after we are done with this one.

As for doing a whole revamp, I'm afraid we would first need to get a more comprehensive set of specs to avoid getting into regressions or missing the chance to fix other bugs. All my latest patches shown that Element.Styles.js was heavily under-specced.

@ibolmo
Member
ibolmo commented Aug 16, 2012

Agreed on the comprehensive specs. A must have. I'll check the above when I can.

@ibolmo ibolmo and 1 other commented on an outdated diff Aug 17, 2012
Source/Element/Element.Style.js
@@ -120,6 +122,12 @@ Element.implement({
}
result = this.getComputedStyle(property);
}
+ if (hasBackgroundPositionXY && /^backgroundPosition[XY]$/.test(property) || property == 'backgroundPosition' && /(top|right|bottom|left)/.test(result)){
@ibolmo
ibolmo Aug 17, 2012 MooTools member

/^backgroundPosition[XY]?$/

@gonchuki
gonchuki Aug 17, 2012 collaborator

not really, the idea here is to test if the property is backgroundPositionX or Y only if the browser supports those extensions.
What goes behind this is that if you try to retrieve backgroundPositionX in say Firefox, you will "correctly" get undefined instead of a wrongly normalized 0px (which we don't need to normalize, as it's non-standard)

@ibolmo
ibolmo Aug 17, 2012 MooTools member

Sorry I was too quick you can reduce the if statement to:

if (hasBackgroundPositionXY && /^backgroundPosition[XY]?$/.test(property)){

Since adding the ? is the same as property == 'backgroundPosition'

@gonchuki
gonchuki Aug 17, 2012 collaborator

thinking a little more about it I think I will settle this case, let's go with your option even if it runs more times than needed (a query for backgroundPosition on an element that has no keyword based components will still enter on the conditional).
At least the code is easier to understand, even if it doesn't reflect the strict cases I was trying to apply this on.

@ibolmo ibolmo and 1 other commented on an outdated diff Aug 17, 2012
Source/Element/Element.Style.js
@@ -120,6 +122,12 @@ Element.implement({
}
result = this.getComputedStyle(property);
}
+ if (hasBackgroundPositionXY && /^backgroundPosition[XY]$/.test(property) || property == 'backgroundPosition' && /(top|right|bottom|left)/.test(result)){
+ return result.replace(/(\w+)/g, function(value) { return value && namedPositions[value] || value || '0px'; }) || '0px';
@ibolmo
ibolmo Aug 17, 2012 MooTools member

When would the last || '0px'; ever execute ... since replace should always return true, unless result is a '' ? But then the following line is if (!result ...) so then you could add if (result && hasBackgroundPositionXY ...)

Finally, is the /(top|right|bottom/left).test check really necessary? Looks like regardless you're checking for namedPositions[value] and default to the matched value ... or false if the value is false. Which in that case just do return (!value ? value : namedPosition[value] || value);

or

if (namedPosition[value]) value = namedPosition[value];
return value || '0px';
@gonchuki
gonchuki Aug 17, 2012 collaborator

This 0px default is for browsers that support backgroundPosition[XY] and are returning an empty value on those properties (IE7 and 8 are on that group). Since those browsers get their backgroundPosition decomposed into the 2 sub-properties when it can't be retrieved directly, we need to return something or else the test against an element that has no set backgroundPosition style will return a space (that is, 2 empty strings concatenated by a space).

For the second part, the regexp against top/right/bottom/left is simply to avoid running the normalization code if we already have a non-keyword based result (only IE7-8-9 enter on that part of the condition).

as for the replace, I think we can further reduce it to just return namedPositions[value] || value;. What's currently there seems to be a left-over of several iterations I did to fix this. The replace will only call the function if it finds something, so there's no chance that empty values reach that point.

@ibolmo
ibolmo Aug 17, 2012 MooTools member

Thanks for the explanation.

Update the PR with the changes, and I'll have a second pass at it.

@ibolmo ibolmo commented on an outdated diff Aug 17, 2012
Source/Element/Element.Style.js
@@ -224,4 +232,7 @@ Element.ShortStyles = {margin: {}, padding: {}, border: {}, borderWidth: {}, bor
Short.borderColor[bdc] = Short[bd][bdc] = All[bdc] = 'rgb(@, @, @)';
});
+if (hasBackgroundPositionXY) {
+ Object.merge(Element.ShortStyles, {backgroundPosition: {backgroundPositionX: '@', backgroundPositionY: '@'}});
@ibolmo
ibolmo Aug 17, 2012 MooTools member
Element.ShortStyles.backgroundPosition = {backgroundPositionX: '@', backgroundPositionY: '@'};
@ibolmo ibolmo and 1 other commented on an outdated diff Aug 17, 2012
Specs/1.4client/Element/Element.Style.js
@@ -175,4 +175,51 @@ describe('Element.Style', function(){
});
});
+
+ describe('getStyle background-position', function(){
+ beforeEach(function(){
+ var className = String.uniqueID();
@ibolmo
ibolmo Aug 17, 2012 MooTools member

No need for random uid. Just pick something like: getStyleBackroundPosition. If you're still paranoid just do a quick search against the repo. and adjust, if necessary.

In the end, a descriptive class name will help debug than a runtime random UI

@gonchuki
gonchuki Aug 17, 2012 collaborator

copy-pasta from another spec on that same file :) Will cleanup as part of next commit.

@ibolmo ibolmo commented on the diff Aug 17, 2012
Specs/1.4client/Element/Element.Style.js
+
+ // fix this, see https://github.com/mootools/mootools-core/issues/2265
+ if (style.styleSheet) style.styleSheet.cssText = definition;
+ else style.set('text', definition);
+
+ document.head.appendChild(style);
+
+ this.element = new Element('div', {
+ 'class': className,
+ text: 'yo'
+ }).inject(document.body);
+ });
+
+ afterEach(function(){
+ this.style.destroy();
+ this.element.destroy();
@ibolmo
ibolmo Aug 17, 2012 MooTools member

I believe you'll still have to null this.element so that IE gc picks it up

@arian
Member
arian commented Aug 17, 2012

Great work!

A way to clean it up would be to create an object with style getters, that's probably already a bit cleaner than all the inlined checks in the method.

@gonchuki
Collaborator

for the record, I installed Win8 Preview on a virtual machine and IE10 exposes the same IE7 and IE8 issue where it returns an empty string for a getStyle('backgroundPosition') on an element that has that style set on the stylesheet. The X/Y decomposition works, and it doesn't seem to return keywords for the components.

edit: I listed another issue here, that was not true (so confused with 5 different open browser windows plus 2 virtual machines)

@ibolmo
Member
ibolmo commented Aug 17, 2012

I got confused with #2409 (comment).

Are the specs passing? What was your question? :D

@gonchuki
Collaborator

specs pass, no question. On the original comment on the PR I stated I didn't test on IE10, so the exercise was to confirm if the fixes here also covered any inconsistency that could be still present in IE10. The interesting thing is that in this case IE10 behaves more like IE8 than IE9.

@ibolmo ibolmo and 1 other commented on an outdated diff Aug 17, 2012
Source/Element/Element.Style.js
@@ -120,6 +122,12 @@ Element.implement({
}
result = this.getComputedStyle(property);
}
+ if (hasBackgroundPositionXY && /^backgroundPosition[XY]?$/.test(property)){
+ return result.replace(/(\w+)/g, function(value) { return namedPositions[value] || value; }) || '0px';
@ibolmo
ibolmo Aug 17, 2012 MooTools member

Considering one of your previous comments I think this would be a good compromise:

return result.replace(/(top|right|bottom|left)/g, function(position){ return namedPosition[position]; }) || '0px';

Now only the named positions are replaced with their respective percentage values.

@ibolmo
ibolmo Aug 17, 2012 MooTools member

Up to you (and it's not a deal breaker) but this may be more aesthetic as well:

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

Note, this would mean that L128 - L130 are no longer needed. Just need for you to confirm that the hasBackgroundPositionXY check doesn't break/filter something out.

@gonchuki
gonchuki Aug 20, 2012 collaborator

I will only go with the keyword replace (way more readable, if not also faster, nice catch there).
We can't integrate the 0px 0px default inside the if block because it will not return the default on browsers that lack the X/Y extensions (Opera in this case). unless we want to remove this default and handle such cases where needed (Fx module)

@ibolmo ibolmo commented on an outdated diff Aug 17, 2012
Source/Element/Element.Style.js
@@ -224,4 +232,7 @@ Element.ShortStyles = {margin: {}, padding: {}, border: {}, borderWidth: {}, bor
Short.borderColor[bdc] = Short[bd][bdc] = All[bdc] = 'rgb(@, @, @)';
});
+if (hasBackgroundPositionXY) {
@ibolmo
ibolmo Aug 17, 2012 MooTools member

You can remove the {} and 1-line.

Also, I just noticed a style guide problem. You'll need to check your diff and find ) { and replace with ){ :D

@ibolmo
Member
ibolmo commented Aug 20, 2012

If everything is green, then we can merge. @arian do you concur ;D

@arian
Member
arian commented Aug 20, 2012

yeah, looks good, maybe just those stylistic changes:

diff --git a/Source/Element/Element.Style.js b/Source/Element/Element.Style.js
index 7ae578d..adfdc30 100644
--- a/Source/Element/Element.Style.js
+++ b/Source/Element/Element.Style.js
@@ -123,7 +123,9 @@ Element.implement({
            result = this.getComputedStyle(property);
        }
        if (hasBackgroundPositionXY && /^backgroundPosition[XY]?$/.test(property)){
-           return result.replace(/(top|right|bottom|left)/g, function(position){ return namedPositions[position]; }) || '0px';
+           return result.replace(/(top|right|bottom|left)/g, function(position){
+               return namedPositions[position];
+           }) || '0px';
        }
        if (!result && property == 'backgroundPosition') return '0px 0px';
        if (result){
@@ -231,4 +233,5 @@ Element.ShortStyles = {margin: {}, padding: {}, border: {}, borderWidth: {}, bor
 });

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

I can commit this and merge it.. or maybe if you fix it, maybe rebase it, we can just press the nice green merge button ;)

@gonchuki
Collaborator

sure, slow week(s) at work :)

@ibolmo ibolmo merged commit 1f73ef0 into mootools:master Aug 20, 2012
@ibolmo
Member
ibolmo commented Aug 20, 2012

Done! Thanks @gonchuki and @arian

@SergioCrisostomo
Member

@gonchuki could you check the PR Arian linked (& related issue in it), if you have some time? I'm proposing a change of order in your code and you might want to have some input about it... Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment