greensock / GSAP

GSAP (GreenSock Animation Platform), a JavaScript animation library for the modern web
https://gsap.com
19.56k stars 1.72k forks source link

line-height is being animated with default px value instead of a number multiplied by the element's font size #206

Closed gibbok closed 7 years ago

gibbok commented 7 years ago

The line-height is being animated with default px value instead of a number multiplied by the element's font size.

To reproduce:

  1. Visit http://jsfiddle.net/gibbok/a9rr6sxn/
  2. Click button animate.
  3. Text should visually animate to an higher line-height, so space between lines (leading) should be increased, instead the leading is decreased.
  4. Inspect the style property for the animated DOM after the animation end (you will see the px unit being automatically added).
  TweenLite.to(target, 5, {
    marginLeft: 400,
    color: 'red',
    lineHeight: "5" // number only on unit
  });

The specification states that a single numeric value could be assigned to line-height. For my understanding TweenLite add a default unit in pixel, so it make impossible use a number only.

In case this is a bug. Could you please provide a work around?

The used value of the property is this number multiplied by the element's font size. Negative values are illegal. The computed value is the same as the specified value.

line-height specification: https://www.w3.org/TR/CSS2/visudet.html#propdef-line-height

catafest commented 7 years ago

I don't understand well what is the problem (3), try this example working well :

<div id="target">
okldajf jklsdjkl jlajf jj ,fmn,masn klaje tkldfjkljklfjkldsjkl gjl jsdgj jdkl</div>
<button aria-disabled="false" role="button" id="btn1">Animate</button>
<button id="btn2">Get line-height</button>

#target {
  width: 200px;
  line-height: 1;
  position: relative;
  width: 150px;
  height: 50px;
  font-family: Arial, Helvetica, sans-serif;
  text-align:center;
  border-bottom: solid #000 10px;
}

btn2.addEventListener('click', function(event) {
  var style = window.getComputedStyle(target);
  alert(style.lineHeight);
});

$('#btn1').on('click', function () {
    //alert('clicked');
    var logo = document.getElementById("target");
    TweenLite.to(target, 5, {
    marginLeft: 400,
    color: 'red',
    lineHeight: "5"
  });
});
gibbok commented 7 years ago

In your example, you are using TweenListe.To to set an animation lineHeight: "5".

The expected result IMO is an increase of leading (the space between line) as would normally happens when in CSS line-height is set to a numeric value without an unit (pixel).

Instead in your example the animation show up a decrease of leading.

The issue I believe it is because GSAP add a pixel unit automatically, when instead it should use only a numeric value with no unit.

  TweenLite.to(target, 5, {
    marginLeft: 400,
    color: 'red',
    lineHeight: "5"
  });
jackdoyle commented 7 years ago

Working on an answer for you; please stand by. Should have it to you later this afternoon.

jackdoyle commented 7 years ago

Okay, so I noticed a few problems:

  1. You were using a SUPER old version of TweenMax :)
  2. The current value is always reported by the browser as px in the computed style. This is a special case where unitless values make sense and we needed to add some conditional logic to accommodate it under the hood in GSAP (which I've done for the next update).

In the mean time, you could work around it using either of these two methods (both are forks of your original example):

http://jsfiddle.net/gs0cgjzn/1/ http://jsfiddle.net/wkqeoydf/1/

Both provide a reusable function that should make it relatively easy for you to convert whatever you need converted (in this case, unitless to px).

Does that help?

gibbok commented 7 years ago

Hi thanks for your help, I appreciate your work around.

Unfortunately I really need to minimize DOM calls, so currently I am solving this issue passing a unit (px or em).

Thanks for considering including this unite-less cases in GSAP for the next update. 👍

jackdoyle commented 7 years ago

Should be resolved now.