883568 - Block not showing during transition if at the end it would be invisible

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u439718]

Attachment: Test Case

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

See Test Case.

Preparation:
Make a div, that has overflow:hidden, this is the container.
Make a new div inside that, that also has overflow:hidden, this is the slide.
Make any element inside that slide, which is visible.

When animating:
Apply left:-100% to the slide, and transform:translateX(100%).
Apply transition to the transform.
Remove the transform:translateX(100%), so the transition happens from 100% to 0%

Real world example:
A 100% wide slider.


Actual results:

The slide didn't render during transition.


Expected results:

Have the slide slide out.

Comment 1

[u439718]

I know this is a weird approach, but I use my javascript animator to have javascript based animation, and have animation using css transitions. And in that, I also have support for translating left, right, top and bottom to transforms, so they are much faster in mobile browsers. The animator takes a function for the final values, so it can be used with a layout function to lay the divs out, and have the animator animate.

Simplified code:
element1.style.left="0%";
element2.style.left="100%";
animate([element1,element2],["left"],duration,easing,function() {
    element1.style.left="-100%";
    element2.style.left="0%";
},callback);

So this is not some "never used in real life" case.

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/5f4a6a474455
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121015110147
Bad:
http://hg.mozilla.org/mozilla-central/rev/8f145599e4bf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121016030544
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5f4a6a474455&tochange=8f145599e4bf


Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/65652fcb58dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121014215409
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/256882fd63c7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121014220208
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=65652fcb58dc&tochange=256882fd63c7

In local build
Last Good: 9edabc0ddc99
First Bad: fa3a84d645fc

Regressed by:
fa3a84d645fc	Robert O'Callahan — Bug 795657. Don't reframe for adding a transform when absolute descendants are present, when the frame is already positioned. r=bz

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

The bug is that, because we weren't transformed before the style change, there's no PreTransformBBoxProperty so UpdateOverflow doesn't do anything --- but it needs to.

Another way to fix this bug would be to have the AddOrRemoveTransform style change hint turn into a Reflow hint when we decide not to reframe. This would force the PreTransformBBoxProperty to be created. However, this would be slower. My patch here may slow something down because it causes more FinishAndStoreOverflow calls and forces walking up the tree, but I think that's OK. Also, fixing the code this way should get us closer to being able to use UpdateOverflow for non-transform uses.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 770653 [details] [diff] [review]
fix

(sorry for the late review)

A couple of nits:
The doc comment for OverflowChangedTracker::AddFrame may need to be
updated with this change.

Since "updateParent = true" now is unconditional in the 
"if (entry->mInitial)" block, the "if (!updateParent)" that follows
can now be an "else", right?  and the "|| entry->mInitial" removed.

This fix may be right, but I don't understand this bug fully --
which UpdateOverflow method is it that "doesn't do anything",
and why is it OK to pass in the border box as overflow areas to
FinishAndStoreOverflow here? how will that be corrected later?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren (:mats) from comment #4)
> Since "updateParent = true" now is unconditional in the 
> "if (entry->mInitial)" block, the "if (!updateParent)" that follows
> can now be an "else", right?  and the "|| entry->mInitial" removed.

Yes.

> This fix may be right, but I don't understand this bug fully --
> which UpdateOverflow method is it that "doesn't do anything",

I said that totally wrong. What I meant was, because there's no PreTransformBBox property, we don't call FinishAndStoreOverflow or call UpdateOverflow, and *that* is wrong.

> and why is it OK to pass in the border box as overflow areas to
> FinishAndStoreOverflow here? how will that be corrected later?

Good question. We should actually be using GetOverflowAreas instead.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually we need a new method GetPreEffectsOverflowAreas that returns both visual and scrollable overflow before transforms or effects.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 787196 [details] [diff] [review]
fix v2

This doesn't look right to me, I think the current code is correct.
Passing in the frame's current overflow areas to FinishAndStoreOverflow
is wrong because they contain the effects that were applied during
FinishAndStoreOverflow at the last reflow of the frame itself.
(we had a bug about that in the past where the outline area grew for
each style change, so I'm a bit surprised this patch passed tests)
The areas passed in to FinishAndStoreOverflow should only contain
the overflow contribution from the children, and the
PreTransformOverflowAreasProperty is the only case were we actually
know that without re-calculating it by unioning the child areas.
(the "if (pre)" block here is just an optimization if you will)

The real bug seems to be in the UpdateOverflow method for scroll
frames...

Comment 9

[Mats Palmgren (inactive)]

Attachment: scroll frame fix

... which doesn't actually do anything in the overflow:hidden case.
The comment there is bogus -- all frames can have overflow areas even
when there is no child overflow, namely due to the effects applied
in FinishAndStoreOverflow.  So we need to call the base class
UpdateOverflow here, which lands in nsFrame::UpdateOverflow which
re-calculates the child overflow and calls FinishAndStoreOverflow
correctly:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#5130

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/072fffc8a0d1

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/072fffc8a0d1

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren (:mats) from comment #9)
> Created attachment 788130 [details] [diff] [review]
> scroll frame fix
> 
> ... which doesn't actually do anything in the overflow:hidden case.
> The comment there is bogus -- all frames can have overflow areas even
> when there is no child overflow, namely due to the effects applied
> in FinishAndStoreOverflow.  So we need to call the base class
> UpdateOverflow here, which lands in nsFrame::UpdateOverflow which
> re-calculates the child overflow and calls FinishAndStoreOverflow
> correctly:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#5130

So the comment is wrong, but I'd still like to understand why the code is wrong.  How does the overflow area of a scroll frame *change* as a the result of a change to one of its descendants?

(I could imagine that happening because of 3-D transforms and preserve-3d, perhaps, but I don't see any 3-D transforms in this testcase.)

Comment 13

[Mats Palmgren (inactive)]

The scroll frame's own overflow areas are not affected by an overflow change
on its descendants (since they are clipped as the comment said).  But its
overflow can change as a result of a style change on the element itself so
we need to redo the FinishAndStoreOverflow etc as nsFrame::UpdateOverflow does.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK.

It's a little ugly that it assumes (for the nsXULScrollFrame case) that there's no nsBoxFrame::UpdateOverflow, though.