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

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zenorbi3, Assigned: roc)

Tracking

({regression, testcase})

19 Branch
mozilla26
x86
All
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 763150 [details]
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.
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Updated

5 years ago
Summary: Block not showing during transition if at the end, it would be invisible → Block not showing during transition if at the end it would be invisible

Comment 2

5 years ago
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
Blocks: 795657
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
Version: 21 Branch → 19 Branch
Attachment #763150 - Attachment mime type: text/plain → text/html
Created attachment 770653 [details] [diff] [review]
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.
Assignee: nobody → roc
Attachment #770653 - Flags: review?(matspal)
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?

Updated

5 years ago
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
Actually we need a new method GetPreEffectsOverflowAreas that returns both visual and scrollable overflow before transforms or effects.
Created attachment 787196 [details] [diff] [review]
fix v2
Attachment #770653 - Attachment is obsolete: true
Attachment #770653 - Flags: review?(matspal)
Attachment #787196 - Flags: review?(matspal)
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...
Attachment #787196 - Flags: review?(matspal) → review-
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
Attachment #788130 - Flags: review?(roc)
Attachment #788130 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/072fffc8a0d1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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.)
Flags: needinfo?(matspal)
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.
Flags: needinfo?(matspal)
OK.

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

Updated

5 years ago
Depends on: 917060
You need to log in before you can comment on or make changes to this bug.