While testing Fennec, I found the instant snapping of the sidebars to be a bit off-putting. With the arguable trend nowadays of applications making everything in the UI go whoosh, I found it easy to make an animation out of this snapping. Just apply it and partly drag out each sidebar to test it. I think mfinkle is the best person to review, but I haven't done any Fennec-specific work until now.

While this works ok on the desktop, it's not smooth on devices. We have a bit of work to do in gecko to make animating elements work better/faster. I'm going to drop the review? flag for now. We can revisit animation after releasing Fennec 1.0 Thanks for the patch, I hope we can use it soon.

I'm working on getting sidebar snapping smooth on device now. I hope it's okay if I take this bug. I've taken the approach of using CSS animations, which seems promising but it is going to need some platform changes. Right now, we issue a reflow for any change in moz transition. This works smoothly on desktop but has issues on my Nexus S. I've managed to get the transition to work rather smoothly by removing the reflow (which I know breaks things) and adding a beginning 200msec delay. Things to consider from here: 1) from my testing it looks like we will need to be smarter about when we reflow. Not reflowing during an animation might be something worth looking into. Perhaps dbaron has some ideas. 2) we need some instrumentation on what is causing the initial pause that makes the animation choppy if we don't delay the animation. 3) we can take this a different direction and use scrollTo on a timer like kinetic panning does.

A test build is available here: http://people.mozilla.org/~bstover/fennec-transition.apk

Comment on attachment 530698 [details] [diff] [review] Snapping the sidebars should be animated Hey David, See comment above. Do you have any ideas on how we could reduce the reflows done on a moz-transform transition? I noticed a significant improvement on device by just commenting out the reflowframe hint for changing the transform.

We currently need to reflow to update overflow areas; I'd like to add a method for updating overflow areas separate from reflow to avoid this problem; I think we have bugs on it.

David, do you have the bug #? Is this something you will be tackling in the Firefox 6 timeframe? Is it something I could help with?

Comment on attachment 534827 [details] [diff] [review] WIP with updating overflow rect It looks like FinishAndStoreOverflow applies all the necessary transformations, so I just restored the pre-transformed overflows. This isn't working though... For my tests, I see gray after one sidebar transition. Daniel Holbert suggested I look at how reflow changed the frames and make sure my shortcut did the same thing. As far as I can tell, this code does the same thing to the overflow rects as reflow does for my test case. David, do you have any advice?

We discussed this in front of a whiteboard this morning. A few notes, though somewhat revised from this morning (based on thinking more about it while I'm writing this, including some important bits about handling scroll frames that I forgot): The key flaw in the patch is failure to propagate overflow areas up to ancestors. I had been thinking I'd like to be able to do general updating of overflow areas in response to dynamic changes. But for changes to transforms (and probably also changes to absolute positioning) we can add a hint that's a good bit less work to do, and probably all we'll ever need. The hint, when given on a frame, would mean that its affect on its ancestor's overflow area (including the conversion of its pre-transform to post-transform overflow area) needs to be updated, but that its overflow area relative to its own bounds doesn't require any updating. We could handle this hint by: 1. recomputing the pre-transform to post-transform overflow area on the target 2. walking up the ancestor chain, for each ancestor: a. walk all its kids and accumulate their overflow areas, and also add in the overflow area from the bounds and from any native appearance, as we do in reflow b. merge them together and finish the overflow area computation - if the overflow areas didn't change, break out of the loop c. if the ancestor in question is a scrolled frame, call some method on its scroll frame to handle the update and then break out of the loop This presumes (which we'd need to check) that there aren't any special cases in how any *containers* compute overflow. (There certainly are special cases for a bunch of leaves, e.g., for text frame.) As bug 504247 points out, there's also a special case needed for blocks where we'd need to update the overflow areas for lines.

Comment on attachment 534898 [details] [diff] [review] WIP with updating overflow rect Hey David, Thanks for all the help so far. This is another WIP patch, but this time it renders correctly for my case! So things I need to know before we begin to do a more appropriate patch: 1) Is it OK to do this overflow calculation in the CSS constructor or where should this live? 2) What is the difference between nsBox and nsBoxFrame? I was surprised to see the only class they had in common was nsIFrame. 3) I assume we'll need to do something similar for HTML frames. Where is the common-case code for that? Hopefully both HTML and XUL frames can use the same method, and then we can refactor the individual reflow paths to use that method for calculating overflow in the common case. 4) You mentioned there may be some container frame special cases for overflow rects. I'm not really sure how to go about finding those, but I think I'll understand a lot more once I know where to do this for HTML frames.

