Fixed: CPViewAnimator was not giving correct frame information

[didierkorthoudt]

Replaces #2500.

With this PR, correct frame/frameSize/frameOrigin are available when animating a view.

If your animated view doesn’t implement a custom layoutSubviews or drawRect, simply set the new flag : [[aView animator] setWantPeriodicFrameUpdates:YES] (this is automatically done for you if you have at least one of the 2 custom methods implemented).

This will call setFrame / setFrameOrigin / setFrameSize accordingly to the property you're animating.

[didierkorthoudt]

CPAnimatablePropertyContainerTest does not behave correctly with this... Custom drawing and subviews animations are not correct. Digging into this...

[cacaodev]

The problem is that the transform property is inherited by subviews and by the canvas. Unless all our controls support the transform property (not tested but improbable), we need a solution for that. Either a css way to block inheritance, or invert the transform matrix. Otherwise we will have to go back to the previous code.

[didierkorthoudt]

I've just tested without subviews own animations and this is must better

[didierkorthoudt]

Do you have an idea of such a control that wouldn't support this so I can test ?

[cacaodev]

Try with a button, I'm pretty sure it will be ugly. But even if it worked, how could we handle custom layout ? We wouldn't know how to apply the transform because custom layout is a black box for us. We could infer an animation if we had the end values (that's what I did with autosize) but unfortunately we don't have that info.

[didierkorthoudt]

I'll try with a button.

Maybe it's a little bit too simple but, for what I understand, with the new organisation (using _inhibitDOMUpdates and so on), the animation acts on the view itself which then calls it's setFrameXXX and/or layoutSubviews / drawRect, using all "natural" Cappuccino system (the only thing we don't do is letting cappuccino act on the animated view DOM to avoid interfering with the CSS animation). So, naively, everything should work as usual...

No ?

[didierkorthoudt]

OK, forget about my (too) naive approach... I've tested with a button and that's horrible !.. oO

I'll adapt code from your comments and search for a solution...

[cacaodev]

There are now too different bug fixes/features in this PR. I have to ask you to keep in this one the original bug fix described in the title. This way I will be able to test it and push it quickly.

You can open a new PR for the others items :

• inhibitDomUpdates
• request frames updates
• sub animations removal

Thanks.

[didierkorthoudt]

Hi @cacaodev Well, I've checked how I could split the two but the fact of fixing the live information in FrameUpdater (top and left values, corrected by using the matrix for the translation) makes the setFrameXXX methods actually work, that is they modify the position of the view, thus interfering with the DOM CSS animation. This is the origin of the inhibitDOMUpdates flag. Indeed the sub animations removal could be considered as a separate update but is a direct consequence of the fix as, everything set, the usual layout system can work as expected and is thus redondant with sub animations. OK, I could remove the wantsPeriodicFrameUpdates flag and redo it in a new PR but is it worth the work ? Just let me know and I'll adapt.

[didierkorthoudt]

A more simple solution could be to rename the PR... ;-)

[cacaodev]

It seems I won't convince you of the benefits of incremental development although they are real (faster to commit, safer code, easier to test) ... anyway in the demo, the frame keyPath is not animated correctly. I'm seeing the animation going too far, then jumping back.

Also, the debate css animations vs. requestAnimationFrame is interesting but it definitly needs to be opened and discussed in another PR.

[didierkorthoudt]

Well, I'm already convince but we should have split code modifications sooner... For the beauty of the art, I'll try to split as you mentioned.

The behavior you observe is strange as it works here. I'll make more tests to be sure. If it jumps back, that means the CSS animation is going too far and then that translation parameters are not correct.

[didierkorthoudt]

Also, I've got performances concerns when using this in my app. The FrameUpdater is called only twice during the animation, whereas, in tests, it's called many-many times...

[didierkorthoudt]

Re-tested and no problem with the frame animation. It stops just touching the bottom of the white area. I've checked subviews + custom layout + custom drawing. FYI, layoutSubviews is called more than 70 times during the animation.

[didierkorthoudt]

Well, found something else. Strange. It worked but no more. Stay tuned.

[cacaodev]

I'm not seeing anymore the jumping back problem, sorry for this false alert, maybe a cache issue. But I see frames not updated in the following configurations:

• frameSize + customDrawing
• frameSize + has Subviews + custom layout
• frameSize + has Subviews + auto layout (means autosize)
• frame + has subviews + autosize

On Safari 10.0.1

[didierkorthoudt]

Yes, that's what I see now also (same browser). Something must have interfere... I dig.

[didierkorthoudt]

OK, done some cleaning in my env.

So : for frame+has subviews+auto layout : we must set wantsPeridicFrameUpdates to YES (that's fine according to our new policy).

For all frameSize, I forgot to impact a fix and an error message should be in the console arguing that undefined blah blah blah (that is because when only animating frame size, there's no matrix, so we can't split nothing...). Fixed. I push a commit for this.

I continue to test (also for my performances problem).

[didierkorthoudt]

One problem remains : frameSize + custom drawing. Strange.

[didierkorthoudt]

• frame & custom drawing. Size changes are no more done.

[didierkorthoudt]

OK. Found. As when animating frame size (width & height) I reverted to your property animation (because of the side effects of using scaling within the matrix), I can't anymore inhibit DOM updates on setFrameSize (but still on setFrameOrigin). I go this way, I push you a commit (so you can test that everything is working) and try to split all those adaptations into 2 separate PR's.

[cacaodev]

With your commit the first 2 items i mentioned are ok. Autosize (checkbox "has Subviews" + "Auto Layout") is still not working for frame &frameSize keyPaths. There is also an issue if you select backgroundColor + frame keypaths + custom drawing.

[didierkorthoudt]

For frame+has subviews+auto layout : we must set wantsPeridicFrameUpdates to YES (that's fine according to our new policy). I've tested this and it works.

    if (leftView)
    {
        [leftView setFrame:CGRectMake(0, 0, 200, 200)];
        [animationSandbox addSubview:leftView];
        [[leftView animator] setWantsPeriodicFrameUpdates:YES];
    }

[didierkorthoudt]

Regarding the backgroundColor + frame keypaths + custom drawing problem (no background color animation), when I test on your version, it doesn't animate background color either... (no need to check frame keypaths, this is also wrong with only background color animation + custom drawing). I wonder if this is not "normal" (or logical) as custom drawing should be responsible to set the background, no ?

[cacaodev]

I'm not seeing a lack of backgroundColor animation; it seems the two animations have different frame sizes. If you say there is no regression, we'll see that later.

[cacaodev]

For frame+has subviews+auto layout : we must set wantsPeridicFrameUpdates to YES (that's fine according to our new policy). I've tested this and it works.

It may work like this but it's a deviation from cocoa where you can animate a control with one line [[control animator] setFrame:aFrame] This won't work in cappuccino if the control contains autosized view (most of them do).

It also break current code and creates an inconsistency with views governed by custom layout where this call is not needed.

[didierkorthoudt]

I was also wondering if we shouldn't fire FrameUpdate in any case... This will be consistent accross various view types... What do you think ? In fact this resumes in removal of wantsPeriodicFrameUpdates and always assume yes...

[didierkorthoudt]

I'm not seeing a lack of backgroundColor animation; it seems the two animations have different frame sizes. If you say there is no regression, we'll see that later.

I've redone the test and selecting background color animation + custom drawing doesn't animate the color.

I'm pretty sure (but won't bet my head on this) I'm on the latest build. Could you please check this ?

If it's working for you, I'll create a fresh install of Cappuccino.

[cacaodev]

I could not reproduce the backgroundColor problem it, maybe I was on the wrong build, sorry for that.

When the others change request are commited in your branch, this PR will be ready to go, with the exception of the removal of the css sub-animations feature which can be discussed in a distinct PR.

[didierkorthoudt]

OK, I'll split this PR. I'll adapt this one and reinsert the code for subviews.

One question remains : can I remove the wantsPeriodicFrameUpdates and assume it's always the case if this is a frameXXX key path ?

[cacaodev]

We discussed that before. IMO, the default frameOrigin animation should not be potentially degraded just to get an information it may not use.

[didierkorthoudt]

OK, that was hard but here we go : I've put back subviews animations (plus the fixes requested).

I think I've tested everything with the CPAnimatablePropertyContainerTest (adding [[leftView animator] setWantsPeriodicFrameUpdates:YES] only to test if frameOrigin can provide live info.

I now open a new PR.

[cacaodev]

Great, one last thing: can you try to re-enable _inhibitDOMUpdates just for the DOM element sizing but not for the canvas sizing. I think the lack of canvas sizing was causing the problem in the configuration frameSize + custom drawing. It would mean adding the condition on CPView L1377.

[didierkorthoudt]

Sorry but I don't understand what you want me to do... (maybe because it's Sunday morning... ;-) ) Also, I don't see any problem with frameSize + custom drawing...

[cacaodev]

Just to change and test -_setDisplayServerSetStyleSize: with this implementation:

- (void)_setDisplayServerSetStyleSize:(CGSize)aSize
{
#if PLATFORM(DOM)
    var scale = [self scaleSize];

    if (!_inihibitDOMUpdates)
        CPDOMDisplayServerSetStyleSize(_DOMElement, aSize.width * 1 / scale.width, aSize.height * 1 / scale.height);

    if (_DOMContentsElement)
    {
        CPDOMDisplayServerSetSize(_DOMContentsElement, aSize.width * _highDPIRatio * 1 / scale.width, aSize.height * _highDPIRatio * 1 / scale.height);
        CPDOMDisplayServerSetStyleSize(_DOMContentsElement, aSize.width * 1 / scale.width, aSize.height * 1 / scale.height);

        _needToSetTransformMatrix = YES;
    }
#endif
}

[cacaodev]

Very last thing: setWantsPeriodicFrameUpdates:NO does not make sense. The naming is not consistent with the current behavior and we don't want to. For frame&frameSize keypath, frame updates are needed in order to work normally. If you agree, we an rename it to requestPeriodicFrameUpdates with no argument. I can do this after your commit if you want.

[didierkorthoudt]

Regarding the setWantsPeriodicFrameUpdates:NO, I don't agree with you (for once... ;-) ). You may have a view that needs to get x/y info on setFrameOrigin animation in some circumstances and don't in other (for performance). I have such a view in my own app : when the view is "linked" to its "parent" (with a drawn link), I need x/y coordinates while animating to update the link. But this view can be "detached" from its parent and then, I don't need to update the link while animating. What do you think ?

[cacaodev]

OK then. It would be great to update the demo to show this: a checkbox that enables/disables a line between the 2 views and a periodic frames request.

[cacaodev]

Merged, thanks !

[cappbot]

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.