opacity 1 should not have a nulling effect in oldIE, fixes #2336 #2411

Merged
merged 1 commit into from Aug 22, 2012

2 participants

@gonchuki

this also re-enables the opacity spec to run on IE, test case on #2336 also works with this modification.

The issue is that there was an incorrect assumption that setStyle('opacity', 1) meant removing the filter attribute as if 1 was the default, when in fact the element could have any other opacity set at the stylesheet level.
Enabled specs are green, pinging @cpojer in case he had some reason for interpreting 1 as null and I'm missing an obscure IE bug that is not showing with the current specs (does an @ mention actually ping the user? :) )

relevant commit that introduced this regression: 0411413

@arian
MooTools member

The problem is that transparent pngs are ugly with the filter. That's why it's removed when it's 1.

@gonchuki

totally right, then I think the next step would be to find out the default on the stylesheet and only null it if it isn't defined or equals 1, not sure if there's any flicker for setting the opacity twice in the same repaint, it shouldn't be but don't know how IE's filters behave in this case.
I already have an idea for this, let's leave the PR open till I get this coded.

@gonchuki

I think this is a good compromise, we let the "old" code do it's stuff but if the user requested an opacity of 1 we check if the element has a filter set in the CSS that gives it a different value.
We can only check this after we already removed the filter, so that's why I left it at the end of the block.

@arian arian commented on an outdated diff Aug 20, 2012
Source/Element/Element.Style.js
if (!element.currentStyle || !element.currentStyle.hasLayout) style.zoom = 1;
if (opacity == null || opacity == 1) opacity = '';
else opacity = 'alpha(opacity=' + (opacity * 100).limit(0, 100).round() + ')';
var filter = style.filter || element.getComputedStyle('filter') || '';
style.filter = reAlpha.test(filter) ? filter.replace(reAlpha, opacity) : filter + opacity;
if (!style.filter) style.removeAttribute('filter');
+ if (requestedOpacity == 1 && element.getStyle('opacity') != 1) style.filter = 'alpha(opacity=100)';
@arian
MooTools member
arian added a note Aug 20, 2012

maybe use getOpacity(element) directly here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian arian and 2 others commented on an outdated diff Aug 20, 2012
Source/Element/Element.Style.js
if (!element.currentStyle || !element.currentStyle.hasLayout) style.zoom = 1;
if (opacity == null || opacity == 1) opacity = '';
else opacity = 'alpha(opacity=' + (opacity * 100).limit(0, 100).round() + ')';
var filter = style.filter || element.getComputedStyle('filter') || '';
style.filter = reAlpha.test(filter) ? filter.replace(reAlpha, opacity) : filter + opacity;
if (!style.filter) style.removeAttribute('filter');
+ if (requestedOpacity == 1 && getOpacity(element) != 1) style.filter = 'alpha(opacity=100)';
@arian
MooTools member
arian added a note Aug 20, 2012

Probably need something like this again, to prevent overwriting other filters.

style.filter += ' alpha(opacity=100)';
@arian
MooTools member
arian added a note Aug 20, 2012

makes me wonder why there isn't a space between filter + opacity at https://github.com/mootools/mootools-core/pull/2411/files#L0R57

not sure, want me to add it?

@arian
MooTools member
arian added a note Aug 20, 2012

yeah, running this test https://github.com/mootools/mootools-core/blob/master/Specs/1.3client/Element/Element.Style.js#L19-36 shows that the result is actually blur(strength=50)alpha(opacity=40).

Technically it's another bug, so should be a separate pull request.. but because it's so tiny, a separate commit would be fine by me too.

@arian
MooTools member
arian added a note Aug 20, 2012

maybe add an

expect(div.style.filter).toEqual('blur(strength=50) alpha(opacity=40)');

to that test would be cool

@ibolmo
MooTools member
ibolmo added a note Aug 20, 2012

We should consider making a Filter (builder) helper and then doing a .filter = filter.toString(). That would simplify and make the code a little more readable.

definitely rethinking this, I wasn't taking into account the possibility of the element having other filters applied, and once that is added into the mix the tests aren't passing anymore. Making a helper seems to be a good idea.

@ibolmo
MooTools member
@arian
MooTools member
arian added a note Aug 21, 2012

Wouldn't that be overcomplicating things, only thing is add/remove opacity. Parsing/building all other properties with all kinds of other types of filter properties sounds hard.

What I'm doing right now is just using a small setFilter helper that recieves an element, regexp and filter string as a param.
It's just reusing the code we had inside setOpacity and makes code more readable and concise, which I think is enough of an improvement. I don't want to go overboard with a small block of code that is only for oldIE. I'm also modifying the 1.3 filter tests to not run on IE9+ as that browser is using native opacity and not filters.

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

blergh, I rebased against master and got a commit that shouldn't be here, let me fix this in a sec...

@arian arian commented on an outdated diff Aug 21, 2012
Specs/1.3client/Element/Element.Style.js
var div = new Element('div'),
- supports_filters;
+ supports_old_filters;
@arian
MooTools member
arian added a note Aug 21, 2012

Maybe change it to camelCase now we're here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian arian commented on the diff Aug 21, 2012
Source/Element/Element.Style.js
@@ -45,16 +45,25 @@ var setVisibility = function(element, opacity){
element.style.visibility = opacity > 0 || opacity == null ? 'visible' : 'hidden';
};
+//<ltIE9>
+var setFilter = function(element, regexp, value){
@arian
MooTools member
arian added a note Aug 21, 2012

Awesome, looks a lot cleaner this way!

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

To explain a bit the changes in 1.3 specs, the reason behind that is that setting opacity only affects the filters on oldIE, and since CSS3 has a working draft spec for allowing SVG filters to act on HTML elements which uses the same filter keyword we don't want to run those tests on browsers that support filter but are not using it to set the opacity.

@gonchuki

let me know if this gets green light so I can rebase to get it ready for a merge.

@arian
MooTools member
@arian arian merged commit 4d6b2af into mootools:master Aug 22, 2012
@arian
MooTools member

great work again! Maybe next time rebase on mootools/master, that's even cleaner :)

@gonchuki

yeah I just squashed my own commits since my rebase on master did ugly stuff a few commits above (the PR included 2 commits that weren't from this branch but were already merged into master). Maybe it's my lack of git-fu

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