kornelski / slip

Slip.js — UI library for manipulating lists via swipe and drag gestures
BSD 2-Clause "Simplified" License
2.44k stars 213 forks source link

What is the utility of this.target.node.style.willChange? #66

Closed Jenselme closed 8 years ago

Jenselme commented 8 years ago

This was introduced in https://github.com/pornel/slip/commit/9649fe050e6c5a56b6adece932940401d2225549 and I don't see what it is used for.

However, we found a bug in an application that uses slijps but if I comment this line it works.

Should it be removed or if it has an interest, should transformPrefix and transformProperty be improved?

Jenselme commented 8 years ago

Maybe setting transformProperty to an empty string on non webkit browsers until the transform property is widely adopted could be a good compromise. This solves our bug on firefox:

diff --git i/src/lib/slip.js w/src/lib/slip.js
index 9c5e3aa..c3c5e67 100644
--- i/slip.js
+++ w/slip.js
@@ -131,7 +131,7 @@ window['Slip'] = (function(){

     var transitionPrefix = "webkitTransition" in testElement.style ? "webkitTransition" : "transition";
     var transformPrefix = "webkitTransform" in testElement.style ? "webkitTransform" : "transform";
-    var transformProperty = transformPrefix === "webkitTransform" ? "-webkit-transform" : "transform";
+    var transformProperty = transformPrefix === "webkitTransform" ? "-webkit-transform" : "";
     var userSelectPrefix = "webkitUserSelect" in testElement.style ? "webkitUserSelect" : "userSelect";

     testElement.style[transformPrefix] = 'translateZ(0)';

What do you think?

kornelski commented 8 years ago

Setting willChange informs the browser that the element will animate soon, so it should be using an accelerated layer. Without it you would have a delay/jerky animation when it starts moving.

Setting transformProperty to an empty string like that would completely break all animations in all browsers that don't emulate WebKit.

kornelski commented 8 years ago

the names of variables are not clear though, transformPrefix should be JSTransformPropertyName and transformProperty should be CSSTransformPropertyName.

Jenselme commented 8 years ago

Setting willChange informs the browser that the element will animate soon, so it should be using an accelerated layer. Without it you would have a delay/jerky animation when it starts moving.

OK. Maybe the detection of FF can be added then? With 'MozTransform' in testElement.style we can detect if we are on FF and then we can set transformProperty to "-moz-transform" which solves our problem as well.

Do you want me to work on a patch for that?

kornelski commented 8 years ago

Firefox has unprefixed transform in 2012 in v16. I don't think there's a need to support versions that old.

Jenselme commented 8 years ago

Firefox has unprefixed transform in 2012 in v16.

Yet on FF 44, transformProperty must be either an empty string or -moz-transform for you bug to be fixed. Don't ask me why thought.

kornelski commented 8 years ago

That's weird. I've checked, and FF (v47) emulates webkit properties, so the unprefixed empty-string transformProperty is not even used.

I've switched the logic around to prefer unprefixed now.

Perhaps willChange exposes a browser bug. Promoting element to a layer could mess with z-index. However, checkboxes and select elements work on my demo page, so I can't reproduce the problem you're reporting.

Jenselme commented 8 years ago

However, checkboxes and select elements work on my demo page, so I can't reproduce the problem you're reporting.

I think that part of the problem is that on non webkit browser, the input is actually a span so it can be styled correctly. However, it is a span on IE and everything works fine.

kornelski commented 8 years ago

Sorry, I don't understand. Are you saying you're using something like <span onclick> instead of <input type=checkbox>?

Jenselme commented 8 years ago

Are you saying you're using something like instead of ?

Yes

kornelski commented 8 years ago

In that case, try disabling swipe-drag on the span:

ol.addEventListener('slip:beforeswipe', function(e){
// change that to detect your <span>
        if (e.target.nodeName == 'INPUT' || /demo-no-swipe/.test(e.target.className)) {
            e.preventDefault();
        }
    }, false);
Jenselme commented 8 years ago

Just tried that. But the e.preventDefault() seems to prevent the click to be taken into account. So the checkbox doesn't work.

kornelski commented 8 years ago

preventDefault is for the custom event, not the click.

You may also want to prevent slip:beforereorder to stop span from being used for dragging up/down.

Jenselme commented 8 years ago

We finally decided to remove willChange in SlipJS to solve our problem.

Anyway, thanks for your help and you awesome lib!