Closed Bug 883568 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

19 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: u439718, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file Test Case (deleted) —
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.
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.
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
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
Attached patch fix (obsolete) (deleted) — Splinter Review
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?
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.
Attached patch fix v2 (deleted) — Splinter Review
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-
Attached patch scroll frame fix (deleted) — Splinter Review
... 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+
Status: NEW → RESOLVED
Closed: 11 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.
Depends on: 917060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: