Effect: ui-effects-wrapper now respects element's display property.

[euyuil]

A small fix to Effect. After the fix, div.ui-effects-wrapper will respect the element's CSS display property.

Try this fiddle written using jQuery UI before this fix (jQuery 1.10.2, jQuery UI 1.10.3).

[tjvantoll]

Thanks @euyuil.

This change seems reasonable to me. I think this test case shows the idea behind this a little better: http://jsfiddle.net/tj_vantoll/FxH3Y/.

Could you:

1. Create a ticket for this.
2. Sign our CLA so we can review this?

Thanks.

[euyuil]

Hi @tjvantoll,

Today I just found that there's not only display but also vertical-align should be followed by .ui-effects-wrapper. Maybe there are even more.

This fiddle for example seems tricky...

And I've just created ticket # 9477 at bugs.jqueryui.com. So what can I do to help next?

[tjvantoll]

@gnarf & @mikesherov Do either of you have any concerns with applying display and vertical-align to the .ui-effects-wrapper? This seems reasonable to me, not sure if there are any implications I'm not thinking of.

[mikesherov]

I'd defer to @gnarf here as the original author. The effects rewrite gets rid of wrapper in favor of a placeholder, so in the future this might be a moot point.

[gnarf]

Hrm... display kind of has me worried, would inline be okay for this wrapper in all cases? I'm not sure... I think generally we don't animate position on the wrapper though, it would require some digging...

vertical-align is less worrying though, 👍

[euyuil]

@mikesherov yes if in the future the effects get rid of wrapper this would be not a problem.

@gnarf, I think for divs if display is not inline-block, the style vertical-align is ignored. (Please correct me if I'm wrong :-) So if display is not gonna be followed, vertical-align is not necessary either.

[gnarf]

There are so many edge cases to worry about here that as long as you do some testing with all of the display properties using a all of the effects that use the wrapper, I'll support it - I just want to make sure it doesn't create another hidden bug in like explode or something...

[tjvantoll]

Thanks @gnarf & @mikesherov.

If a wrapper won't be used after the effects rewrite, and that's due to land in 1.11... then it's not really worth going down this route.

I do think we should leave #9477 open as this is a valid use case. If the rewrite doesn't resolve this, then we can re-investigate.

Thoughts?

[mikesherov]

Thanks again for contributing, but as discussed last year, this is a moot point given the effects rewrite, and therefore closed, as the effects rewrite is almost here.