(In reply to comment #13) > 2) What is the difference between nsBox and nsBoxFrame? I was surprised to > see the only class they had in common was nsIFrame. Every frame is an nsBox. That's sort of a legacy thing :-(. nsBoxFrame is the frame type for XUL container frames. (In reply to comment #11) > c. if the ancestor in question is a scrolled frame, call some method on > its scroll frame to handle the update and then break out of the loop If the scrollable overflow area for a scrolled frame changes, then we can just do a FrameNeedsReflow on the scrolled frame to ensure that the scrollframe's scrollbars get updated ... except when the scrollbar styles returned by nsIScrollableFrame::GetScrollbarStyles() are HIDDEN and the horizontal and vertical scroll offsets are both zero; in that case we don't have to do anything since we know no scrollbar state needs to change, nor do we have to change the scroll position (sometimes when the scrollable overflow area decreases, we need to scroll up or left).

Comment on attachment 534898 [details] [diff] [review] WIP with updating overflow rect A few naming changes: nsChangeHint_NeedOverflow -> nsChangeHint_UpdateOverflow PreTransformBBoxScrollableProperty -> PreTransformScrollableOverflowProperty (in nsCSSFrameConstructor.cpp) overflowFrame -> ancestor In nsCSSFrameConstructor.cpp, you should not be calling frame->SetRect(). I think you should just implement OverflowChanged on nsIFrame (put the implementation in nsFrame.cpp) rather than making it pure virtual. (It still needs to be virtual to implement the handling for scrollframes.) In OverflowChanged, you should use the nsIFrame child list methods, and you should iterate over all child lists. (Search for callers of GetAdditionalChildListName to see how.) You also need to implement the handling of scrollframes, probably per roc's comment. You'll need to override the method on both nsHTMLScrollFrame and nsXULScrollFrame, probably forwarding both to a helper method on nsGfxScrollFrameInner (their mInner, for code shared between them). I also think you should reverse the meaning of the boolean return value -- make it return whether its own overflow changed. (And it should have a comment saying that it should get called when a child's overflow changed.) The |differentOverflow| check is wrong, since FinishAndStoreOverflow changes the overflow areas in various ways. You really need to compare the old overflow areas to the new ones *after* FinishAndStoreOverflow has done its thing. You should also remove the ComputesOwnOverflowArea() check. That's just an ugly piece of glue between XUL box layout and non-box reflow, so we don't compute overflow areas twice. You should also remove the DoesClipChildren() check once you add an override of the method on both classes of scroll frame (in nsGfxScrollFrame.cpp).

> If the scrollable overflow area for a scrolled frame changes, then we can > just do a FrameNeedsReflow on the scrolled frame to ensure that the > scrollframe's scrollbars get updated ... except when the scrollbar styles > returned by nsIScrollableFrame::GetScrollbarStyles() are HIDDEN and the > horizontal and vertical scroll offsets are both zero; in that case we don't > have to do anything since we know no scrollbar state needs to change, nor do > we have to change the scroll position (sometimes when the scrollable > overflow area decreases, we need to scroll up or left). If I understand this right, the whole point is to avoid FrameNeedsReflow :(

Well, you only need to do FrameNeedsReflow on the scrolled frame, not on the descendant(s) whose overflow areas changed. That's a win. And the rest of comment #14 explains how to avoid doing FrameNeedsReflow for overflow:hidden elements.

> A few naming changes: > nsChangeHint_NeedOverflow -> nsChangeHint_UpdateOverflow Done. > PreTransformBBoxScrollableProperty -> > PreTransformScrollableOverflowProperty Done. > (in nsCSSFrameConstructor.cpp) overflowFrame -> ancestor Done. > In nsCSSFrameConstructor.cpp, you should not be calling frame->SetRect(). Yep, done. > I think you should just implement OverflowChanged on nsIFrame (put > the implementation in nsFrame.cpp) rather than making it pure virtual. > (It still needs to be virtual to implement the handling for scrollframes.) Done. > In OverflowChanged, you should use the nsIFrame child list methods, > and you should iterate over all child lists. (Search for callers > of GetAdditionalChildListName to see how.) Thanks. Done. > You also need to implement the handling of scrollframes, probably per > roc's comment. You'll need to override the method on both nsHTMLScrollFrame > and nsXULScrollFrame, probably forwarding both to a helper method on > nsGfxScrollFrameInner (their mInner, for code shared between them). Yes. Done. > I also think you should reverse the meaning of the boolean return value -- > make it return whether its own overflow changed. (And it should have > a comment saying that it should get called when a child's overflow changed.) Good idea. Done. > The |differentOverflow| check is wrong, since FinishAndStoreOverflow > changes the overflow areas in various ways. You really need to compare > the old overflow areas to the new ones *after* FinishAndStoreOverflow has > done its thing. Good point! Done. > You should also remove the ComputesOwnOverflowArea() check. That's > just an ugly piece of glue between XUL box layout and non-box reflow, so > we don't compute overflow areas twice. Done. > You should also remove the DoesClipChildren() check once you add an > override of the method on both classes of scroll frame (in > nsGfxScrollFrame.cpp). Done. This patch is now (seemingly) working perfectly for me. I am now storing the entirety of the overflow areas before they are touched at all by FinishAndStoreOverflow, just to make sure I was getting the exact results through the "normal" path, aside from the updated transform. For my purposes, I never see the scroll frame needing to reflow anyway (yay!), so I left out roc's optimization above for now, with a comment.

Hmm, I seem to be missing some file changes I made... Going to lunch, but will look at it afterwards.

Updated patch with missing file, but sadly it means that I wasn't properly testing earlier. Now, scroll frames do end up needing to be reflowed for me. :( Doing an early return in the scroll frame creates painting problems. David, I could still use a review on what I've done so far. Maybe I'm doing something wrong.

Updated with gfx scroll frame optimization. Still hoping for a comment on how correct this patch looks.

OK! The problem was that the overflow rect was already transformed by the time we got to invalidating the old area. This looks good for Fennec now.

Comment on attachment 539655 [details] [diff] [review] Recompute overflow without reflowing for transforms Review of attachment 539655 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +4589,5 @@ > + nsIFrame* child = GetFirstChild(childList); > + while (child) { > + nsOverflowAreas childOverflow = > + child->GetOverflowAreas() + child->GetPosition(); > + overflowAreas.UnionWith(childOverflow); You should be calling ConsiderChildOverflow. @@ +4596,5 @@ > + childList = GetAdditionalChildListName(listIndex++); > + } while (childList); > + > + nsOverflowAreas oldOverflow = GetOverflowAreas(); > + FinishAndStoreOverflow(overflowAreas, GetSize()); There's a problem here, which is that some reflow implementations do something special to compute the overflow area, like nsBlockFrame::ComputeOverflowAreas, so their overflow areas could be set incorrectly here. I'm afraid some tedious auditing of callers to FinishAndStoreOverflow is needed, and you may need to factor out some code into another virtual function you can call. ::: layout/generic/nsGfxScrollFrame.cpp @@ +3277,5 @@ > + return changed; > + } > + > + nsIScrollableFrame* sf = do_QueryFrame(mOuter); > + ScrollbarStyles ss = sf->GetScrollbarStyles(); Just call this->GetScrollbarStyles() directly, that's all the outer frame does. @@ +3281,5 @@ > + ScrollbarStyles ss = sf->GetScrollbarStyles(); > + if (ss.mVertical == NS_STYLE_OVERFLOW_HIDDEN && > + ss.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN && > + GetScrollPosition() == nsPoint()) { > + // No need to overflow in this special case. There are no scrollbars and we "No need to reflow" @@ +3291,5 @@ > + nsPresContext* presContext = mOuter->PresContext(); > + presContext->PresShell()->FrameNeedsReflow( > + mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); > + > + return changed; I think you can always return false here. The overflow ::: layout/generic/nsGfxScrollFrame.h @@ +494,5 @@ > } > virtual PRBool IsScrollingActive() { > return mInner.IsScrollingActive(); > } > + PRBool OverflowChanged() { virtual @@ +732,5 @@ > } > virtual PRBool IsScrollingActive() { > return mInner.IsScrollingActive(); > } > + PRBool OverflowChanged() { virtual ::: layout/generic/nsIFrame.h @@ +1739,5 @@ > /** > + * Indicates that an overflow rect of this frame's children has changed. > + * @return PR_TRUE iff the overflow of this frame has changed. > + */ > + virtual PRBool OverflowChanged(); Call it UpdateOverflow to indicate that this overflow areas of this frame should be modified if necessary. ::: layout/style/nsStyleStruct.cpp @@ +2149,3 @@ > */ > if (mTransform != aOther.mTransform) > + NS_UpdateHint(hint, nsChangeHint_UpdateOverflow); Include nsChangeHint_UpdateTransformLayer, surely? How would this work otherwise?

(In reply to comment #24) > ::: layout/generic/nsFrame.cpp > @@ +4589,5 @@ > > + nsIFrame* child = GetFirstChild(childList); > > + while (child) { > > + nsOverflowAreas childOverflow = > > + child->GetOverflowAreas() + child->GetPosition(); > > + overflowAreas.UnionWith(childOverflow); > > You should be calling ConsiderChildOverflow. Moved ConsiderChildOverflow to nsIFrame to do this. > > @@ +4596,5 @@ > > + childList = GetAdditionalChildListName(listIndex++); > > + } while (childList); > > + > > + nsOverflowAreas oldOverflow = GetOverflowAreas(); > > + FinishAndStoreOverflow(overflowAreas, GetSize()); > > There's a problem here, which is that some reflow implementations do > something special to compute the overflow area, like > nsBlockFrame::ComputeOverflowAreas, so their overflow areas could be set > incorrectly here. I'm afraid some tedious auditing of callers to > FinishAndStoreOverflow is needed, and you may need to factor out some code > into another virtual function you can call. OK, I can definitely work on nsBlockFrame. I'm currently looking through everything that implements ::Reflow to do this audit; should I be looking for anything else? I'll have a follow-up comment with specific questions. > > ::: layout/generic/nsGfxScrollFrame.cpp > @@ +3277,5 @@ > > + return changed; > > + } > > + > > + nsIScrollableFrame* sf = do_QueryFrame(mOuter); > > + ScrollbarStyles ss = sf->GetScrollbarStyles(); > > Just call this->GetScrollbarStyles() directly, that's all the outer frame > does. Ah, I assume you mean GetScrollbarStylesFromFrame? Done. > > @@ +3281,5 @@ > > + ScrollbarStyles ss = sf->GetScrollbarStyles(); > > + if (ss.mVertical == NS_STYLE_OVERFLOW_HIDDEN && > > + ss.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN && > > + GetScrollPosition() == nsPoint()) { > > + // No need to overflow in this special case. There are no scrollbars and we > > "No need to reflow" Done. > > @@ +3291,5 @@ > > + nsPresContext* presContext = mOuter->PresContext(); > > + presContext->PresShell()->FrameNeedsReflow( > > + mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); > > + > > + return changed; > > I think you can always return false here. The overflow Incomplete thought? But this makes sense. If we schedule a reflow we might as well stop going up the ancestor chain. > > ::: layout/generic/nsGfxScrollFrame.h > @@ +494,5 @@ > > } > > virtual PRBool IsScrollingActive() { > > return mInner.IsScrollingActive(); > > } > > + PRBool OverflowChanged() { > > virtual > > @@ +732,5 @@ > > } > > virtual PRBool IsScrollingActive() { > > return mInner.IsScrollingActive(); > > } > > + PRBool OverflowChanged() { > > virtual Done. > > ::: layout/generic/nsIFrame.h > @@ +1739,5 @@ > > /** > > + * Indicates that an overflow rect of this frame's children has changed. > > + * @return PR_TRUE iff the overflow of this frame has changed. > > + */ > > + virtual PRBool OverflowChanged(); > > Call it UpdateOverflow to indicate that this overflow areas of this frame > should be modified if necessary. Done. > > ::: layout/style/nsStyleStruct.cpp > @@ +2149,3 @@ > > */ > > if (mTransform != aOther.mTransform) > > + NS_UpdateHint(hint, nsChangeHint_UpdateOverflow); > > Include nsChangeHint_UpdateTransformLayer, surely? How would this work > otherwise? Yes, this was a casualty from a code cleanup. Done.

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLeafFrame.cpp#81 I'm assuming that anything that doesn't have children will never see an overflow change from its children. :) Does everything that is a leaf derive from this? If so, we can override to give a warning if this is ever called on a child frame. http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#627 This is a leaf frame, yes? So I think the "no children" argument applies? http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#185 It shouldn't be possible to transform the first letter of a frame or any of its children. --- I'm going to pause here because I'm not entirely confident I'm going about this the right way. It's looking like for a lot of cases we can say "this will never get called", and perhaps just in case fire a warning and reflow. We can add a helper to do this in nsIFrame (um, UnexpectedUpdateOverflow?), and have these frames call this method specially.

Hm, it looks like any computation of nsOverflowAreas is a better place to start: http://mxr.mozilla.org/mozilla-central/search?string=nsOverflowAreas&find=layout

So, for a start, I think the special-case code in nsBlockFrame is really a special-case fix for a bug that we should have fixed in the general case, and is probably much easier to fix in the general case now than it once was. In particular, we should just consider all margins as part of scrollable overflow (but not visual overflow). I think that's the intent of what nsBlockFrame does differently. (This probably requires an audit to make sure that we're ok with scrollable overflow being larger than visual overflow.) Second, I tend to think probably roc or I should do the audit for other similar things; I think asking somebody new to the layout code to do that probably isn't a great idea (both because it's a lot harder and because it's easier to get wrong). I could probably do this soon, but probably not this week. roc, does this seem reasonable to you?

The special exceptions I see for nsBlockFrame: 1. The bottom margin is not added in children overflow rects, so it must be done for the parent. Why doesn't this logic apply to other margins? Having this margin outside of the child overflow makes OverflowChange not work for us. The simplest solution is to cache this calculation and include it when overflows need updating. 2. Likewise, the bottom padding needs to be applied here. Where are the other paddings applied? In the lines overflow? We could cache this too. 3. Bullet overflows are sometimes not applied by the lines so it is done explicitly. I'm assuming bullets are normal children of the frame. This should be covered by UpdateOverflow() I think? 4. Of course, this function uses lines to calculate the overflow instead of iterating through children. I'm not sure what the difference is here, but I assume it takes things like margins and padding and borders into account? This isn't something UpdateOverflow() for the current frame. 5. ocBounds is unioned in if there is a previous frame in flow. I don't know what this means exactly, but it looks like it just iterates through a subset of the child frames which is already covered by UpdateOverflow(). 6. fcBounds is unioned, which I believe is already covered by UpdateOverflow().

(In reply to comment #25) > > @@ +3291,5 @@ > > > + nsPresContext* presContext = mOuter->PresContext(); > > > + presContext->PresShell()->FrameNeedsReflow( > > > + mOuter, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); > > > + > > > + return changed; > > > > I think you can always return false here. The overflow > > Incomplete thought? But this makes sense. If we schedule a reflow we might > as well stop going up the ancestor chain. I was going to observe that scrollframes clip their contents, so changes to the overflow of the scrolled content can't affect the overflow of the scrollframe. However, your observation is also correct.

(In reply to comment #28) > So, for a start, I think the special-case code in nsBlockFrame is really a > special-case fix for a bug that we should have fixed in the general case, > and is probably much easier to fix in the general case now than it once was. > In particular, we should just consider all margins as part of scrollable > overflow (but not visual overflow). I think that's the intent of what > nsBlockFrame does differently. I agree. > Second, I tend to think probably roc or I should do the audit for other > similar things; I think asking somebody new to the layout code to do that > probably isn't a great idea (both because it's a lot harder and because it's > easier to get wrong). I could probably do this soon, but probably not this > week. > > roc, does this seem reasonable to you? Yes, except that I'm not eager to do the audit myself :-) (In reply to comment #29) > 1. The bottom margin is not added in children overflow rects, so it must be > done for the parent. Why doesn't this logic apply to other margins? nsBlockFrame includes left and top margins when computing the positions of lines. Right margins may in fact be an existing bug. > Having > this margin outside of the child overflow makes OverflowChange not work for > us. The simplest solution is to cache this calculation and include it when > overflows need updating. I think David's right that we should just include margins in the scrollable overflow area of a frame. Then the special code in block frames can go away. > 2. Likewise, the bottom padding needs to be applied here. Where are the > other paddings applied? In the lines overflow? We could cache this too. The bottom padding is included in the border-box so we shouldn't need to add that into the scrollable overflow area explicitly, since we always include the border-box. > 3. Bullet overflows are sometimes not applied by the lines so it is done > explicitly. I'm assuming bullets are normal children of the frame. This > should be covered by UpdateOverflow() I think? Yes. > 4. Of course, this function uses lines to calculate the overflow instead of > iterating through children. I'm not sure what the difference is here, but I > assume it takes things like margins and padding and borders into account? > This isn't something UpdateOverflow() for the current frame. I don't think the lines are contributing anything to the overflow areas that the frames don't know about (assuming you traverse the float and abs-pos child lists, which you do). > 5. ocBounds is unioned in if there is a previous frame in flow. I don't know > what this means exactly, but it looks like it just iterates through a subset > of the child frames which is already covered by UpdateOverflow(). Yes. > 6. fcBounds is unioned, which I believe is already covered by > UpdateOverflow(). Yes.

Can someone point me in the direction I need to go? Where do margins get added to overflow rects? Also, what is the difference between scrollable overflow and visual overflow? Why were margins not included in scrollable overflow before?

(In reply to comment #32) > Can someone point me in the direction I need to go? Where do margins get > added to overflow rects? It should be a relatively straightforward change in ConsiderChildOverflow, but it probably belongs in a separate patch (ideally underneath this one). I would expect a few tests would likely need adjusting. > Also, what is the difference between scrollable overflow and visual > overflow? Why were margins not included in scrollable overflow before? Mainly because we only started distinguishing between the two relatively recently.

(In reply to comment #33) > (In reply to comment #32) > > Can someone point me in the direction I need to go? Where do margins get > > added to overflow rects? > > It should be a relatively straightforward change in ConsiderChildOverflow, > but it probably belongs in a separate patch (ideally underneath this one). > I would expect a few tests would likely need adjusting. Wouldn't we add the margin to the scrollable overflow area in FinishAndStoreOverflow, so ConsiderChildOverflow doesn't need to get it? The margins should be added before we transform the scrollable overflow, I think. I assume the idea here is that the margin on the scrollable overflow area will be the computed margin, not taking into account margin collapsing. I think that should be OK.

> Wouldn't we add the margin to the scrollable overflow area in > FinishAndStoreOverflow, so ConsiderChildOverflow doesn't need to get it? The > margins should be added before we transform the scrollable overflow, I think. Yes. > I assume the idea here is that the margin on the scrollable overflow area > will be the computed margin, not taking into account margin collapsing. I > think that should be OK. As we discussed, for non-negative margins this would work fine since we only union these overflows together. However, even the possibility of having negative margins means we cannot simply union scrollable overflow areas together in the parent frame. Take for instance: <div id="parent"> <div id="modelChild" style="margin-bottom: 20px; height: 10px"></div> <div id="blackSheep" style="margin-top: -20px; height: 10px"></div> </div> During reflow overflow computation for /parent/, we can't simply decide exclude the margin during /modelChild/'s FinishAndStoreOverflow because we have no idea what /blackSheep/ may contain. This is problematic. At /parent/, we basically need to calculate a new overflow rect for /modelChild/; one that either doesn't include margins (and then unioning that with the bottom edge of children like we do now) or one that now factors in /blackSheep/. I have ideas to circumvent this, but I doubt this refactoring will come out any cleaner than it is now.

I think we should just add the computed margin to the scrollable overflow area and ignore margin-collapsing, unless there's a good example of where that causes problems.

(In reply to comment #36) > I think we should just add the computed margin to the scrollable overflow > area and ignore margin-collapsing, unless there's a good example of where > that causes problems. See above. We possibly can't union child overflow rects if there was a negative margin somewhere in the descendants. Am I missing something?

We would ignore negative margins when modifying the scrollable overflow area. So in your example, modelChild would have scrollable overflow area (0,0,w,30) at position 0,0, and blackSheep would have scrollable overflow area (0,0,w,10) at position 0,10. So the computed scrollable overflow area for 'parent' would be (0,0,w,30). What's the problem here, other than we can scroll a bit further than we really need to?

I thought it would be a good idea to continue this discussion on bug 665597, as it is a rather separate issue from snappy sidebars. :) If we really think this is acceptable, great. Otherwise, one really simple solution is to just cache the bottom edge of children and re-use it during overflow calculations.

Could we move forward on this? What if HTML elements continued to reflow and XUL did this overflow update thing? That would be enough to move forward with this bug.

Run over to dbaron's desk and harass him :-)

I'm looking through the code for exceptions that we need to think about. So far I've found: * blocks need to set overflow areas on line boxes * scrollframes (of course) * visibility:collapse in tables (which is currently somewhat broken) * nsBox::IsCollapsed * things that cause clipping ('clip', nsBox::DoesClipChildren) That was from looking through callers of FinishAndStoreOverflow. Now I need to look through everything that touches nsHTMLReflowState::mOverflowAreas...

I did the remainder of the code auditing: Audit of mOverflowAreas ======================= nsBlockFrame::ComputeOverflowAreas has special code for text-overflow and for shadows nsTableCellFrame::VerticallyAlignChild considers collapsing borders as overflow (call to GetBorderOverflow()) nsTableFrame::Reflow does the same for tables (call to GetExcludedOuterBCBorder) nsMathMLContainerFrame::GatherAndStoreOverflow does some fun stuff (shadows and char stuff) Auditing callers of nsHTMLReflowMetrics::ScrollableOverflow() found nothing. Auditing callers of nsHTMLReflowMetrics::VisualOverflow() found nothing. Need to remember to apply GetWidgetOverflow to visual overflow. The other danger (that's hard to audit for) is children we intentionally do not add to the overflow areas.

With the help of this patch we can have scrollbars on fennec with close to zero repaints, see bug 657893. So I really want this :) I rebased it and fixed an apparent bug about updating the transform property.

(In reply to Florian Hänel [:heeen] from comment #44) > Created attachment 552392 [details] [diff] [review] > added transform hint, rebased > > With the help of this patch we can have scrollbars on fennec with close to > zero repaints, see bug 657893. So I really want this :) > > I rebased it and fixed an apparent bug about updating the transform property. This is going to take some work, heeen. See the comments above; there are a lot of edge cases about this, and we also need something like the patch from bug 665597. Perhaps we could convince dbaron to only allow this new overflowchanged behavior for XUL elements, and HTML elements could still trigger a reflow like the old scrollboxes? David, what do you think? Heeen, would you be interested in picking this patch up?

(In reply to Benjamin Stover (:stechz) from comment #45) > This is going to take some work, heeen. See the comments above; there are a > lot of edge cases about this, and we also need something like the patch from > bug 665597. Perhaps we could convince dbaron to only allow this new > overflowchanged behavior for XUL elements, and HTML elements could still > trigger a reflow like the old scrollboxes? > > David, what do you think? So, about a quarter of the exceptions above apply only to XUL elements, so there'd still be work to do. Secondly, I'm not sure what the "for XUL elements" test is. You'd need a test that applies to the element and all its ancestors, and probably (though I haven't done a code audit for *that*) all children (but not all descendants) of any ancestors. So I'm not at all convinced that such a restriction would reduce the amount of work left.

Comment on attachment 552392 [details] [diff] [review] added transform hint, rebased This is not ready for review yet.

Taking myself off as owner on the bug due to lots of bugs on my plate. Chris and I have some plans to work on this together after Chris is more up to speed.

Hopefully this takes most of the previous comments into account and adds code to nsBlockFrame, nsTableCellFrame and nsTableFrame to update their overflow areas correctly(?) I haven't touched nsMathMLContainerFrame yet, as it seems a bit hairy - do we want to just reflow in the case of these frames for now, or is it not as hard as it looks? Not sure if how I've gotten the nsStyleDisplay in nsBlockFrame is correct (same in nsTableFrame), or in fact if anything I've done is correct :) Feedback very much appreciated.

Comment on attachment 554908 [details] [diff] [review] Added special-case code to various frames, rebased Review of attachment 554908 [details] [diff] [review]: ----------------------------------------------------------------- Good stuff! We don't need to do anything for text-overflow, since it happens entirely at paint time. We should also have sufficient invalidation already. I think the remaining stuff from David's list that needs to be addressed is just: * nsBox::IsCollapsed * things that cause clipping ('clip', nsBox::DoesClipChildren) * For nsMathMLContainerFrame I think we should just force a reflow. Sometimes MathML alters layout based on the overflow area. We don't need to be adding complexity to optimize transforms inside MathML containers :-). ::: layout/generic/nsFrame.cpp @@ +4506,5 @@ > + overflowAreas.UnionWith(childOverflow); > + child = child->GetNextSibling(); > + } > + childList = GetAdditionalChildListName(listIndex++); > + } while (childList); Here I think you need to exclude popup-list children. ::: layout/generic/nsIFrame.h @@ +1734,5 @@ > > /** > + * Updates the overflow areas of the frame. This can be called if an > + * overflow area of the frame's children has changed without reflowing. > + * @return PR_TRUE if the overflow of this frame has changed. to be clear "return true if either of the overflow areas for this frame have changed". ::: layout/tables/nsTableFrame.cpp @@ +1893,5 @@ > + overflowAreas.UnionWith(childOverflow); > + child = child->GetNextSibling(); > + } > + childList = GetAdditionalChildListName(listIndex++); > + } while (childList); Probably should stick this code in a helper function in nsLayoutUtils. @@ +1906,5 @@ > + overflowAreas.UnionAllWith(bounds); > + > + nsOverflowAreas oldOverflow = GetOverflowAreas(); > + FinishAndStoreOverflow(overflowAreas, GetSize()); > + return GetOverflowAreas() != oldOverflow; Maybe these three lines should be shared too. Maybe FinishAndStoreOverflow could just return a boolean if it changed the areas?

Comment on attachment 554908 [details] [diff] [review] Added special-case code to various frames, rebased > // Compute our final size > ComputeFinalSize(*reflowState, state, aMetrics); >- ComputeOverflowAreas(*reflowState, aMetrics); I'm wondering where did you get these sources? according to this commit:http://hg.mozilla.org/mozilla-central/rev/abef302b61be#l1.14 it is 2008... ? >+ nsRect areaBounds = nsRect(0, 0, aMetrics.width, aMetrics.height); >+ ComputeOverflowAreas(areaBounds, aReflowState.mStyleDisplay, aMetrics.mOverflowAreas); > // Factor overflow container child bounds into the overflow area

(In reply to Oleg Romashin (:romaxa) from comment #51) > Comment on attachment 554908 [details] [diff] [review] > Added special-case code to various frames, rebased > > > > // Compute our final size > > ComputeFinalSize(*reflowState, state, aMetrics); > >- ComputeOverflowAreas(*reflowState, aMetrics); > I'm wondering where did you get these sources? according to this > commit:http://hg.mozilla.org/mozilla-central/rev/abef302b61be#l1.14 it is > 2008... ? > > >+ nsRect areaBounds = nsRect(0, 0, aMetrics.width, aMetrics.height); > >+ ComputeOverflowAreas(areaBounds, aReflowState.mStyleDisplay, aMetrics.mOverflowAreas); > > // Factor overflow container child bounds into the overflow area Sorry, I forgot to add that this depends on the patch found in bug #665597 - added the dependency now.

I think this is doing the other things that have been asked - again, if I've misinterpreted anything, please do enlighten me so I can fix it :) To consider IsCollapsed(), I've removed the nsBoxLayoutState parameter - nothing used it anyway, so I figured this was better than trying to cache it when it's available. How far are we away from getting this, and the patch in bug #665597 landed do you think?

Comment on attachment 555130 [details] [diff] [review] Reflow MathML container frames, consider IsCollapsed, etc. Review of attachment 555130 [details] [diff] [review]: ----------------------------------------------------------------- CSS "clip" is handled by FinishAndStoreOverflow already so you don't have to do anything extra there. The rest looks good. This is great work by you and Ben. I'm almost r+ here, just want to see the suggested change in nsFrame.cpp. dbaron should review this too. ::: layout/base/nsLayoutUtils.cpp @@ +166,5 @@ > + PRInt32 listIndex = 0; > + do { > + nsIFrame* child = aFrame->GetFirstChild(childList); > + while (child) { > + if (!IsPopup(child)) { More efficient to compare childList to nsGkAtoms::popupList ::: layout/generic/nsFrame.cpp @@ +6188,4 @@ > nsIFrame::FinishAndStoreOverflow(nsOverflowAreas& aOverflowAreas, > nsSize aNewSize) > { > + nsOverflowAreas oldOverflow = GetOverflowAreas(); Instead of doing this we can be slightly more efficient and have SetOverflowAreas and ClearOverflowRects return whether they changed the overflow area. It's pretty easy for the implementations of those methods to determine that. Maybe it doesn't matter, but this code is hot. ::: layout/mathml/nsMathMLContainerFrame.cpp @@ +880,5 @@ > + // Our overflow areas may have changed, reflow the frame > + PresContext()->PresShell()->FrameNeedsReflow( > + this, nsIPresShell::eResize, NS_FRAME_IS_DIRTY); > + > + return PR_TRUE; I think when we force a reflow we can return false here since we can be sure things will be updated correctly, we don't need to bother propagating the change up further. ::: layout/mathml/nsMathMLContainerFrame.h @@ +171,5 @@ > const nsRect& aDirtyRect, > const nsDisplayListSet& aLists); > > + virtual PRBool > + UpdateOverflow(); One line ::: layout/xul/base/src/nsBox.cpp @@ +638,5 @@ > + nsRect bounds(nsPoint(0,0), GetSize()); > + nsOverflowAreas overflowAreas(bounds, bounds); > + > + if (!DoesClipChildren() && !IsCollapsed()) > + UnionChildOverflow(overflowAreas); {}

(In reply to Chris Lord [:cwiiis] from comment #53) > How far are we away from getting this, and the patch in bug #665597 landed > do you think? I think we're getting pretty close!

This does most of what you said - I didn't change SetOverflowAreas/ClearOverflowRects in the end - it's easy for ClearOverflowRects to know if something has changed, but SetOverflowAreas ends up doing about as much work as just doing the comparison anyway. We could add an extra check below the visualOverflowChanged check, but I decided to keep the current check, but to short-circuit if there's no change in either area, and that ought to save us even more work anyway, I think? If this looks ok, I'll add the r? flags for you and dbaron.

Realised that you couldn't short-circuit the function like that, so just added the extra scrollableOverflowChanged check below the visualOverflowChanged check.

Comment on attachment 555360 [details] [diff] [review] Small fix-ups Review of attachment 555360 [details] [diff] [review]: ----------------------------------------------------------------- r+ from me! ::: layout/base/nsLayoutUtils.cpp @@ +174,5 @@ > + } > + child = child->GetNextSibling(); > + } > + childList = aFrame->GetAdditionalChildListName(listIndex++); > + } while (childList); Stuff has landed on mozilla-inbound that changes the way we iterate through child lists, so you'll need to pull that and rebase on top of it.

I'm re-adding the r? as I'm not sure I picked the correct list of children to iterate (all except pop-ups) I've also not finished a build (had to clobber), but if this doesn't build I'll update it as soon as my build finishes with a fixed version (wanted to get it up ASAP for review/feedback).

Comment on attachment 555691 [details] [diff] [review] Rebase with new childlist iteration Review of attachment 555691 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +182,5 @@ > + nsOverflowAreas childOverflow = > + child->GetOverflowAreas() + child->GetPosition(); > + aOverflowAreas.UnionWith(childOverflow); > + } > + } Don't do this! Use FrameChildListIterator.

Used FrameChildListIterator.

Any progress here? this is quite important for QGL rendering performance...

I have about 115 lines of review comments so far; still not through the patch yet, and I haven't started reviewing my previous comments. Hopefully I'll finish the review within a day or two.

Comment on attachment 555988 [details] [diff] [review] Use FrameChildListIterator In the future, please do changes like the removal of the parameter to IsCollapsed() as a separate patch rather than mixing it in to a substantive patch. In the IsCollapsed changes, you missed changing the implementation of IsCollapsed in layout/forms/nsTextControlFrame.{h,cpp} Please change the signature there too. So the way that nsIFrame::PreTransformScrollableOverflowProperty() is set is pretty odd -- I suggested that name because the existing PreTransformBBoxProperty is essentially the pre-transform visual overflow. However, I didn't intend that the scrollable overflow should have both overflow areas. You should either: * merge the two into a single property, containing nsOverflowAreas, or * have two separate properties, each containing nsRect rather than the current situation where we store the visual overflow twice. (It doesn't much matter where in FinishAndStoreOverflow the *scrollable* overflow gets set, since FinishAndStoreOverflow should be idempotent. But where the visual overflow is stored may well matter for the existing users of the PreTransformBBoxProperty. Renaming PreTransformBBoxProperty to PreTransformVisualOverflowProperty would be entirely reasonable.) In nsChangeHint.h, please add a comment above nsChangeHint_UpdateOverflow saying: // The frame's effect on its ancestors overflow areas has changed, // either through a change in its transform or change in its position. Given the current code in nsFrame::ConsiderChildOverflow, you're breaking the IsTableClip case by not explicitly calling ConsiderChildOverflow from nsLayoutUtils::UnionChildOverflow. However, the easiest way to fix that is to just fix that issue. In other words, remove the IsTableClip() check in ConsiderChildOverflow and add it in nsFrame::FinishAndStoreOverflow as a third reason to enter this: > if (disp->mOverflowX == NS_STYLE_OVERFLOW_CLIP || > nsFrame::ApplyPaginatedOverflowClipping(this)) { > // The contents are actually clipped to the padding area· > aOverflowAreas.SetAllTo(bounds); > } The bug in your patch that this (I think) fixes is that if you dynamically change a transform on something inside of a table cell (or other table part) that has overflow:hidden, the overflow:hidden will suddenly stop working. nsBlockFrame::UpdateOverflow is incorrect. This is because lines have cached overflow areas (block frames group their children into lines, but lines aren't strictly speaking part of the frame tree in terms of the frame tree traversal apis), and nsBlockFrame::UpdateOverflow simply digs through the existing cached overflow areas. I think the most straightforward fix here is to either add code either: * directly in nsBlockFrame::UpdateOverflow * in nsBlockFrame::ComputeOverflowAreas, triggered by a boolean passed as true from UpdateOverflow and as false from the other callers that recomputes all of the lines' overflow areas. (Probably a little better to put it in UpdateOverflow since then you can only walk the lines once, and have better memory locality. Er, actually, scratch that... see my next review comment.) This is probably easiest to do using nsFrame::ConsiderChildOverflow. Beware that iterating the children of a line requires using line->GetChildCount(); if you don't use that you'll just keep walking into all later lines and end up with an O(N^2) algorithm. The bug this causes is that any overflow changes from transform changes simply don't propagate through blocks. It would be nice to add a reftest for this, and make sure it fails with your current patch (probably easiest to do with scrollbars rather than trying to do a repainting-based test). Additionally, nsBlockFrame::UpdateOverflow needs to consider the other child lists that are added to overflow areas after the call to ComputeOverflowAreas in Reflow. These are: * overflow containers (these occur only when paginating) * pushed floats (these occur only when paginating) * absolutely positioned elements I'm inclined to suggest that you give up on the attempts to adapt nsBlockFrame::ComputeOverflowAreas to be usable from nsBlockFrame::UpdateOverflow (i.e., undo the changes to ComputeOverflowAreas and IsClippingChildren), and then change nsBlockFrame::UpdateOverflow to: * do the loop over the lines I described in my previous comment to update (since they'll need to be updated for the next time we Reflow) * then call nsLayoutUtils::UnionChildOverflow to rebuild the overflow areas, which will go through all child lists correctly In nsIFrame::UpdateOverflow, I think the IsCollapsed() check is incorrect. IsCollapsed is a XUL layout method; though the XUL layout methods have been folded into nsIFrame, its result is still only meaningful for XUL boxes, and your check makes UpdateOverflow inconsistent with the original (non-update) codepath. The code that checks IsCollapsed in the normal codepath is in nsBox::SyncLayout, called from nsBox::Layout, called from nsIFrame::Layout, which is called only when we're using the box layout protocol. I believe the easiest fix is to change: >+ if (!IsCollapsed()) { to: + bool collapsed = (IsBoxFrame() || IsBoxWrapped() && IsCollapsed(); + if (!collapsed) { nsGfxScrollFrameInner::UpdateOverflow should not be virtual; this adds a vtable pointer to a class that does not currently have one and should never need one. (nsHTMLScrollFrame:: and nsXULScrollFrame::UpdateOverflow should still be virtual.) In nsIFrame.h, |friend class nsCSSFrameConstructor| shouldn't be needed. Why is it needed? Please get rid of it, and make what you need public instead or use appropriate existing public methods. nsGfxScrollFrameInner::UpdateOverflow has two serious problems. Scroll frames never have overflow area, since they always clip their children. This means that calling the base class's UpdateOverflow method is incorrect, since it means that it will set an inappropriate overflow area. Likewise, returning true is always incorrect. You should revise it to: * remove the chunk at the beginning that calls the base class's UpdateOverflow method (whose return value is pretty meaningless here) * always return false It's probably best to invert the condition and move the FrameNeedsReflow call inside it, and thus just have one return statement at the end of the function. In nsMathMLContainerFrame::UpdateOverflow, please put a "so" after the first comma and a period at the end of both sentences. In nsStyleDisplay::MaxDifference, could you at least wrap your revision at less that 80 characters (and maybe the existing overlong line too)? nsTableCellFrame::UpdateOverflow should use nsLayoutUtils::UnionChildOverflow rather than calling ConsiderChildOverflow directly. There's other work in progress (for absolute positioning inside any relatively positioned element) that's likely to lead to table cells having other child lists, I think. nsTableFrame::UpdateOverflow would be cleaner if you inflate the bounds up front, before constructing overflowAreas. In nsTableFrame::UpdateOverflow: >+ const nsStyleDisplay* display = mContent->GetPrimaryFrame()->GetStyleDisplay(); mContent->GetPrimaryFrame() returns |this|, but very slowly. >+ if (display->IsTableClip()) { You're missing a ! here. Combine the two, and make it just: >if (!GetStyleDisplay()->IsTableClip()) { (and, as I said above, earlier in the function). nsBox::UpdateOverflow is wrong for a bunch of reasons. It needs a little background to explain, though. XUL display types have a rather distinct layout model: most of the methods they use for position calculation are different. The class inheritance hierarchy used to involve those XUL objects inheriting from both nsIFrame and nsBox. However, to (a) simplify the interaction between XUL and non-XUL layout and (b) reduce the memory consumption of XUL layout objects (reduce the number of vtable pointers), nsBox was stuck into the inheritance hierarchy for all frames; thus nsFrame now inherits from nsBox. However, only frame classes that inherit from nsBoxFrame and nsLeafBoxFrame (which are much further down in the hierarchy) actually *use* the layout logic from nsBox. What you've done in nsBox::UpdateOverflow is implemented an UpdateOverflow method that actually applies to *all* frame classes. Essentially, nsIFrame::UpdateOverflow is never used because nsBox::UpdateOverflow is always called instead, for all concrete frame types. This is a problem in three ways: * there's code in nsBox::UpdateOverflow that should only apply to real XUL boxes and not to other frame types. What you've done makes certain XUL concepts apply to other frame types in an inconsistent way (i.e., only after certain dynamic changes have taken place) * it's confusing -- you have an nsIFrame::UpdateOverflow implementation that's never used * what you've done actually *doesn't work* on non-XUL-box frames, because it uses GetChildBox() for frame tree iteration (which there's actually no need to do). So as far as I can tell this makes the patch simply not work for most cases. There's also code in nsBox::UpdateOverflow -- in particular, the view update code, that really does need to apply to all frame classes. There shouldn't be an nsBox::UpdateOverflow. Everything from it that's needed should be merged into nsIFrame::UpdateOverflow -- this includes the DoesClipChildren() check (which, like the IsCollapsed check, should only be tested on box frames) and the view updating code. Now, to look through the parts of comment 42 and comment 43 to make sure they're all addressed (either in the patch or in comments above made while reviewing it): the following points seem not to be addressed: from comment 42: >* visibility:collapse in tables (which is currently somewhat broken) from comment 43: >nsBlockFrame::ComputeOverflowAreas has special code for text-overflow >and for shadows (addressed in the current version, but needs to be readdressed given my nsBlockFrame comments above) Given the current broken state of visibility:collapse in tables, I supposed I'd be ok postponing dealing with that to a followup bug. Given the errors I *found*, I suspect there are some that I didn't find and thus I suspect this patch is likely to break Web content due to errors in the patch. I think it would probably be a good idea to try writing some automated tests that test this patch's effect on Web content before landing it, given that most of the code in the patch didn't actually achieve what it was intending to do. Testing that scrollable overflow areas have been dynamically updated correctly is relatively straightforward: you can examine the extents of scrollbars. Testing that visual overflow areas are updated correctly is a little harder, since it involves tests involving repainting -- and you need to hit a relevant repainting codepath without doing something that triggers reflow of the subtree. Testing that you've found such a codepath probably may require poking at things in the debugger or with printfs. Marking review- because I think I do need to look at another revision of this one.

(In reply to David Baron [:dbaron] from comment #64) > Testing that scrollable overflow areas have been dynamically updated > correctly is relatively straightforward: you can examine the extents of > scrollbars. Testing that visual overflow areas are updated correctly is > a little harder, since it involves tests involving repainting -- and you > need to hit a relevant repainting codepath without doing something that > triggers reflow of the subtree. Testing that you've found such a > codepath probably may require poking at things in the debugger or with > printfs. However, I should also note: given the similarity of the two codepaths, it may well be sufficient to test only scrollable overflow for now.

(In reply to David Baron [:dbaron] from comment #64) > So the way that nsIFrame::PreTransformScrollableOverflowProperty() is > set is pretty odd -- I suggested that name because the existing > PreTransformBBoxProperty is essentially the pre-transform visual > overflow. However, I didn't intend that the scrollable overflow should > have both overflow areas. You should either: > * merge the two into a single property, containing nsOverflowAreas, or > * have two separate properties, each containing nsRect And, on second thought, I think a single property (say, PreTransformOverflowProperty) is clearly better. It would need to be set where the PreTransformBBoxProperty is currently set.

Wow, my review was very feeble. Thanks David!

(In reply to David Baron [:dbaron] from comment #43) > I did the remainder of the code auditing: > > Audit of mOverflowAreas > ======================= > > nsBlockFrame::ComputeOverflowAreas has special code for text-overflow > and for shadows The shadows bit was removed in https://hg.mozilla.org/mozilla-central/rev/1d2b61ed8e44 ; I'm not sure where I got the text-overflow bit from, since I can't find anything now.

ok, here's a patch with most (but not all) of the comments addressed, hopefully. Things I've not done: * Combine PreTransformBBoxProperty / PreTransformScrollableOverflowProperty properties * Write tests Any suggestions for tests would be good - would animating a CSS translate transform and checking the scrollHeight DOM property of a containing element with overflow: auto/scroll be enough? Asking for feedback to make sure I've interpreted the comments correctly, but not review yet as I realise even if I have, it isn't quite ready yet.

(In reply to Chris Lord [:cwiiis] from comment #69) > Any suggestions for tests would be good - would animating a CSS translate > transform and checking the scrollHeight DOM property of a containing element > with overflow: auto/scroll be enough? If by animating, you mean dynamically changing (just need to change it once), yes. Note that you want to change from translate(0) to translate(something else) (rather than changing from 'none'), that the thing you check scrollHeight on has to be scrollable (overflow:hidden may be easiest), and that the interesting thing about the tests is what is an ancestor of the element with the transform but still a descendant of the scrollable element -- since what you want to test is how overflow propagates through different things.

I've now combined the PreTransformBBoxProperty and PreTransformScrollableOverflowProperty into a single PreTransformOverflowAreasProperty. I've also written a small test (which I'll attach afterwards) which shows that this isn't working correctly... I'll go through debugging it, but any feedback would be greatly appreciated.

The attached test works in current Firefox/fennec but breaks when this patch and the patch from bug #665597 are applied. I'm in the process of debugging this.

ok, after looking over this a bit, I think the logic might be wrong... This test fails because the overflow of container2 doesn't change (because it's overflow: visible). So, either this is a bug that the outer container becomes scrollable (given the intermediate overflow:visible, should it?), or the logic of breaking when overflow areas don't change isn't quite right. If it's a bug, that's interesting, if not, should the logic be changed so that if our overflow hasn't changed because we're clipping (or non-scrollable), we should return true from UpdateOverflow?

(In reply to Chris Lord [:cwiiis] from comment #73) > ok, after looking over this a bit, I think the logic might be wrong... This > test fails because the overflow of container2 doesn't change (because it's > overflow: visible). So, either this is a bug that the outer container > becomes scrollable (given the intermediate overflow:visible, should it?), or > the logic of breaking when overflow areas don't change isn't quite right. The overflow of container2 should change.

Either I misunderstood something dbaron said on IRC, or I made a typo interpreting it, but that's what caused the test to fail. Here's the revised patch for review.

This patch adds a test to the layout tests that checks whether dynamic transform changes result in the correct scrollable overflow being propagated through parent frames.

Comment on attachment 564950 [details] [diff] [review] part 6, add overflow handling test This is good, though it would probably also be a good idea to test overflow propagation through a few other types of things (e.g., some things that should clip the overflow like scroll frames ('overflow'), 'clip', collapsed XUL boxes, and perhaps also some things that should propagate the overflow). r=dbaron on this

Comment on attachment 564948 [details] [diff] [review] Address review, fix bad misunderstanding/typo Comments based on reviewing the diff between the previous patch I reviewed and this one: >+ // The frame's effect on its ancestors overflow areas has changed, >+ // either through a change in its transform or a change in its position. s/ancestors/ancestors'/ (my fault!) nsBlockFrame::UpdateOverflow should, at the end, call through to its immediate base class, i.e.: return nsBlockFrameSuper::UpdateOverflow(); instead of: return nsFrame::UpdateOverflow(); so that if a new override is added in between, nsBlockFrame will pick it up. nsBlockFrame::UpdateOverflow should not have this bit: >+ const nsStyleDisplay* display = mContent->GetPrimaryFrame()->GetStyleDisplay(); >+ if (IsClippingChildren(this, display)) >+ return PR_FALSE; It still needs to update the overflow in its line boxes, and nsFrame's UpdateOverflow has the appropriate checks. Just remove these lines. Still in nsBlockFrame::UpdateOverflow: >+ ConsiderChildOverflow(lineAreas, lineFrame); >+ lineAreas.UnionWith(lineFrame->GetOverflowAreas()); The first line should be sufficient. The second line is incorrect, since it unions in a rectangle that's in a different coordinate system (compare it to what nsFrame::ConsiderChildOverflow does). Remove the second of these lines. You shouldn't need to move GetLayoutFlags and DoesClipChildren from nsBox to nsIFrame now that the base UpdateOverflow is on nsFrame rather than nsIFrame. Put them back where they were. In nsFrame::UpdateOverflow: >+ PRUint32 flags = 0; >+ GetLayoutFlags(flags); >+ >+ nsIView* view = GetView(); >+ if (view && (flags & NS_FRAME_NO_SIZE_VIEW) == 0) { Could you null-check |view| before calling the virtual and rarely-needed |GetLayoutFlags| function? Also, please run the boolean updating scripts from mwu's blog to convert PRBool->bool, PR_FALSE->false, and PR_TRUE->true. >+ bool hasTransform = IsTransformed(); >+ if (hasTransform) { >+ Properties().Set(nsIFrame::PreTransformOverflowAreasProperty(), >+ new nsOverflowAreas(aOverflowAreas)); >+ } else { >+ Properties().Delete(nsIFrame::PreTransformOverflowAreasProperty()); >+ } You can remove this; setting it below is sufficient, except that you'll need to add the |Delete| bit to the part below. In nsFrame::ConsiderChildOverflow, also remove |disp|. In nsGfxScrollFrameInner::UpdateOverflow, the brace-indent should be 2 spaces, not 4 (so it shouldn't line up with the multiline if condition). Also, I don't think you really need the |presContext| temporary; may as well just use one line. Comments based on going through my previous review comments (comment 64) in relation to this patch: Still need to file a followup bug on 'visibility: collapse' in tables. r=dbaron with those things fixed, though I think some additional tests would also be a good idea

Tested latest reviewed patch with CSS3D transforms and still see the problem with rendering some Text objects... http://romaxa.bolshe.net/css3d/morphing/morphing-cubes.html http://romaxa.bolshe.net/css3d/poster/poster-circle.html with these patches "Text frames" are disappearing, with previous patches I've seen also whole morphing planes disappeared too, now is only text

This is a rebased patch with all issues from comment #78 addressed. I also tested the two links in comment #80 and both appear fine for me, but I'd appreciate feedback to see if they work ok for you. The last time I pushed this to try, some tests failed, so I won't check this in until I have positive feedback and all tests pass. This patch should not change any rendering behaviour.

Comment on attachment 569978 [details] [diff] [review] Rebase and address comment #78 Checked this patch and it seems to work fine for me

Should bug 157681 depend on this one?

Yes.

Minor edits to make it compile.

The failed test was layout/reftests/bugs/609272-1.html The bug was that IsTableClip() was used on all types of frames, it's only valid for table frames.

BTW, do we still need to reflow for a transform-origin change? or could we use nsChangeHint_UpdateOverflow for the other transform properties as well? http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2220

Comment on attachment 581270 [details] [diff] [review] Bug fix and style nits + (GetType() == nsGkAtoms::tableFrame && disp->IsTableClip()) || Reverse the order of the && parts. disp->IsTableClip will be fast, but GetType() is a new virtual call. Seems to me, looking at ApplyOverflowHiddenClipping, we should be checking or tableCellFrame/bcTableCellframe too? Maybe we should have a new IsFrameOfType bit instead of hardcoding frame types? Seems to me your assertion will fail in calls to IsTableClip from nsFrame::ConsiderChildOverflow. How about we pass in the frame and have IsTableClip check the frame type and return false if the frame is not a suitable table part? Then ApplyOverflowHiddenClipping would call IsTableClip instead of checking the frame type itself.

(In reply to Mats Palmgren [:mats] from comment #87) > BTW, do we still need to reflow for a transform-origin change? or could we > use > nsChangeHint_UpdateOverflow for the other transform properties as well? > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct. > cpp#2220 I think we could avoid reflowing for all those property changes, yes.

We could also do this for 'outline' changes, but that should probably be a separate patch or bug.

(In reply to Mats Palmgren [:mats] from comment #85) > Created attachment 581267 [details] [diff] [review] > Updated to trunk > > Minor edits to make it compile. It looks like you also updated it to no longer be on top of bug 665597 -- which means that I think it will now cause incremental reflow bugs related to bottom margins. The reason for bug 665597 was to avoid that -- though we might end up having to take the alternative approach of never including margins in overflow rather than (as bug 665597 does) always including them -- to replace the current state of sometimes including them that's hard to maintain with this new code.

This is on top of the patches in bug 665597.

> Seems to me, looking at ApplyOverflowHiddenClipping, we should be checking > or tableCellFrame/bcTableCellframe too? Yes. We have quite a few of these test functions so I replaced them with just one. While we cannot possibly match ::blockFrame for the calls inside nsTableFrame and vice-versa for the calls in nsBlockFrame, I think it's worth it to consolidate these tests into one function. > Maybe we should have a new > IsFrameOfType bit instead of hardcoding frame types? It's now only in one place again, so it didn't seem worth abstracting for just one call site. So the changes are: 1. consolidate ApplyOverflowHiddenClipping/ApplyOverflowClipping/ ApplyPaginatedOverflowClipping/IsTableClip into one function - static bool nsFrame::ApplyOverflowClipping 2. use nsChangeHint_UpdateOverflow for all transform style changes (except for BackfaceVisibility as before) 3. fix a bug in nsBlockFrame::ComputeOverflowAreas() that the original patch introduced when changing 'aReflowState.mComputedBorderPadding' to 'GetUsedPadding()'. (Bug 665597 causes plenty of reftest failures though, with or without these patches.)

Updated to trunk, on top of new set of patches in bug 665597. No changes from last version.

See comment 93 for a description of the changes. This new version is just an update over the new patches in bug 665597, no new changes.

Comment on attachment 585535 [details] [diff] [review] part 2, additional fixes Review of attachment 585535 [details] [diff] [review]: ----------------------------------------------------------------- Let's land the nsStyleStruct.cpp changes as a separate patch for better bisection. ::: layout/generic/nsBlockFrame.cpp @@ +950,5 @@ > // If we have non-auto height, we're clipping our kids and we fit, > // make sure our kids fit too. > if (aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE && > aReflowState.ComputedHeight() != NS_AUTOHEIGHT && > + nsFrame::ApplyOverflowClipping(this, aReflowState.mStyleDisplay)) { why is the nsFrame:: qualifier needed, here and a lot of other places below in nsFrame subclasses? Get rid of it :-) ::: layout/generic/nsFrame.cpp @@ +1395,5 @@ > // Other overflow clipping is applied by nsHTML/XULScrollFrame. > + // We allow -moz-hidden-unscrollable to apply to any kind of frame. This > + // is required by comboboxes which make their display text (an inline frame) > + // have clipping. > + if (nsFrame::ApplyOverflowClipping(aFrame, aDisp)) { Write it as if (!...) return false; ::: layout/generic/nsFrame.h @@ +600,5 @@ > + // Actually there were also table rows and the inner table frame > + // doing this, but 'overflow' isn't applicable to them according to CSS > + // 2.1 so I (roc) removed them. Also, we used to clip at tableOuterFrame > + // but we should actually clip at tableFrame (as per discussion with Hixie > + // and bz). Just remove this comment.

Also, I'd like to have a patch in this bug that makes 'outline' changes use the UpdateOverflow hint. The reason I'd like that in this bug is that should produce a good performance improvement in a lot of situations (changing focus rings should no longer require a reflow). It may also expose bugs in the infrastructure here.

For the record, the latest patch set does fail a reftest on Android: https://tbpl.mozilla.org/?tree=Try&rev=f3b461d774ed The test is layout/reftests/transform/dynamic-inherit-1.html It's a dynamic -moz-transform-origin change so I suspect the patches in this bug rather than bug 665597.

> Also, I'd like to have a patch in this bug that makes 'outline' changes use > the UpdateOverflow hint. > It may also expose bugs in the infrastructure here. Yes, it did - we need to update the overflow for all continuations and their ancestors (but not the ancestors' continuations, right?).

> Let's land the nsStyleStruct.cpp changes as a separate patch > for better bisection. Ok, this is that patch (with a fix for the Android failure - I mistakenly changed Repaint to UpdateTransformLayer hint in the last version)

With nits in comment 96 addressed as suggested.

Looks green so far: https://tbpl.mozilla.org/?tree=Try&rev=9138ecb6f3dc https://tbpl.mozilla.org/?tree=Try&rev=2a757d5d81cc

Comment on attachment 586049 [details] [diff] [review] part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those. Review of attachment 586049 [details] [diff] [review]: ----------------------------------------------------------------- The code to only update the parent when we hit the last continuation with that parent is great, but the hashtable optimization doesn't seem right. If you have two continuations, A and B, whose parents AP and BP are different, but their parent C is the same, then updating overflow on A, then AP, then C, then B and BP but not C again could be incorrect. The contribution of BP's updated overflow area to C would not be recorded. I'd just rip out the hashtable optimization. I doubt it will help very much in practice, since at some point up the ancestor chain, probably not very far, the overflow area will stop changing as we process each continuation, at which point we stop walking the chain, so this should not be expensive even though we have to visit some frames multiple times. And updating overflow only on frames with many continuations is likely to be very rare. And fixing the optimization to be correct would add a lot of complexity, it would have to become some kind of bottom-first traversal of the frame tree.

Comment on attachment 586049 [details] [diff] [review] part 2, Update the overflow for all the affected frames, not just the primary frame. And the ancestors of those. Review of attachment 586049 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you remove the hashtable optimization.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #104) Yes, good points, thanks. I removed the hashtable optimization. I'll push this bug + bug 665597 when the tree reopens.

I suggest letting bug 665597 go into a nightly before landing this bug, to make regression hunting easier. I also suggest filing a followup bug to have a good look at CalcDifference implementations and see which reflow hints can be converted to UpdateOverflow. I think there's quite a few that can be converted...

Filed bug 719177 to investigate using the UpdateOverflow hint for more properties. https://hg.mozilla.org/integration/mozilla-inbound/rev/8651133cd196 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0e181c6aa4 https://hg.mozilla.org/integration/mozilla-inbound/rev/4352ce320584 https://hg.mozilla.org/integration/mozilla-inbound/rev/73eaf1199ff0 https://hg.mozilla.org/integration/mozilla-inbound/rev/a915d5820eb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/016472ad4a74

https://hg.mozilla.org/mozilla-central/rev/8651133cd196 https://hg.mozilla.org/mozilla-central/rev/9d0e181c6aa4 https://hg.mozilla.org/mozilla-central/rev/4352ce320584 https://hg.mozilla.org/mozilla-central/rev/73eaf1199ff0 https://hg.mozilla.org/mozilla-central/rev/a915d5820eb8 https://hg.mozilla.org/mozilla-central/rev/016472ad4a74

Part 4 was backed out in bug 724432.