Correct visibility when fade is chained #2597

Merged
merged 2 commits into from Feb 6, 2015

Projects

None yet

5 participants

@SergioCrisostomo
Member

fixes #2593

When a new fade is called inside the onComplete event of first fade out,
then the opacity fades as expected but the visibility will not be set back to visible. This happens because the first fade sets the visibility after the fade is done. So the onComplete actually fires before the visibility is set.

related/history #2074

@SergioCrisostomo SergioCrisostomo Correct visibility when fading from onComplete
When a new fade is called inside the onComplete event of first fade out,
then the opacity fades as expected but the visibility will not be set
back to visible. This happens because the first fade sets the visibility
after the fade is done. So the onComplete actually fires before the
visibility is set.
b3c009e
@ibolmo ibolmo commented on the diff May 23, 2014
Specs/Fx/Fx.Tween.js
@@ -75,6 +75,33 @@ describe('Fx.Tween', function(){
});
+ it('should fade out an element and fade in when triggerd inside the onComplete event', function(){
+
+ var element = new Element('div').inject($(document.body));
+ var firstOpacity, lastOpacity, lastVisibility, runOnce = true;
+ element.set('tween', {
+ duration: 100,
+ onComplete: function (){
+ if(runOnce){
@ibolmo
ibolmo May 23, 2014 Member

tab/space problems.

@ibolmo ibolmo commented on the diff May 23, 2014
Source/Fx/Fx.Tween.js
@@ -87,11 +87,26 @@ Element.implement({
if (!toggle) this.eliminate('fade:flag');
fade[method].apply(fade, args);
var to = args[args.length - 1];
- if (method == 'set' || to != 0) this.setStyle('visibility', to == 0 ? 'hidden' : 'visible');
- else fade.chain(function(){
- this.element.setStyle('visibility', 'hidden');
- this.callChain();
- });
+
+ if (method == 'set'){
@ibolmo
ibolmo May 23, 2014 Member

I have a problem with this method. I think we added for some stupid reason (that jQuery had a similar method). Anyway. We need to unroll this method like this:

    fade: function(method){
        var tween = this.get('tween');
        if (!method || method == 'toggle') method = this.getStyle('opacity') ? 'out' : 'in';
        if (method == 'in') tween.start('opacity', 1);
        if (method == 'out') tween.start('opacity', 0);
        if (method == 'show') tween.set('opacity', 1);
        if (method == 'hide') tween.set('opacity', 1);
        return this;
    },

Only issue is the (stupid) edge cases. For example, I dropped the args magic. I mean that's ridiculous. Also the setting and unsetting of visibility shouldn't be fade's job. That should be set('opacity')'s job. Which I thought it did.

So what's next? Could you do me the huge favor of using my code, and running the specs. Also, if they pass just give yourself 15 more minutes to think of all the possible (ridiculous) edge cases that fade might have supported.

@SergioCrisostomo
SergioCrisostomo May 23, 2014 Member

@ibolmo sure! the code "as is" is not very clear. Will try your version.

@kentaromiura
kentaromiura May 26, 2014 Member

@ibolmo probably the set on fade is there to support IE wrong opacity with nearest 0 values, iirc there was some issue with that, that's why it was setting visibility.

@ibolmo
ibolmo May 26, 2014 Member

Yeah could be. Let's see what @SergioCrisostomo finds out.

@SergioCrisostomo
Member

(sorry it took me some time to answer on this)

So,

I would like us to follow @ibolmo 's suggestion and simplify this method.

The docs say:

Element shortcut method for tween with opacity. Useful for fading an Element in and out or to a certain opacity level.

The code needed to support what the Docs say .fade() should do is:

fade: function(method){
    var tween = this.get('tween');
    if (!method || method == 'toggle') method = this.getStyle('opacity') ? 'out' : 'in';
    if (method == 'in') tween.start('opacity', 1);
    if (method == 'out') tween.start('opacity', 0);
    if (method == 'show') tween.set('opacity', 1);
    if (method == 'hide') tween.set('opacity', 0);
    if (typeof method == 'number') tween.start('opacity', method); // corrected after Olmo's sharp eye
    return this;
},  
Question #1:

would it be more performative with else if instead of many if?

Question #2:

The specs refer to a "old" method that allows to do .fade(from, to). I did not find this in the docs. Looked from 1.2 to 1.5. Unless I am wrong I vote for removing that spec.

If this is needed we could add something like:

if (arguments.length == 2) {
    tween.set('opacity', arguments[0]);
    tween.start('opacity', arguments[1]);
    return this;
}

but still remove these specs

Question #3:

Should fade set visibility? There is no problem with opacity as I could see in IE6, or IE8 (ping @kentaromiura ). This fiddle works good in those old IE's making the element "disapear". So the visibility could be set in the onComplete event of the tween instead.

If we decide to keep this behavior that fade sets visibility both on fade-in and fade-out, then we need to use something like this.

@ibolmo
Member
ibolmo commented May 26, 2014

Note your change would be ... tween.start('opacity', method) right?

Question 1: Nope. if else is more expensive than a simple conditional.
Question 2: I'd say we drop it, but add a compatibility block just to be sure.
Question 3: I'm not looking at the source, but I would expect that setOpacity should be called by Fx.Tween. If not we have an issue there. If setOpacity is being called then Element.Style already checks for the need to set visibility.

@SergioCrisostomo
Member

@ibolmo, you were right about the "note", i edited/corrected above.
(and thanks for nice feedback!)

About visibility, it's tricky I think.

One option is to add this line under, inside the stop: method of FX.js, just before the complete events starts to fire (here):

if (this.to && this.to.opacity) this.element.setStyle('visibility', this.to.opacity[0].value == 0 ? 'hidden' : 'visible');

This will also be applied in other tween of opacity, not just fade.

Another option, based on the one above, is adding a flag on the storage, and retrieve/reset inside the stop:. This would avoid visibility been set when !fade

Otherwise I see only the "chain option" as is in the PR, based on what @arian added to solve past problems; i.e. that the setStyle('visibility') is passed into the chain to be ran at the end, thought after the complete event is called which is not so good.

@ibolmo
Member
ibolmo commented May 26, 2014

I'm confused. Why isn't this working already. Looking at Fx.CSS.js and
render:

render: function(element, property, value, unit){
 element.setStyle(property, this.serve(value, unit));
},

Therefore visibility should be taken care of because of:

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

and

var setOpacity = (hasOpacity ? function(element, opacity){
 element.style.opacity = opacity;
} : (hasFilter ? function(element, opacity){
if (!element.currentStyle || !element.currentStyle.hasLayout)
element.style.zoom = 1;
 if (opacity == null || opacity == 1){
setFilter(element, reAlpha, '');
if (opacity == 1 && getOpacity(element) != 1) setFilter(element, reAlpha,
'alpha(opacity=100)');
 } else {
setFilter(element, reAlpha, 'alpha(opacity=' + (opacity * 100).limit(0,
100).round() + ')');
 }
} : setVisibility));

On Mon, May 26, 2014 at 5:45 PM, Sergio Crisostomo <notifications@github.com

wrote:

@ibolmo https://github.com/ibolmo, you were right about the "note", i
edited/corrected above.

About visibility, it's tricky I think.

One option is to add this line under, inside the stop: method of FX.js,
just before the complete events starts to fire (herehttps://github.com/mootools/mootools-core/blob/master/Source/Fx/Fx.js#L104
):

if (this.to && this.to.opacity) this.element.setStyle('visibility', this.to.opacity[0].value == 0 ? 'hidden' : 'visible');

This will also be applied in other tween of opacity, not just fade.

Another option would be based on the one above, but adding a flag on
the storage, and retrieve and reset inside the stop: as above. This would
avoid visibility been set when !fade

Otherwise I see only the "chain option" as is in the PR, based on what
@arian https://github.com/arian added to solve past problems; i.e. that
the setStyle('visibility') is passed into the chain to be ran at the end,
thought after the complete event is called which is not so good.


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2597#issuecomment-44222119
.

@SergioCrisostomo
Member

@ibolmo setOpacity does not currently set visibility in browsers with hasOpacity or hasFilter. The fade is currently the one to do this by adding a setStyle('visibility') in the tween.chain. This was added here.

If we change this behaviour, so that when setStyle('opacity') is called visibility is also set, then these aditions (marqued with +) would do the job (specs looks green) and could be used use the new re-factored code posted above. This idea to always set visibility was abandoned before. So setStyle should only change the actual prop passed.

If we set visibility only when fired from a fade, here is my suggestion (http://jsfiddle.net/ZzZEF/), based on Olmo's refactor and @arian 's chain idea:

fade: function(method){
    var tween = this.get('tween');
    if (!method || method == 'toggle') method = this.getStyle('opacity') ? 'out' : 'in';
    //<1.3compat>
    if (arguments.length == 2) {
        tween.set('opacity', arguments[0]);
        tween.start('opacity', arguments[1]);
        return this;
    }
    //</1.3compat>
    if (method == 'in') tween.start('opacity', 1);
    if (method == 'out') tween.start('opacity', 0);
    if (method == 'show') tween.set('opacity', 1);
    if (method == 'hide') tween.set('opacity', 0);
    if (typeof method == 'number') tween.start('opacity', method);
    var to = tween.to[0].value, visible = this.getStyle('visibility') == 'visible';

    function changeVisibility(value, $tween){       
        if ($tween.$chain.length){
            $tween.chain(function (){
                this.element.setStyle('visibility', value);
                this.callChain();
            });
        } else {
            this.setStyle('visibility', value);
        }
    };

    if (to && !visible) changeVisibility.call(this, 'visible', tween);
    if (to == 0 && visible) changeVisibility.call(this, 'hidden', tween);
    return this;
},

Would be nice to have @arian looking at this refactor since he was the one to fix the opacity only setting visibility if fired from a fade

@ibolmo
Member
ibolmo commented May 27, 2014

Let's wait on @arian but I'd say drop the visibility. If we dropped it in
Element.setStyle then we should drop it here too.

On Tue, May 27, 2014 at 6:42 AM, Sergio Crisostomo <notifications@github.com

wrote:

@ibolmo https://github.com/ibolmo setOpacity does not currently set
visibility in browsers with hasOpacity or hasFilter. The fade is
currently the one to do this by adding a setStyle('visibility') in the
tween.chain. This was added herehttps://github.com/mootools/mootools-core/commit/11b4257f12a51454bd513ab1ac32cd5239d66098
.

If we change this behaviour, so that when setStyle('opacity') is called
visibility is also set, then these aditionshttps://gist.github.com/SergioCrisostomo/fccda619ca4cbd23a218(marqued with
+) would do the job (specs looks green) and could be used use the new
re-factored code posted above. This idea to always set visibility was
abandoned beforehttps://github.com/mootools/mootools-core/issues/2074#issuecomment-2114173.
So setStyle should only change the actual prop passed.

If we set visibility only when fired from a fade, here is my suggestion
(http://jsfiddle.net/ZzZEF/), based on Olmo's refactor and @arianhttps://github.com/arian's chain idea:

fade: function(method){
var tween = this.get('tween');
if (!method || method == 'toggle') method = this.getStyle('opacity') ? 'out' : 'in';

//<1.3compat>

if (arguments.length == 2) {
    tween.set('opacity', arguments[0]);
    tween.start('opacity', arguments[1]);

    return this;
}
//</1.3compat>

if (method == 'in') tween.start('opacity', 1);
if (method == 'out') tween.start('opacity', 0);
if (method == 'show') tween.set('opacity', 1);
if (method == 'hide') tween.set('opacity', 0);


if (typeof method == 'number') tween.start('opacity', method);
var to = tween.to[0].value, visible = this.getStyle('visibility') == 'visible';

function changeVisibility(value, $tween){
    if ($tween.$chain.length){
        $tween.chain(function (){
            this.element.setStyle('visibility', value);
            this.callChain();
        });
    } else {
        this.setStyle('visibility', value);
    }
};

if (to && !visible) changeVisibility.call(this, 'visible', tween);
if (to == 0 && visible) changeVisibility.call(this, 'hidden', tween);
return this;},

Would be nice to have @arian https://github.com/arian looking at this
refactor since he was the one to fix the opacity only setting visibilityif fired from a
fade


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2597#issuecomment-44264753
.

@timwienk
Member

If I recall correctly, we made the "stop setting visibility in setStyle" change for 1.4.0, which was a logical change. But then we had to very quickly release 1.4.1 to fix fade, because our implementation of that always set visibility to hidden after fading opacity to 0 (arguably makes sense to do that anyway, fade is more than just a "change style" operation).

I wouldn't be in favour of "just" dropping the setting of visibility in fade, expecting problems if we do. We should first investigate if we want to do such a breaking change. #2074 and #2081 provide more context.

@ibolmo
Member
ibolmo commented May 27, 2014

Based on that I would say we should add:

Warning pseudo

tween.addEvent('complete', function(){
  (opacity == 0) ? hide : visibile;
});

On Tue, May 27, 2014 at 8:12 AM, Tim Wienk notifications@github.com wrote:

If I recall correctly, we made the "stop setting visibility in setStyle"
change for 1.4.0, which was a logical change. But then we had to very
quickly release 1.4.1 to fix fade, because our implementation of that
always set visibility to hidden after fading opacity to 0 (arguably makes
sense to do that anyway, fade is more than just a "change style"
operation).

I wouldn't be in favour of "just" dropping the setting of visibility in
fade, expecting problems if we do. We should first investigate if we want
to do such a breaking change. #2074https://github.com/mootools/mootools-core/issues/2074and
#2081 #2081 provide more
context.


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2597#issuecomment-44273686
.

@ibolmo ibolmo added this to the 1.5.1 milestone Jul 3, 2014
@SergioCrisostomo
Member

@arian could you give your opinion here?

@arian
Member
arian commented Jul 4, 2014

What @timwienk said. setStyle('opacity', x) doesn't set the visibility anymore, which is a good thing. This have some problems in this fade method, which is why .fade should set the visibility. This makes sense because otherwise links are still clickable and such.

So instead of refactoring the entire method, it should do exactly the same as it does now, but slightly different so it sets the visibility at the right moment (which was the original issue).

@SergioCrisostomo SergioCrisostomo modified the milestone: 1.5.2, 1.5.1 Jul 6, 2014
@SergioCrisostomo
Member

Really nice feedback from you guys!

I made now a new approach and cleaned the PR.
Added a Spec that reproduces the problem described at #2593 and got green specs.
Would this be a ok approach?

@ibolmo ibolmo commented on an outdated diff Feb 6, 2015
Source/Fx/Fx.Tween.js
@@ -87,11 +87,26 @@ Element.implement({
if (!toggle) this.eliminate('fade:flag');
fade[method].apply(fade, args);
var to = args[args.length - 1];
- if (method == 'set' || to != 0) this.setStyle('visibility', to == 0 ? 'hidden' : 'visible');
- else fade.chain(function(){
- this.element.setStyle('visibility', 'hidden');
- this.callChain();
- });
+
+ if (method == 'set'){
+ this.setStyle('visibility', to == 0 ? 'hidden' : 'visible');
+ } else if (to != 0){
+ if (fade.$chain.length){
+ fade.chain(function (){
@ibolmo
ibolmo Feb 6, 2015 Member

remove space in function () same in L103.

@ibolmo
Member
ibolmo commented Feb 6, 2015

After the space is cleaned, this LGTM. I think we can invest more time in this, but it's something that I think we can revisit when we refactor for 1.6.

@ibolmo ibolmo merged commit 3ad6cd0 into mootools:master Feb 6, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@SergioCrisostomo SergioCrisostomo deleted the SergioCrisostomo:fix-fade-chain branch Feb 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment