Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effects Rewrite #1017

Closed
wants to merge 40 commits into from

Conversation

Projects
None yet
5 participants
@mikesherov
Copy link
Member

commented Jun 26, 2013

Tasks

  • add backCompat for deprecated methods.
  • ensure blind / fold also animate the placeholder.
  • deprecate .effect('transfer'), introduce .transfer()
  • each effect should return itself for AMD.
  • .restoreStyle should remove .data()
  • .removePlaceholder shouldn't reset styles, introduce utility method.
  • Effects demo: The second call (showing) to fade is showing instantly.
  • Effects demo: Fold is broken in IE8 (vertical blind is good, but then regresses before the horizontal blind).
  • Effects demo: Highlight is broken (no effect).
  • Effects demo: Puff is broken (expands to bottom right instead of growing from center; doesn't fade).
  • Effects demo: Pulsate is broken (no effect).
  • Effects demo: Scale is broken (implodes to top right instead of center).
  • Effects demo: Slide is broken (no effect).
  • Effects demo: back to back effects aren't queueing properly for: size, scale
  • We need to look at what happens if you load an old effect definition with the new core. Does it break default modes? Anything else?
  • demos/effect/show.html, Scale is broken, throws Uncaught TypeError: Cannot call method 'call' of undefined in effect-scale.js:70.

    Discuss

  • size and scale are broken in master
  • the following items don't call show everytime first:
    • scale (calls size which does)
    • pulsate
    • puff (calls size which does)
    • fade (because it toggles)

Testing

  • re-enable createWrapper test since IE7 is no more.
  • demos in IE8+
  • unit tests pass in IE8+
  • visual tests are looking good in IE8+
  • get tests passing in "modern" browsers
  • visual tests in "modern" browsers.
  • demos in "modern" browsers.

Follow up

  • http://bugs.jqueryui.com/ticket/10606 Not a regression introduced by this PR (aka already broken): In IE8 the toggleClass demo doesn't animate the height decrease (first click), while the height increase is animated (second click).
  • http://bugs.jqueryui.com/ticket/10605 Other than transfer and size, we could simplify all effects by having the common done() function remove the placeholder (if it exists), and do the final hide based on the mode. There are a few effects which use if ( !show ) instead of if ( hide ) to determine if .hide() should be called, but those can probably all be normalized since this would only affect people explicitly passing a mode of effect when they really mean hide, which probably isn't happening. We'd need to have .createPlaceholder() store a reference to the placeholder to make this work, but that's simple.
  • file bug with core: ( subpixel jump on FF shake visual test )
  • file bug with chrome: (chrome "auto" reports "0px" for clip ) https://code.google.com/p/chromium/issues/detail?id=412921
  • file bugs with ui: deprecate backCompat'ed method, remove backcompat'ed methods
    http://bugs.jqueryui.com/ticket/10599 http://bugs.jqueryui.com/ticket/10600
@jzaefferer

This comment has been minimized.

Copy link
Member

commented Jun 26, 2013

Travis/grunt sezs: "FFFFFFFFF". At least there's no "UUUUUUUUUU".

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2013

Yup, tests are broken. They test the wrong thing. I'll fix, but wanted to get some human eyes on it.

@gnarf

This comment has been minimized.

Copy link
Member

commented Oct 16, 2013

👍 seems sane to me.

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2014

@scottgonzalez @jzaefferer ping. The last bits here are just verifying that visual and units are working in IE ( they all work in Safari, Opera, Chrome, FF), and also animating the placeholder for blind and fold.

Can you both review the code please?

@jzaefferer

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

Even when ignoring white space changes, I have a hard time reviewing this, since I know so little about the existing code. @mikesherov how about a Skype session where we review the code together? You could walk me (and Scott, if available) through the changes.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

If @mikesherov's availability is still low, I can walk you through the code some time this week. I have a decent understanding of the existing code and I've done a walk through of this code at least once with @mikesherov (though it was a long time ago).

@@ -75,20 +75,6 @@ test( "removeClass", function() {
equal( "", element[ 0 ].className );
});


/* TODO: Disabled - Can't figure out why this is failing in IE 6/7

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Determine if we can enable this now that we don't support IE 6/7.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

Well, it's testing a deprecated function anyway, but I'll see if I can enable it.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 13, 2014

Author Member

Looked into enabling it, and it seemed like it passes, but I'm still removing the test because I'd have to remove it anyway in the future.

animation = {},
show = mode === "show",
wrapper, distance, margin;
$.effects.define( "blind", "hide", function( o, done ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Need to return effect for AMD.

},

restoreStyle: function( element ) {
element[ 0 ].style.cssText = element.data( dataSpace + "style" ) || "";

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Should this clear out the data?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

It can. It doesn't need to, but I suppose it should.

// removes a placeholder if it exists and restores
// properties that were modified during placeholder creation
removePlaceholder: function( placeholder, el ) {
$.effects.restoreStyle( el );

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Asymmetric use of style saving/restoring.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

Can you explain this comment further please?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Creating the placeholder doesn't save the styles, but removing it does.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

I'll add an additional wrapper that calls both, to avoid the asymmetry.

effectMethod = $.effects.effect[ args.effect ],
defaultMode = effectMethod.mode,
modes = [],
effectPrefilter = function() {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Was this added to avoid reflows? It seems like this could just be the first thing in run().

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 12, 2014

Author Member

I recall now... this was done because storing styles need to all be done before ANY style modifications get made, but after ALL elements are shown if necessary, so that if a placeholder is made, it is the correct widht/height should any of the elements have fluid intrinsic dimensions.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Sep 13, 2014

Member

Can you add a comment so that information isn't lost?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 13, 2014

Author Member

added at call site.

@@ -1184,7 +1315,16 @@ $.fn.extend({
}
}

return queue === false ? this.each( run ) : this.queue( queue || "fx", run );
function prefilter( next ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Why not just handle queues directly in effectPrefilter()?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

There is a reason that I can't recall now O_o . I'll think about it and when I remember, I'll let you know.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 12, 2014

Author Member

I recall now... this was done because storing styles need to all be done before ANY style modifications get made, but after ALL elements are shown if necessary, so that if a placeholder is made, it is the correct widht/height should any of the elements have fluid intrinsic dimensions.


el.animate({
opacity: mode
$.effects.define( "fade", "toggle", function( o, done ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

It looks like this is right, but it's functionally broken while master works. Open the default effect demo and run $( "#effect" ).effect( "fade" ) twice. The second call (showing) is instant.

This comment has been minimized.

Copy link
@gnarf

gnarf Feb 20, 2014

Member

What happened is now that the .show() is happening in the prefilter, animate { opacity: "show" } is a noop.

This is one place where abstracting the show into the prefilter hurt.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

What do you suggest I do here?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

If we expand it to have all the logic, then it should work, right? Alternatively, we could just drop this effect. I only created it because there was no way to use fade as an effect for widgets, but the standardized show and hide options now support core's fade via show: true.

This comment has been minimized.

Copy link
@gnarf

gnarf Feb 20, 2014

Member

We need to look for other situations where the .show() happens too early now, and possibly take that abstraction out of the prefilter, or have some way to inject a "pre prefilter" :)

This comment has been minimized.

Copy link
@mikesherov

mikesherov Mar 3, 2014

Author Member

Actually, this isn't because of the early show. It's because "effect" was getting coerced to "show" too early. I have a fix incoming.

});
});
if ( placeholder ) {
placeholder.remove();

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Why does this do a direct removal instead of using the helper?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

this is the one place where we don't also want to restore styles, which the helper does.

@@ -23,10 +23,10 @@
}
}(function( $ ) {

return $.effects.effect.transfer = function( o, done ) {
$.effects.define( "transfer", function( o, done ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Should we move this out of effects and make it just .transfer() or similar?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

why? Isn't this still an effect like any other?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Not really, it only has the effect mode. It doesn't make sense to use this with show or hide; if you were doing that you'd probably want to animate the showing/hiding in addition to the transfer.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

OK, do we then still need to keep this as backCompat for a release?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Yeah, let's discuss this with the team though in case there's a better way to handle this. I think this is one of the more useful effects, but it just doesn't match the others.

var elem = $( this ),
complete = args.complete,
mode = args.mode;
var elem = $( this );

function done() {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Feb 20, 2014

Member

Other than transfer and size, we could simplify all effects by having the common done() function remove the placeholder (if it exists), and do the final hide based on the mode. There are a few effects which use if ( !show ) instead of if ( hide ) to determine if .hide() should be called, but those can probably all be normalized since this would only affect people explicitly passing a mode of effect when they really mean hide, which probably isn't happening.

We'd need to have .createPlaceholder() store a reference to the placeholder to make this work, but that's simple.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Feb 20, 2014

Author Member

Well, it actually depends on the default mode for that effect, which is now a defined thing, so yeah, we can simplify even for those.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 12, 2014

Author Member

I'd like to defer this work to a later refactor once I write a bunch of tests to gaurantee that I'm not missing anything. I'll file this as a bug to be tackled in 1.13 rather than right now, OK?

This comment has been minimized.

Copy link
@scottgonzalez
@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Feb 20, 2014

We need to look at what happens if you load an old effect definition with the new core. Does it break default modes? Anything else?

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Feb 21, 2014

Initial findings from /demos/effect/default.html:

  • Fold is broken in IE8 (vertical blind is good, but then regresses before the horizontal blind).
  • Highlight is broken (no effect).
  • Puff is broken (expands to bottom right instead of growing from center; doesn't fade).
  • Pulsate is broken (no effect).
  • Scale is broken (implodes to top right instead of center).
  • Slide is broken (no effect).
@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2014

Hmmm. Any other good test files to use, @scottgonzalez? I was using the visual test suite, but apparently that isn't nearly good enough.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Feb 24, 2014

Other than the actual test files, I think the default effect demo is the only other file to use.

@jzaefferer

This comment has been minimized.

Copy link
Member

commented Apr 24, 2014

demos/effect/show.html, Scale is broken, throws Uncaught TypeError: Cannot call method 'call' of undefined in effect-scale.js:70.

@jzaefferer

This comment has been minimized.

Copy link
Member

commented Apr 24, 2014

Not a regression introduced by this PR (aka already broken): In IE8 the toggleClass demo doesn't animate the height decrease (first click), while the height increase is animated (second click).

@scottgonzalez scottgonzalez added this to the 1.12 milestone Sep 2, 2014

@mikesherov mikesherov force-pushed the mikesherov:clip branch from 23f04a1 to c2a1ca8 Sep 3, 2014

@mikesherov mikesherov force-pushed the mikesherov:clip branch from c2a1ca8 to b0ae7f3 Sep 12, 2014

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2014

Not a regression introduced by this PR (aka already broken): In IE8 the toggleClass demo doesn't animate the height decrease (first click), while the height increase is animated (second click).

I'm not going to fix this in the effects rewrite as I don't touch any of the toggleClass stuff. Filed a bug instead: http://bugs.jqueryui.com/ticket/10606

@jzaefferer

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

@mikesherov what's the status here? There's two open issues on the task list above, are you working on those? Do you need help with testing?

@mikesherov mikesherov force-pushed the mikesherov:clip branch from 0ca0cc5 to 6ae27b7 Nov 15, 2014

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

@scottgonzalez @jzaefferer ready for rereview!

// Sentinel for duck-punching the :animated psuedo-selector
el.data( dataSpaceAnimated, true );

// save effect mode for later use,

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

capitalization

// as the .show() below destroys the initial state
modes.push( normalizedMode );

// see $.uiBackCompat inside of run() for removal of defaultMode in 1.13

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

capitalization

if ( elem.is( ":hidden" ) ? mode === "hide" : mode === "show" ) {
elem[ mode ]();
done();
// override mode option on a per element basis,

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

capitalization


if ( $.uiBackCompat !== false && !defaultMode ) {
if ( elem.is( ":hidden" ) ? mode === "hide" : mode === "show" ) {
// call the core method to track "olddisplay" properly

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

blank line, capitalization

} else {
effectMethod.call( elem[0], args, done );
if ( args.mode === "none" ) {
// call the core method to track "olddisplay" properly

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

blank line, capitalization

@@ -1243,9 +1453,78 @@ $.fn.extend({
}
});
return val;
},

// getter/setter for an object representing clip values

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Nov 24, 2014

Member

capitalization, though I'm not sure this comment is necessary

This comment has been minimized.

Copy link
@mikesherov

mikesherov Nov 25, 2014

Author Member

agreed.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

I still need to do some actual testing in the browser, but I finished looking through the changes.

@mikesherov Can you file an api.jqueryui.com issue for all the API changes and new methods? We can fill in the actual details later, but it'd be good to have a list of what we need to document.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

The height of the main element in the effect demos is now too small because of the font-size changes in master.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

Scale show doesn't animate, but showing via toggle does.

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2014

@scottgonzalez the punchline was that the supplied params in the demo weren't sane. The code was fine :-\

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2014

@jzaefferer @scottgonzalez ping for rereview

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2014

@jzaefferer note that the semantics of .show( "size", to: { width:x, height:y } ) has changed to match .toggle, such that there is no longer a magic "from zero to to behavior" for show. show behaves exactly as a .toggle that ultimately called a show would do: that is, use the to as the starting point, and end on the non-hidden size of the element.

@scottgonzalez

This comment has been minimized.

Copy link

commented on ui/effect-size.js in 1cd8fb8 Nov 26, 2014

I know this isn't a new var, but can we change the name of this?

This comment has been minimized.

Copy link
Owner Author

replied Nov 27, 2014

No problem. Will address.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Nov 26, 2014

Looks good. Just need to address the element size (see previous comment).

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

@scottgonzalez addressed the element size. Should be good for final review. /cc @jzaefferer

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

@scottgonzalez not sure what I need to document. Basically all the methods I deprecated were already not documented to begin with. Is the intent that for the effects rewrite we will document all of the helper methods I created on $.effects like $.effects.createPlaceholder and $.fn.cssClip? I'm not sure what needs to be documented. Would appreciate some insight here.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

We should document anything that an effect author would need. I'm not too worried about documenting stuff for old versions. Right now I just want a list of the method signatures in an issue on api.jqueryui.com so we make sure it gets done before the release.

@mikesherov

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2014

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

Perfect. Thanks.

@@ -950,113 +1131,109 @@ $.extend( $.effects, {
// this should be a little more flexible in the future to handle a string & hash

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Dec 10, 2014

Member

Is this comment still correct? It looks like only the formatting changed, but I doubt that we're going to change this method in the future.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Dec 10, 2014

Author Member

I left it in because I didn't write the original :-)

width: element.outerWidth(true),
height: element.outerHeight(true),
// Creates a placeholder element so that the original element can be made absolute
// also stores all modified properties on the element so they can be restored later

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Dec 10, 2014

Member

I can't tell which properties this refers to. There are modifications to element, but with the separate saveStyle method above, I'm not sure how those are related.

This comment has been minimized.

Copy link
@mikesherov

mikesherov Dec 10, 2014

Author Member

you're right. I'll remove this line

@@ -1152,48 +1329,109 @@ function standardAnimationOption( option ) {
$.fn.extend({

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Dec 10, 2014

Member

Not part of the diff, but there are a bunch of comments above that should start uppercased.

@jzaefferer

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

Tested locally and in BrowserStack on various browsers, using the visual test pages. Reviewed a few individual effects and effect.js in detail. Didn't find anything interesting except for those comments. Not sure if any of those even need to change, so this is likely good to land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.