Skip to content
This repository

Fx.Tween/Fx.Morph problem with '%' unit #2160

Closed
stecb opened this Issue December 07, 2011 · 19 comments

6 participants

Stefano Ceschi Berrini Olmo Maldonado Arian Stolwijk Dimitar Christoff jbgithub Christoph Pojer
Stefano Ceschi Berrini

Given this element:

<div id="box">box</div>

and this related css:

#box{
  position:absolute;
  width:200px;
  height:200px;
  left:10%;
  background:red;
}

I want to animate the box from 10% (original css value) to 50%, and the js is the following (wrapped inside a domready callback)

$('box')
  .set('morph',{unit : '%'})
  .morph({'left' : 50});

Demo : http://jsfiddle.net/steweb/6EnuH/

The problem: on firefox (and opera 11.5 on ubuntu), the initial morph value (that should be 10%), that is calculated by getComputedStyle, will return the px value (see https://bugzilla.mozilla.org/show_bug.cgi?id=707691). Therefore the animation will start from a wrong offset. On the other browsers (opera on mac os, chrome, safari, ie) the behav is correct.

Taking a look at the code, on the Fx.Css class, there's the code of the prepare method:

//prepares the base from/to object

prepare: function(element, property, values){
  values = Array.from(values);
  if (values[1] == null){
    values[1] = values[0];
    values[0] = element.getStyle(property); //with unit '%' the initial value will be the computed in px on ffox
  }
  var parsed = values.map(this.parse);
  return {from: parsed[0], to: parsed[1]};
}

A possible solution (tested) could be: if the unit is set to '%' (this.options.unit) put the element to display:none and once the value is retrieved, set it back to display:block (see https://bugzilla.mozilla.org/show_bug.cgi?id=707691#c7)

Arian Stolwijk
Owner

As we discussed on IRC the units are pretty inconsistent across browsers. Element.measure in MooTools More does a similar trick (although the other way around) with the display: block.

So the problem is in setStyle, which probably should return the value in '%' instead. The hack with the display:none doesn't sound as a very good idea to do that every time when you want to get a CSS property that has a length value..

Olmo Maldonado
Owner

@arian perhaps this is best solved in Fx.CSS reasoning stands that getStyle should fix this, but if it does it will be impractical unless there's a way to know what kind of unit you're expecting (perhaps an additional argument to getStyle?). Which is where Fx.CSS has the advantage. We can edge case check for '%' and use a feature detect to fix the behavior.

Although I'm not excited to display: none then display: block e.g. will cause a refresh glitch. Se la vi.

TL;DR:

  1. feature detect for bad behavior
  2. Fx.CSS.prepare, if bad behavior and %, display none, getStyle, and display: block
  3. add Note in getStyle about FFx and Opera bug. Optional:
  4. Add a github issue for adding a unit argument to getStyle for conversions.
Olmo Maldonado ibolmo referenced this issue from a commit in ibolmo/mootools-core January 29, 2012
Olmo Maldonado Fixes #2160.
Adds coverage for Fx.Morph unit: %.
25a996a
Olmo Maldonado ibolmo referenced this issue from a commit in ibolmo/mootools-core January 29, 2012
Olmo Maldonado Fixes #2160.
Using @ryanmorr's fix for non px starting values. This fix is primarily
for FFx, but it's not very easy to feature detect this Firefox bug. Open
to suggestions.

For now, this works quite well and also covers em, and other units.

This doesn't work for getStyle since we require an ending value, and
besides we wouldn't want to do this across all units.

PASSES: IE6-9; Firefox 3-5, 8, 10; Opera 11; Chrome latest; Safari 5
955eb53
Olmo Maldonado ibolmo closed this in 7c26dd3 February 05, 2012
Dimitar Christoff

this has broken Fx.Morph - https://github.com/mootools/mootools-core/blob/bd9275e325add7b7fa5e77f8d91c7a9918dc03ef/Source/Fx/Fx.CSS.js#L29 does not check if this.options.unit is != false first. result: cannot morph when values being morphed is defined as a string with unit like so: 100px.

http://alpha.jsfiddle.net/YFrwa/1/

Arian Stolwijk
Owner

Probably better solution in #2282

Olmo Maldonado
Owner

@DimitarChristoff, @stecb please give us a hand and test the master branch with your code.

Here are the instructions on how to build mootools: https://github.com/mootools/mootools-core/wiki/Build-MooTools-from-git-master

Dimitar Christoff
Stefano Ceschi Berrini

Oh yes, will do it asap!

jbgithub

I appreciate everyone’s work to make Mootools better but for the last three releases, major errors have been introduced. v1.4.2 crashed IE7, v1.4.3 prevented get() from retrieving custom attributes in IE8 or less, and now v1.4.4 causes NaN errors in the FX Class which is caused by the change in this thread.

Your home page states: “Mootools is compatible and fully tested with Safari, Internet Explorer 6+, Firefox, Opera, and Chrome.” As of late “fully tested” is falling short, especially for IE.

Having the public test your releases is not the way to keep your current user base or to increase it.

Olmo Maldonado
Owner

@jbgithub Undoubtedly we have released versions that introduced new bugs as a result of fixing old bugs. Even though we did thoroughly test each version prior to commit, I think you can appreciate the difficulty it is to have thorough and effective tests.

As soon as someone submits a new issue we usually fix the behavior only after we've created specs/tests that confirm the problem. This ensures that future commits cover those cases. We are not, however, predisposed to knowing if a fix will cause another bug if our specs/tests never covered the created edge case. As you're probably aware.

I also, hope, you're not suggesting that 1.4.1 and previous versions were bug free.

I think your suggestion that we should have a freeze period prior to public release, is acceptable and something we've already considered and put into action. As you'll notice we've asked @stecb and @DimitarChristoff to test in their builds and sites. Likewise, you're welcome to test master branch against your code base.

I also recommend that you separate your environments to include a staging area. That is not public, but also not behind a firewall, so you can invite private beta testers. This may include us.

Overall, it's a difficult issue since we cannot be stalled with the fear of introducing new bugs. We are going to adjust to have a buffer prior to release so as to reduce the chance that a release will have critical bugs. Finally: we can never guarantee a bug free release. With your help, and our community's help, we will find the bugs faster. Create coverage. Fix the bugs. Release a new version; and repeat.

jbgithub

I definitely appreciate the difficulty in developing and testing for the various platforms, browsers and browser versions. I have my own test environment (an array of virtual machines and MACs) in which I test. When I tested v1.4.2 I informed you of the line # that crashes IE7 in issue #2163. When I tested v1.4.3 I changed my code to use getAttribute() instead of get() or getProperty() since I didn't know if this was a bug or a permanent change. Yesterday I commented out the changes to prepare() to get v1.4.4 to work.

Never said the previous versions didn't have bugs but some bugs are more severe than others; v1.4.2 is unusable for IE7 and v1.4.4 is unusable for code reliant on the FX class.

My development and test areas are behind a firewall and my corporate clients have their own staging areas; so I unable to make these areas available.

I'd be happy to help and test a build during a freeze period, is there a way to get notified one is available?

On a side note I was happy that you brought back IE's fireEvent() in v1.4.2(3), I don't have to rename Mootool's fireEvent() anymore. This was a major issue to overcome in deciding to adopt Mootools in my development. One of the issues with Mootools fireEvent() is it does not create an event object to be passed to the event handler as dispatchEvent() and IE's fireEvent do. This prevents sharing one event handler with more than one element since you cannot determine the triggering element (event.target).

Olmo Maldonado
Owner

Thanks for your understanding, and I'm excited to have you on board for the freezing period. At some point, we'll open it up for more volunteers but we'll use issue contributors to ping for testing.

For now please check the stability of 1.4.5dev and if any of your tests fail, please let us know. We should start trickling specs/tests from user space to dev. We'd like to release 1.4.5 some time this week.

Regarding the fireEvent, could you please report it in another issue? I'd like for others to review it.

Dimitar Christoff
Dimitar Christoff

The jury on 1.4.5 nightly build is back.

UI acceptance test
-------------------------
Result:     Tests passed: 349 

Looking good thus far but it's only on the front-end, will try backoffice as well. Tomorrow QA will spend an hour on it or so.

Olmo Maldonado
Owner

Thank you @DimitarChristoff .. @arian looking go so far. Let us know when QA gives you the seal of approval, we'll tag it and ship it then -- I believe.

jbgithub

@ibolmo How do I get access to the nightly build? I am new to GitHub, am I to pull all the files and build it myself or is readily available?

Dimitar Christoff

QA have also finished testing for 3 hours, no application regressions were found in: IE7, IE9, FF (latest stable), Chrome (latest stable).

This does not by any means guarantee 1.4.5 is going to be bug free, particularly when you consider we referenced a mootools-more from 1.4.1 - though it's stable enough that we'd consider moving to it within a week or so after general release. I am saying it so it's not on us if things are wrong again - the business is puzzled by the need to contribute back into mootools as it is - though they have come to accept the clear benefits of a stable release we can use.

thanks to @agillborn and @obray for their hard work.

Christoph Pojer
Owner

wow thank you!

jbgithub

I ran thru my sites using 1.4.5dev and didn't encounter anything that would prevent me from implementing it when it is released. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.