Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($animateCss): consider options.delay value for closing timeout #13363

Merged
merged 1 commit into from
Nov 26, 2015
Merged

fix($animateCss): consider options.delay value for closing timeout #13363

merged 1 commit into from
Nov 26, 2015

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Nov 23, 2015

Previously, options.delay was only considered when a class added an
extra transition style, because that leads to style recalculation.

Fixes #13355

@Narretz Narretz added this to the 1.4.8 milestone Nov 23, 2015
@Narretz Narretz mentioned this pull request Nov 23, 2015
Closed
gkalpak
gkalpak reviewed Nov 23, 2015
View reviewed changes
src/ngAnimate/animateCss.js
@@ -623,6 +623,7 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {

if (options.delay != null) {
var delayStyle = parseFloat(options.delay);
maxDelay = Math.max(options.delay, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this rather be delayStyle (instead of options.delay) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. The maxDelay is mainly used for determining the closing timeout. And options.delay overwrites values set in the transition styles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delayStyle is just parseFloat(options.delay).

But thinking about it again, neither option will work as expected in case options.delay === true (which is a valid option according to the docs).
From a superficial look, we might need to make something like on animateCss.js#L849-L850 and animateCss.js#L863-L867.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that looks more complex than I initially thought. I'll discuss this with Matias tomorrow.

@Narretz Narretz force-pushed the fix-nganimate-css-13355 branch 2 times, most recently from b78a1d2 to 38c4db2 Compare November 24, 2015 14:32
Narretz
Narretz reviewed Nov 24, 2015
View reviewed changes
test/ngAnimate/animateCssSpec.js
@@ -1234,6 +1234,67 @@ describe("ngAnimate $animateCss", function() {
$timeout.flush();
}).not.toThrow();
}));

