Bug Tracker

Ticket #7109 (closed bug: fixed)

Opened 4 years ago

Last modified 2 years ago

animate width starts with invalid width on webkit

Reported by: kellyfelkins Owned by: gibson042
Priority: blocker Milestone: 1.8
Component: effects Version: 1.4.2
Keywords: Cc: snover
Blocking: Blocked by:

Description

This works nicely in firefox.

Not so nice in chrome and safari.

If you change width via animate, it appears the start of the animation is incorrect (too large).

The item is sitting there at a certain width, say "30%". If you animate it, changing its width to 0% it appears to first jump to about 60%, then animate smoothly to zero.

Attachments

animate.html Download (433 bytes) - added by kellyfelkins 4 years ago.
animates a div from 30% to 1% width. Has initial bump on webkit

Change History

Changed 4 years ago by kellyfelkins

animates a div from 30% to 1% width. Has initial bump on webkit

comment:1 Changed 4 years ago by snover

  • Cc snover added
  • need changed from Review to Patch
  • Component changed from unfiled to fx
  • Priority changed from undecided to low

comment:2 Changed 4 years ago by snover

  • Status changed from new to open

comment:3 Changed 4 years ago by snover

  • Milestone 1.4.3 deleted

Resetting milestone to future.

comment:4 Changed 4 years ago by aivo@…

I have seen similar buggy behavior with opacity and position on Chrome. Both are reproduced here:  http://jsfiddle.net/24cVh/3/

Position animation jumps to wrong position at the start and then animation is smooth to the right target position. Opacity jumps to 0 and then animates correctly to target value. Both animations should not jump, as they work in FF.

Position is buggy both in Windows and Linux, Opacity only in Linux version of Chrome 7.

comment:5 Changed 4 years ago by anonymous

I'm seeing a similar bug with jQuery 1.5.1 and Chrome 10.

animate position works on FireFox, but on Chrome, the start of the animation jumps back to the initial value from page load, rather than the current value.

comment:6 Changed 4 years ago by timmywil

  • Milestone set to 1.next

comment:7 Changed 3 years ago by gnarf

This issue doesn't seem to be "nice in firefox" either - It appears that the start value of any % based animation is being calculated incorrectly.

It has something to do with the calculation based on non-px values... There is a precision error going on here.

if ( unit !== "px" ) {
	jQuery.style( this, p, (end || 1) + unit);
	start = ((end || 1) / e.cur()) * start;
	jQuery.style( this, p, start + unit);
}

If the value for "end" is closer to say 100% the accuracy of the "starting" point is much higher...

Compare the jumpiness of the first frame of the animation on these two fiddles:  http://jsfiddle.net/GVFah/5/ (end point 1%)  http://jsfiddle.net/GVFah/4/ (end point 100%) -- In the case of % values we might just want to always use "100%" for this original start point calculation?

Last edited 3 years ago by gnarf (previous) (diff)

comment:8 Changed 2 years ago by jeff_themovie

I submitted a pull request at  https://github.com/jquery/jquery/pull/808.

The position animation bug that aivo mentioned (comment 4) is actually #9505. (I can't see anything wrong with the opacity animation.)

comment:9 Changed 2 years ago by mikesherov

  • Priority changed from low to blocker
  • Milestone changed from 1.next to 1.8

gnarf, reviewing this ticket, noticed that there is a major regression for percentage based animations. Run this fiddle on edge, and compare the behavior to 1.7.2:  http://jsfiddle.net/GVFah/4/

comment:10 Changed 2 years ago by mikesherov

  • Owner set to mikesherov
  • Status changed from open to assigned

comment:11 Changed 2 years ago by gibson042

@mikesherov, it is possible for tween.cur() to return 0 in the starting value computation, and the current method will respond by setting start to Infinity. I advocate a fix generalizing the above  pull request into iterative approximation; something like the following (but perhaps less obfuscated):

// We need to compute starting value
if ( unit !== "px" && start ) {
    // Iteratively approximate from a nonzero starting point
    // Prefer the current property; this process will be trivial if units match
    // Fallback to end or a simple constant
    start = parseFloat( jQuery.style( tween.elem, prop ) ) || end || 1;
    
    do {
        // If previous iteration zeroed out, double until we get *something*
        scale = scale || 0.5;
        
        // Adjust and apply
        start = start / scale;
        jQuery.style( tween.elem, prop, start + unit );
        
    // Update scale, tolerating zeroes from tween.cur()
    // Stop looping if scale is unchanged or we've hit the mark
    } while ( scale !== (scale = tween.cur() / target) && scale !== 1 );
}

 http://jsfiddle.net/JjQ4e/

(Note that we can skip the block entirely when start === 0).

And as an aside, I love the new animation hook points; they made this a breeze.

comment:12 Changed 2 years ago by mikesherov

  • Owner changed from mikesherov to gibson042

richard, take this one home! You have the solution already.

comment:13 Changed 2 years ago by gibson042

Damn; I was really hoping you'd have something to offer on "perhaps less obfuscated"... that while condition is hairy as hell.

comment:14 Changed 2 years ago by mikesherov

I suppose I can take a look. I was hastily reassigning it to you. I figured I can nitpick after you get the PR together :-P

comment:15 Changed 2 years ago by mikesherov

The other question is, does this resolve the regression I pointed out?

comment:16 Changed 2 years ago by gibson042

It'll shake out with the unit tests, but I believe so. The code on master miscalculates non-px start from trying to set style information on an animation object instead of its element. A naive correction would still suffer from miscalculation when end is sufficiently small, catastrophically when it is small enough to make tween.cur() return 0. PR 808 handles the first issue and goes a long way to handling the second; I just took it that final step.

Actually, I'd really just like to see PR 808 updated with the generalized code and unit tests so @jeff_themovie gets the credit he deserves. I'll save this for now and come back to it if there's no activity within a few days.

comment:17 Changed 2 years ago by jeff_themovie

I've updated the PR with Richard's test case and generalized solution. (Thanks Richard - awesome stuff!) Let me know if there's any other changes I should make.

comment:18 Changed 2 years ago by gnarf

I would think that the best way to calculate this would be to always assume "100" + unit when calculating the "unit value" -- it should give us enough granularity

comment:19 Changed 2 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.