it("should consider a positive options.delay value for the closing timeout",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this test fails. It should ensure that the animation ends exactly then when it's supposed to be, not before. And it passed locally for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fails with jquery, because the .css implementation is different from jqlite's

@Narretz Narretz force-pushed the fix-nganimate-css-13355 branch from 38c4db2 to 962509e Compare November 25, 2015 18:36
Narretz added a commit to Narretz/angular.js that referenced this pull request Nov 25, 2015
@Narretz
fix($animateCss): consider options.delay value for closing timeout
962509e 
Previously, options.delay was only considered when a class added an
extra transition style (which leads to style recalculation).

Fixes angular#13355
Closes angular#13363
@Narretz Narretz force-pushed the fix-nganimate-css-13355 branch from 962509e to 8b32e52 Compare November 26, 2015 13:07
Narretz added a commit to Narretz/angular.js that referenced this pull request Nov 26, 2015
@Narretz
fix($animateCss): consider options.delay value for closing timeout
8b32e52 
Previously, options.delay was only considered when a class added an
extra transition style (which leads to style recalculation).

Fixes angular#13355
Closes angular#13363
@petebacondarwin petebacondarwin modified the milestones: 1.4.8, 1.4.9 Nov 26, 2015
@Narretz Narretz force-pushed the fix-nganimate-css-13355 branch from 06b2530 to fb0bd2e Compare November 26, 2015 15:52
Narretz added a commit to Narretz/angular.js that referenced this pull request Nov 26, 2015
@Narretz
fix($animateCss): consider options.delay value for closing timeout
fb0bd2e 
Previously, options.delay was only considered when a class added an
extra transition style (which leads to style recalculation).

Fixes angular#13355
Closes angular#13363
@Narretz
fix($animateCss): consider options.delay value for closing timeout
7ffb2d3 
Previously, options.delay was only considered when a class added an
extra transition style (which leads to style recalculation).

Fixes angular#13355
Closes angular#13363
@Narretz Narretz force-pushed the fix-nganimate-css-13355 branch from fb0bd2e to 7ffb2d3 Compare November 26, 2015 17:16
@Narretz Narretz merged commit 7ffb2d3 into angular:master Nov 26, 2015
Narretz added a commit that referenced this pull request Nov 26, 2015
@Narretz
fix($animateCss): consider options.delay value for closing timeout
592bf51 
Previously, options.delay was only considered when a class added an
extra transition style (which leads to style recalculation).

Fixes #13355
Closes #13363
kkirsche added a commit to kkirsche/kibana that referenced this pull request Feb 21, 2016
@kkirsche
Update AngularJS
d64590c 
<a name="1.4.9"></a>
# 1.4.9 implicit-superannuation (2016-01-21)

## Bug Fixes

- **Animation**
  - ensure that animate promises resolve when the document is hidden
  ([9a60408c](angular/angular.js@9a60408))
  - do not trigger animations if the document is hidden
  ([09f6061a](angular/angular.js@09f6061),
   [elastic#12842](angular/angular.js#12842), [elastic#13776](angular/angular.js#13776))
  - only copy over the animation options once
  ([2fc954d3](angular/angular.js@2fc954d),
   [elastic#13722](angular/angular.js#13722), [elastic#13578](angular/angular.js#13578))
  - allow event listeners on document in IE
  ([5ba4419e](angular/angular.js@5ba4419),
   [elastic#13548](angular/angular.js#13548), [elastic#13696](angular/angular.js#13696))
  - allow removing classes that are added by a running animation
  ([6c4581fc](angular/angular.js@6c4581f),
   [elastic#13339](angular/angular.js#13339), [elastic#13380](angular/angular.js#13380), [elastic#13414](angular/angular.js#13414), [elastic#13472](angular/angular.js#13472), [elastic#13678](angular/angular.js#13678))
  - do not use `event.timeStamp` anymore for time tracking
  ([620a20d1](angular/angular.js@620a20d),
   [elastic#13494](angular/angular.js#13494), [elastic#13495](angular/angular.js#13495))
  - ignore children without animation data when closing them
  ([be01cebf](angular/angular.js@be01ceb),
   [elastic#11992](angular/angular.js#11992), [elastic#13424](angular/angular.js#13424))
  - do not alter the provided options data
  ([7a81e6fe](angular/angular.js@7a81e6f),
   [elastic#13040](angular/angular.js#13040), [elastic#13175](angular/angular.js#13175))
  - correctly handle `$animate.pin()` host elements
  ([a985adfd](angular/angular.js@a985adf),
   [elastic#13783](angular/angular.js#13783))
  - allow animations when pinned element is parent element
  ([4cb8ac61](angular/angular.js@4cb8ac6),
   [elastic#13466](angular/angular.js#13466))
  - allow enabled children to animate on disabled parents
  ([6d85f24e](angular/angular.js@6d85f24),
   [elastic#13179](angular/angular.js#13179), [elastic#13695](angular/angular.js#13695))
  - correctly access `minErr`
  ([0c1b54f0](angular/angular.js@0c1b54f))
  - ensure animate runner is the same with and without animations
  ([937942f5](angular/angular.js@937942f),
   [elastic#13205](angular/angular.js#13205), [elastic#13347](angular/angular.js#13347))
  - remove animation end event listeners on close
  ([d9157849](angular/angular.js@d915784),
   [elastic#13672](angular/angular.js#13672))
  - consider options.delay value for closing timeout
  ([592bf516](angular/angular.js@592bf51),
   [elastic#13355](angular/angular.js#13355), [elastic#13363](angular/angular.js#13363))
- **$controller:** allow identifiers containing `$`
  ([2563ff7b](angular/angular.js@2563ff7),
   [elastic#13736](angular/angular.js#13736))
- **$http:** throw if url passed is not a string
  ([c5bf9dae](angular/angular.js@c5bf9da),
   [elastic#12925](angular/angular.js#12925), [elastic#13444](angular/angular.js#13444))
- **$parse:** handle interceptors with `undefined` expressions
  ([7bb2414b](angular/angular.js@7bb2414))
- **$resource:** don't allow using promises as `timeout` and log a warning
  ([47486524](angular/angular.js@4748652))
- **formatNumber:** cope with large and small number corner cases
  ([9c49eb13](angular/angular.js@9c49eb1),
   [elastic#13394](angular/angular.js#13394), [elastic#8674](angular/angular.js#8674), [elastic#12709](angular/angular.js#12709), [elastic#8705](angular/angular.js#8705), [elastic#12707](angular/angular.js#12707), [elastic#10246](angular/angular.js#10246), [elastic#10252](angular/angular.js#10252))
- **input:**
  - fix URL validation being too strict
  ([6610ae81](angular/angular.js@6610ae8),
   [elastic#13528](angular/angular.js#13528), [elastic#13544](angular/angular.js#13544))
  - add missing chars to URL validation regex
  ([2995b54a](angular/angular.js@2995b54),
   [elastic#13379](angular/angular.js#13379), [elastic#13460](angular/angular.js#13460))
- **isArrayLike:** recognize empty instances of an Array subclass
  ([323f9ab7](angular/angular.js@323f9ab),
   [elastic#13560](angular/angular.js#13560), [elastic#13708](angular/angular.js#13708))
- **ngInclude:** do not compile template if original scope is destroyed
  ([9590bcf0](angular/angular.js@9590bcf))
- **ngOptions:**
  - don't skip `optgroup` elements with `value === ''`
  ([85e392f3](angular/angular.js@85e392f),
   [elastic#13487](angular/angular.js#13487), [elastic#13489](angular/angular.js#13489))
  - don't `$dirty` multiple select after compilation
  ([f163c905](angular/angular.js@f163c90),
   [elastic#13211](angular/angular.js#13211), [elastic#13326](angular/angular.js#13326))
- **select:** re-define `ngModelCtrl.$render` in the `select` directive's postLink function
  ([529b2507](angular/angular.js@529b250),
   [elastic#13583](angular/angular.js#13583), [elastic#13583](angular/angular.js#13583), [elastic#13663](angular/angular.js#13663))

## Minor Features

- **ngLocale:** add support for standalone months
  ([54c4041e](angular/angular.js@54c4041),
   [elastic#3744](angular/angular.js#3744), [elastic#10247](angular/angular.js#10247), [elastic#12642](angular/angular.js#12642), [elastic#12844](angular/angular.js#12844))
- **ngMock:** add support for `$animate.closeAndFlush()`
  ([512c0811](angular/angular.js@512c081))

## Performance Improvements

- **ngAnimate:** speed up `areAnimationsAllowed` check
  ([2d3303dd](angular/angular.js@2d3303d))

## Breaking Changes

While we do not deem the following to be a real breaking change we are highlighting it here in the
changelog to ensure that it does not surprise anyone.

- **$resource:** due to [47486524](angular/angular.js@4748652),

**Possible breaking change** for users who updated their code to provide a `timeout`
promise for a `$resource` request in version v1.4.8.

Up to v1.4.7 (included), using a promise as a timeout in `$resource`, would silently
fail (i.e. have no effect).

In v1.4.8, using a promise as timeout would have the (buggy) behaviour described
in angular/angular.js#12657 (comment).
(I.e. it will work as expected for the first time you resolve the promise and will
cancel all subsequent requests after that - one has to re-create the resource
class. This was not documented.)

With this change, using a promise as timeout in v1.4.9 onwards is not allowed.
It will log a warning and ignore the timeout value.

If you need support for cancellable `$resource` actions, you should upgrade to
version 1.5 or higher.

<a name="1.4.8"></a>
# 1.4.8 ice-manipulation (2015-11-19)

## Bug Fixes

- **$animate:** ensure leave animation calls `close` callback
  ([6bd6dbff](angular/angular.js@6bd6dbf),
   [elastic#12278](angular/angular.js#12278), [elastic#12096](angular/angular.js#12096), [elastic#13054](angular/angular.js#13054))
- **$cacheFactory:** check key exists before decreasing cache size count
  ([2a5a52a7](angular/angular.js@2a5a52a),
   [elastic#12321](angular/angular.js#12321), [elastic#12329](angular/angular.js#12329))
- **$compile:**
  - bind all directive controllers correctly when using `bindToController`
  ([5d8861fb](angular/angular.js@5d8861f),
   [elastic#11343](angular/angular.js#11343), [elastic#11345](angular/angular.js#11345))
  - evaluate against the correct scope with bindToController on new scope
  ([b9f7c453](angular/angular.js@b9f7c45),
   [elastic#13021](angular/angular.js#13021), [elastic#13025](angular/angular.js#13025))
  - fix scoping of transclusion directives inside replace directive
  ([74da0340](angular/angular.js@74da034),
   [elastic#12975](angular/angular.js#12975), [elastic#12936](angular/angular.js#12936), [elastic#13244](angular/angular.js#13244))
- **$http:** apply `transformResponse` even when `data` is empty
  ([c6909464](angular/angular.js@c690946),
   [elastic#12976](angular/angular.js#12976), [elastic#12979](angular/angular.js#12979))
- **$location:** ensure `$locationChangeSuccess` fires even if URL ends with `#`
  ([6f8ddb6d](angular/angular.js@6f8ddb6),
   [elastic#12175](angular/angular.js#12175), [elastic#13251](angular/angular.js#13251))
- **$parse:** evaluate once simple expressions only once
  ([e4036824](angular/angular.js@e403682),
   [elastic#12983](angular/angular.js#12983), [elastic#13002](angular/angular.js#13002))
- **$resource:** allow XHR request to be cancelled via a timeout promise
  ([7170f9d9](angular/angular.js@7170f9d),
   [elastic#12657](angular/angular.js#12657), [elastic#12675](angular/angular.js#12675), [elastic#10890](angular/angular.js#10890), [elastic#9332](angular/angular.js#9332))
- **$rootScope:** prevent IE9 memory leak when destroying scopes
  ([87b0055c](angular/angular.js@87b0055),
   [elastic#10706](angular/angular.js#10706), [elastic#11786](angular/angular.js#11786))
- **Angular.js:** fix `isArrayLike` for unusual cases
  ([70edec94](angular/angular.js@70edec9),
   [elastic#10186](angular/angular.js#10186), [elastic#8000](angular/angular.js#8000), [elastic#4855](angular/angular.js#4855), [elastic#4751](angular/angular.js#4751), [elastic#10272](angular/angular.js#10272))
- **isArrayLike:** handle jQuery objects of length 0
  ([d3da55c4](angular/angular.js@d3da55c))
- **jqLite:**
  - deregister special `mouseenter` / `mouseleave` events correctly
  ([22f66025](angular/angular.js@22f6602),
   [elastic#12795](angular/angular.js#12795), [elastic#12799](angular/angular.js#12799))
  - ensure mouseenter works with svg elements on IE
  ([c1f34e8e](angular/angular.js@c1f34e8),
   [elastic#10259](angular/angular.js#10259), [elastic#10276](angular/angular.js#10276))
- **limitTo:** start at 0 if `begin` is negative and exceeds input length
  ([4fc40bc9](angular/angular.js@4fc40bc),
   [elastic#12775](angular/angular.js#12775), [elastic#12781](angular/angular.js#12781))
- **merge:**
  - ensure that jqlite->jqlite and DOM->DOM
  ([2f8db1bf](angular/angular.js@2f8db1b))
  - clone elements instead of treating them like simple objects
  ([838cf4be](angular/angular.js@838cf4b),
   [elastic#12286](angular/angular.js#12286))
- **ngAria:** don't add tabindex to radio and checkbox inputs
  ([59f1f4e1](angular/angular.js@59f1f4e),
   [elastic#12492](angular/angular.js#12492), [elastic#13095](angular/angular.js#13095))
- **ngInput:** change URL_REGEXP to better match RFC3987
  ([cb51116d](angular/angular.js@cb51116),
   [elastic#11341](angular/angular.js#11341), [elastic#11381](angular/angular.js#11381))
- **ngMock:** reset cache before every test
  ([91b7cd9b](angular/angular.js@91b7cd9),
   [elastic#13013](angular/angular.js#13013))
- **ngOptions:**
  - skip comments and empty options when looking for options
  ([0f58334b](angular/angular.js@0f58334),
   [elastic#12190](angular/angular.js#12190), [elastic#13029](angular/angular.js#13029), [elastic#13033](angular/angular.js#13033))
  - override select option registration to allow compilation of empty option
  ([7b2ecf42](angular/angular.js@7b2ecf4),
   [elastic#11685](angular/angular.js#11685), [elastic#12972](angular/angular.js#12972), [elastic#12968](angular/angular.js#12968), [elastic#13012](angular/angular.js#13012))

## Performance Improvements

- **$compile:** use static jquery data method to avoid creating new instances
  ([55ad192e](angular/angular.js@55ad192))
- **copy:**
  - avoid regex in `isTypedArray`
  ([19fab4a1](angular/angular.js@19fab4a))
  - only validate/clear if the user specifies a destination
  ([d1293540](angular/angular.js@d129354),
   [elastic#12068](angular/angular.js#12068))
- **merge:** remove unnecessary wrapping of jqLite element
  ([ce6a96b0](angular/angular.js@ce6a96b),
   [elastic#13236](angular/angular.js#13236))

## Breaking Changes

Signed-off-by: Kevin Kirsche <[email protected]>
@kkirsche kkirsche mentioned this pull request Feb 21, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@matsko matsko

Labels
cla: yes component: ngAnimate frequency: low severity: broken expected use type: bug
Projects
None yet
Milestone
1.4.9
Development

Successfully merging this pull request may close these issues.
5 participants
@Narretz @gkalpak @petebacondarwin @matsko @googlebot