results on win-xp pro,1.1ghz,512ram. trunk 2002071508: 851ms msie6: 411ms

To clarify what the hell this bug report is talking about.... setting .style.left currently reflows the div. It _should_ just reposition its view and frame and then invalidate. This has been talked about a bit, but there seems to be no bug on it. Things that would need to get done: 1) A way to detect that a move is enough and that reflow is not needed. 2) A way to actually do the moving.

Athlon 750 Moz trunk 2002070304 - 2340 IE6 - 428

PII 366MHz 160MB win98 Mozilla 2002071508 (1.1+) : 3.680s IE6 : 1.040s

Nomianting for nsbeta1 as this appears to improve performance.

> dbaron: nsAbsoluteContainingBlock reflows the abs pos elements, right/ > dbaron: ? <dbaron> yes > dbaron: could it keep a cache of computed position values for each element? <dbaron> why? <dbaron> (see also bug 75121) > dbaron: and optimize away reflows to repositions if the computed props did not change sufficiently <dbaron> bz: Doing it that would would require having StyleChangedReflow tell us what changed. I think it's easier to make a different type of style hint. > dbaron: true, since it could involve a font change or something in addition to the reposition > dbaron: so we would need a position-prop hint or something <dbaron> yeah <dbaron> And it would go to a function that decided whether to reflow or reposition.

nsbeta1+

Athlon 1000, 512MB RAM, Debian+KDE3 1.1: 2676 konqueror 3.0.2: 1373

oh.. Opera 6.03/linux: 556 (!)

This would be such a huge win, and perhaps not difficult now. The biggest problem I see is that repositioning a frame might require view bounds to grow. We'd have to recalculate view bounds up the view tree.

This would be a major performance boost on overal DHTML and highly desired for 1.2 - let's get things rockin'

Some further thoughts on implementing this (just off the top of my head, so I'm not sure they're right): 1. The "additional style hint" (a "move" hint) idea is a good bit easier now thanks to roc's work for bug 160936. 2. In order to process that style hint: + for non-positioned elements, do nothing + for relatively positioned elements, just move. We might need to cache the un-offset position as a frame property or something like that, since we don't know the old value. I'm not quite sure where we could do that since just about anything (with just about any parent) can be relatively positioned. + for absolutely positioned elements, we need to decide whether to move or reflow. We currently cache a bit of information as a frame property (I think an nsRect representing the overflow area) in nsAbsoluteContainingBlock.cpp. We should probably turn this rect into a struct containing the rect and put more information in it -- in particular, whether the positioned element wrapped when it was last reflowed, and the values of the four offset properties at the last reflow. It might even be useful to cache the preferred width and preferred minimum width. This information is really only needed to handle a 'width' of 'auto' (see section 10.3.7 of CSS 2.1 -- *not* CSS 2). We would then need to reflow rather than reposition in the following cases for auto-width positioned elements: + something wrapped at the previous reflow (i.e., we used the available width or the minimum width rather than the preferred width -- although it's theoretically not that hard to optimize some cases where minimum width is used as well, those cases are extremely rare on the web), and we now have *more* available space + we now have *less* available space, and the old width doesn't fit in the container anymore If we don't need to reflow, then we can just reposition. Handling switches of the offset properties to and from 'auto' might also be a little tricky, although I think not. It might be nice to have bug 75121 in before this, although hopefully they won't trip over each other too much. However, bug 75121 is about exposing a bunch of that nsAbsoluteContainingBlock code that's been unused for years and has bugs in it, so it's not clear how much will need to be changed (I haven't even had a chance to even reduce the testcases for the things the patch breaks). 3. Then, given that something has moved, we need to handle view sizes and repainting. I'll let roc talk about this bit. :-)

(Note that it might be worth checking to see if the rules in CSS 2.1 are really compatible with what MSIE implements for absolute positioning, or whether MSIE does something that would be easier to implement this optimization on. If that's the case, it might be worth trying to change the rules further in CSS 2.1.)

Also note that in the case of relatively positioned elements, we can't forget continuation frames, since inline elements can be relatively positioned. Or we could skip the optimization for relatively positioned elements entirely in the first pass, and just make them reflow. Or is animation of relative positioning also a big perf issue in real-world "DHTML"?

We have a number of serious relative positioning bugs that we should tackle before we worry about optimizing relative positioning :-). (The main issue being that lines and the reflow state for inline frames need to keep TWO "overflow areas" --- one which pretends everything is in-flow, and one which accounts for relative positioning.)

After moving a frame, repainting is easy; we just repaint the old and new frame overflow areas. Resizing the views and cached overflow areas is harder. Then we run up the view stack making views bigger if they need to grow to contain new overflowing children. If children move so that the view can be smaller, then things will mostly work fine even if we don't shrink the view (except for scrolling views, see below). That's good because otherwise we would need to know the overflow areas of a lot of frames. A tough problem is fixing up scrollbars if a scrolled area grows or shrinks. This requires computing the exact size of the scroll view which could require overflow areas we don't really have. We might want to be able to detect this case and just reflow. I think we should focus on bug 75121 before we tackle this. I would be very interested to see what the performance of an incremental reflow targeted at an absolutely positioned frame is like after that fix, especially when the positioned frame is something simple like an image. BTW the thought just occurred to me that we don't actually need to keep the overflow area of an absolutely positioned frame in the frame property list; it should always be the same as the bounds for that frame's view.

Nominating for topembed as DHTML perf is something we should address in the next milestone for embedding customers.

Actually, having worked on 75121 and with some further reflection, I think we should concentrate on improving the incremental reflow of absolute frames rather than adding an entirely new code path.

Why do you think that?

I can't think of a reason why repositioning of absolute frames using incremental reflow has to do any more work than we'd have to do with a separate non-reflow frame repositioning code path. With 75121 we don't do much work as we descend through a stack of absolutely positioned frames to get to the frame that's moving. On the way back out of the stack we do some work, but as far as I can tell, we do exactly what we'd need to do in a frame repositioning code path. With 75121 the big cost will probably be the cost of processing a stylechange reflow for the moved frame, and that could be expensive, and that's something we should fix (perhaps by adding a "PositionFrame" incremental reflow reason?). If I'm wrong about that and there are some big costs on the incremental reflow path, then maybe we should see if we can fix those. If we can get comparable performance, then doing this in the context of reflow has architectural advantages: we'll add a lot less new code --- and especially we'll reduce code duplication, which is really evil. We won't have to worry so much about special cases that reflow already takes care of. The optimization might also be applicable to more situations. Is there a convincing case that this can't be done in the context of reflow?

The only time there's a strong case for repositioning instead of full reflow is when we know the size did not change. The idea is to optimize away the reflow of the children of the positioned frame. If we can usefully optimize this in the general case for reflow, we should by all means do so. The problem right now is that to decide what our size is after style.left is set we have to do a full reflow of all the descendants. We need to detect the following constraints during reflow: 1) Our size is independent of whatever happened to trigger the reflow or [ 2a) Nothing changed about our descendants (this is captured by the "resize" reflow type, which we would need to use here, somehow) and 2b) Our size depends only on our descendants (this typically only applies to things that are shrink-wrapping, like floats, tables, positioned elements). ] If we can usefully detect these situations in normal reflow, I would be all in favor of doing so and putting some shortcut code in ::Reflow() to bail out early without reflowing kids when those conditions are met.

One thing that would make this a lot easier would be if .style.left actually went through the nsStyleStruct CalcDifference path instead of the nsCSSParser hint computation path. In the former path you have a lot more information to work with to generate a good hint.

But yes, we definitely need a way to detect that style changes don't require reflowing of children, and we need it *everywhere*.

Yeah, dbaron was thinking of not using the parser hint there in any case... We should just do it.

Do you feel like doing it, Mr Style System Peer? :-)

Once I have a tree again (eta: 2.5 weeks) I'll give it a shot, sure. See bug 158713 for that, probably.

What other style changes require a reflow that doesn't require reflowing of children? (OK, vertical margins / border / padding sometimes don't (when floats aren't present), but they require a much larger reflow of everything afterwards, and I don't think they're common enough to merit optimization.)

Hmm. Maybe no important properties, once clip and z-index are fixed to not require reflow. Maybe we could have CalcDifference produce two extra hints, nsChangeHint_ReflowRepositionFrame and nsChangeHint_ReflowResizeFrame (subhints of nsChangeHint_ReflowFrame). Store the hint in the StyleChange reflow command, and whack nsBlockFrame::Reflow to understand them --- i.e., basically do nothing if it's just a RepositionFrame, and do a resize reflow if it's just a ResizeFrame.

Note that there are cases in which CalcDifference can't tell whether a reflow is needed. In particular, consider changing .left on something with auto width and right... Whether a reflow is needed will depend on whether the preferred width is smaller than the available width both before and after the .left change.

Always treating that as a size change probably isn't too bad.

Are there still such cases in the spec? I hope the revised absolute positioning rules got rid of them. (I described this in more details in comment 11, but I don't remember the summary.)

My previous comment was a response to comment 28. Also, I'd rather see separate handling of this separate case than trying to make nsBlockFrame::Reflow do yet more stuff (see comment 11). It's complicated enough as-is.

Marking as topembed+, as it is desired by major embedding partners.

Moz 20021210: 3405 IE6 : 350 Opera7 : 10205

This depends on bz's inline style resolution fix. Marking an explicit dependency. However, I think once that's fixed, this will not be too hard: I suggets dividing nsChangeHint_Reflow into two sub-hints nsChangeHint_ReflowPosition and nsChangeHint_ReflowGeneral. ReflowPosition would generated when a style change affects 'left', 'top', 'bottom' or 'right' and appropriate dimension ('width' or 'height') is not 'auto'. When we get a ReflowPosition hint, we set something in the incremental reflow command to say that the ResizeReflow for the frame itself can be suppressed.

This is 1.4 material?

could be, although I think for many DHTML benchmarks painting is our bottleneck now

Just checked the testcase again: results on win-xp pro,1.1ghz,512ram. trunk 2003022709: 1242ms msie6 sp1: 281ms Any idea why we are doing even worse now [compared to when filing the bug]?

I wonder why in this testcase we need to reflow at all between assignments to style.left.

Do we? I've been thinking about it, and with the way that testcase is written I would expect we just post a ton of reflow commands at the div that all get merged into a single reflow, executed after the loop finishes. So I don't think this testcase is in fact a good testcase for the problem this bug is meant to address. This is a profile (gzipped HTML) of that testcase (with the loop number bumped up an order of magnitude). Nothing really major going on here other than the usual -- style resolution, parsing of the CSS, change notification processing.

Right. Not a good testcase for this bug. This testcase is only testing style/DOM. A better testcase for this bug would be one which animates something simple.

Some of the work needed for bug 175364 is rather similar to what needs to be done for this bug. (Really, it's a small subset of what needs to be done, except that, as for this bug, we need to double-check that we're following the absolute positioning rules correctly and avoiding any dependencies on the containing block for anything other than percentages. Of course, if we're not, we'll just end up with incremental reflow bugs.)

is this still on track for 1.5a?

This isn't a priority for me. nsIFrame deCOMtamination and bug fixing is all I have on my radar.

Moving to 1.6a. 1.5a is not realistic.

Maybe 1.5b ?! We all do agree that this is really important - but unfortunately postponed from milestone to milestone (originally 1.2alpha, as indicated by bug history).

roc/dbaron - anyone of you willing to take this ... kin seems to be able to get to this in the near future.

The following testcase shows that the size of absolutely positioned elements does depend on the remaining space within the containing block, as CSS2.1 says: http://dbaron.org/css/test/2003/abs-pos-intrinsic3 This means we can't just move in the general case, which makes this a good bit harder to solve, particularly with our current architecture. (If min-width / preferred with computation were separate from reflow, it might be easier to cache the results and optimize based on those results.)

I guess this again will have to be retargeted ... 1.7a ?

taking bug

Updating testcase http://dbaron.org/css/test/2003/abs-pos-intrinsic3 instead of http://www.world-direct.com/mozilla/styleleft.htm

The new testcase performs much better (28+-3% CPU) than the old one (100% CPU peak). Intel Celeron 2.8 GHz on 2.6.17-11-generic K/Ubuntu 6.10 (KDE 3.5.5) with display: :0.0 screen: 0 OpenGL vendor string: ATI Technologies Inc. OpenGL renderer string: RADEON XPRESS Series Generic OpenGL version string: 2.0.6011 (8.28.8) Opera 9.20 took 260 for the old testcase and Firefox 2.0.0.3 took 1316.

Just to update on the state of things here, bug 300030 fixed the issue David talks about in comment 47.

Is it still worth keeping this bug open?

Yes, I think so.

I talked to dbaron about this, and here is what we plan to do here. According to http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width, the only case where the width can change as a result of changing the position of an absolutely positioned element is rule number 5. This is the only case where we would need to overflow, and we can skip that for all other cases. The implementation would add two new hints for recomputing the horizontal and vertical position. These two hints will be returned from nsStylePosition::CalcDifference, and in the processing code: a) If the element is relatively positioned, we would recompute the position, and we would never need to reflow in this case. b) If the element is absolutely positioned (including the case for position: fixed), we fall back to reflow in case width is auto and left and right are not auto. For all other cases, we recompute the position, and avoid reflowing. Note that in the cases where the reflow is skipped, we also need to use the UpdateOverflow hint in order to update the overflow information of the ancestors (which might have changed because of the position change.)

(In reply to Ehsan Akhgari [:ehsan] from comment #57) > I talked to dbaron about this, and here is what we plan to do here. > > According to http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width, > the only case where the width can change as a result of changing the > position of an absolutely positioned element is rule number 5. This is the > only case where we would need to overflow, and we can skip that for all > other cases. s/need to overflow/need to reflow/ (I think that's what you meant) > The implementation would add two new hints for recomputing the horizontal > and vertical position. These two hints will be returned from > nsStylePosition::CalcDifference, and in the processing code: > > a) If the element is relatively positioned, we would recompute the position, > and we would never need to reflow in this case. > b) If the element is absolutely positioned (including the case for position: > fixed), we fall back to reflow in case width is auto and left and right are > not auto. For all other cases, we recompute the position, and avoid > reflowing. We also might need to fall back to reflow for the cases where the new position is the "static position". (Though it might be easy enough to refactor that code so it's usable.) If we do, that's the main reason for wanting separate horizontal and vertical hints, since that would allow us to use this "move and update overflow" code when we have movement in one dimension but the opposite dimension uses a static position. > Note that in the cases where the reflow is skipped, we also need to use the > UpdateOverflow hint in order to update the overflow information of the > ancestors (which might have changed because of the position change.) We can just return the UpdateOverflow hint unconditionally. nsCSSFrameConstructor::ProcessRestyledFrames *already* has code to ignore the UpdateOverflow hint when we also did a reflow. (We should probably add the move-position hint *before* reflow in that function (so that it's easier for it to fall back to doing reflow), but ignore it when we have a hint that already requires reflow.)

layout/reftests/position-dynamic-changes/horizontal/fromauto-leftN-widthA-rightA-2.html seems to be affected by the same problem as bug 688545, which is probably an invalidation bug. I will mark the manifest as such before landing.

This is the same as the previous patch, except that I added a couple of test cases for relative positioning as well.

This is the implementation patch, the comments at the beginning of the patch explain what it does.

Comment on attachment 611042 [details] [diff] [review] Part 2: Implementation Here are review comments on everything except for the implementation of nsCSSFrameConstructor::RecomputePosition and the commit message (which I hope to get to tomorrow): We have an assumption right now that the processing of a style hint also applies that style hint to all descendants, except for the ones listed in nsFrameManager::ReResolveStyleContext at http://hg.mozilla.org/mozilla-central/file/4c43cfe73516/layout/base/nsFrameManager.cpp#l1067 You should add your new hint to that code... although, actually, the better fix would be to add the union of the bitfields listed there and your new nsChangeHint_RecomputePosition as an additional value to the enum nsChangeHint, with a comment explaining the distinction, so that it's more likely that those adding new bits will notice that they might need to consider this. Also please add a test that fails without this -- i.e., one that makes simultaneous (i.e., without a flush between) changes to the offset properties on an ancestor and a descendant. nsCSSFrameConstructor.cpp: >@@ -7931,6 +7933,12 @@ nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList& aChangeList) > ApplyRenderingChangeToTree(presContext, frame, hint); > didInvalidate = true; > } >+ if (hint & nsChangeHint_RecomputePosition) { This condition could check && !didReflow just like the UpdateOverflow hint does. STILL NEED TO REVIEW CONTENTS OF nsCSSFrameConstructor::RecomputePosition Need to think about: - does this work correctly when done on the root element's frame? nsHTMLReflowState.cpp: In the defintion of the function, please change: void to: /* static */ void so it's clear that it's static. Please make aComputedOffsets the last parameter of ComputeRelativeOffsets since it's the output. Also, just drop the aPresContext parameter entirely and use: FrameProperties props = aFrame->Properties(); rather than messing with getting a pres context and messing with the complicated construction of a FrameProperties object. nsStyleStruct.cpp: You don't need to change the maxHint above the Display struct comparison, since you're only changing things related to the Position struct. (Also, reading this code led me to file bug 741012.)

(In reply to David Baron [:dbaron] from comment #65) > We have an assumption right now that the processing of a style hint also > applies that style hint to all descendants, except for the ones listed > in nsFrameManager::ReResolveStyleContext at > http://hg.mozilla.org/mozilla-central/file/4c43cfe73516/layout/base/ > nsFrameManager.cpp#l1067 > You should add your new hint to that code... although, actually, the > better fix would be to add the union of the bitfields listed there and > your new nsChangeHint_RecomputePosition as an additional value to the > enum nsChangeHint, with a comment explaining the distinction, so that > it's more likely that those adding new bits will notice that they might > need to consider this. Also, as I said in bug 741012 comment 4, you should also add an assertion to the DO_STRUCT_DIFFERENCE macro in nsStyleContext::CalcDifference that nsStyle##struct_::ForceCompare() || NS_SubtractHint(nsStyle##struct_::MaxDifference(), nsChangeHint_NonInherited_Hints) == nsStyle##struct_::MaxDifference() where nsChangeHint_NonInherited_Hints is the new value I'm asking that you add. (nsStylePosition's ForceCompare does return true, but nothing checks that.)

Oh, so... I thought you'd come up with a solution where you compute the width and then decide whether to reflow, rather than taking the conservative route (which won't help nearly as many cases). What led you not to do that? I still think it's worth trying.

(In reply to David Baron [:dbaron] from comment #67) > Oh, so... I thought you'd come up with a solution where you compute the > width and then decide whether to reflow, rather than taking the conservative > route (which won't help nearly as many cases). What led you not to do that? > I still think it's worth trying. I'd like to take on doing that in a follow-up bug, as it can be done with a small change to nsCSSFrameConstructor::RecomputePosition(), but requires a bit more testing.

Addressed the review comments so far.

I'm getting reftest failures like this on Windows: https://tbpl.mozilla.org/php/getParsedLog.php?id=10588925&tree=Try If you look at the move-top-left.html image difference, it seems like I'm invalidating one pixel less that what is necessary, but I don't know why this happens. Is invalidating the overflow rect not enough?

Invalidating the overflow rect should be enough. Make sure both the old and new areas are invalidated.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72) > Invalidating the overflow rect should be enough. Make sure both the old and > new areas are invalidated. I invalid the overflow rect once before I change the position of the frame and once after it. That should be enough, right?

Yes. This failure looks to me like the text overflow area isn't including all of the antialiased pixels. I thought we added enough fuzz on Windows now to avoid that? Maybe not. You can work around this by adding padding:1px or 2px to some of those elements, I guess.

But wouldn't that be just a wallpaper to make the tests pass? The trail caused by moving the text node is actually visible on the screen. I don't know if that's acceptable!

Add padding to the tests and then file a bug about the overflow area issue and assign it to Jonathan :-).

Heh, OK, filed bug 742176.

Try run for 2cf3cc3351c4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2cf3cc3351c4 Results (out of 217 total builds): success: 180 warnings: 37 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-2cf3cc3351c4

Strangely, adding the padding did not fix the reftest failure on Linux!

(In reply to Ehsan Akhgari [:ehsan] from comment #80) > Strangely, adding the padding did not fix the reftest failure on Linux! Sorry meant Windows.

Comment on attachment 611632 [details] [diff] [review] Part 2: Implementation Sorry for dropping this for so long. >+ if ((hint & nsChangeHint_RecomputePosition) && !didReflow) { Actually, now that I look at this again, this is broken, as is the existing code that does this for UpdateOverflow. If didReflow had a variant local to an iteration of the while loop, it would be ok. Or we could skip it. (We should add a testcase for the brokenness with UpdateOverflow. It involves doing a reflow in one part of the tree and updating transform in another.) >+ NS_ASSERTION(nsStyle##struct_::ForceCompare() || \ >+ (int)NS_SubtractHint(nsStyle##struct_::MaxDifference(), \ >+ nsChangeHint_NonInherited_Hints) == \ >+ (int)nsStyle##struct_::MaxDifference(), \ >+ "nsChangeHint_NonInherited_Hints set but ForceCompare() returns false"); \ Why the (int) casts? They both return nsChangeHint. Also, how about making the assertion text: Structs that can return non-inherited hints must return true from ForceCompare and wrapping it across two lines (just two strings adjacent to each other, but with a space at the trailing end of the first) And why is the test using mozRequestAnimationFrame rather than a MozReftestInvalidate event? The remainder of the review is about nsCSSFrameConstructor::RecomputePosition: Something should have a comment that the return value of RecomputePosition returns whether it avoided the need to reflow. After you call ComputeRelativeOffsets you should assert that its result has left == -right and top == -bottom. >+ // For absolute positioning, the width can potentially change if width is auto and >+ // either of left or right are not. The height can also potentially change if height >+ // is auto and either of top or bottom are not. In these cases we fall back to a >+ // reflow, and in all other cases, we attempt to move the frame here. You should make it clearer that "these cases" means "where the height/width might change" but that we intend to change it to "where it does change". Also, please wrap at less than 80 characters. (There are also a few other 80th column violations that you should fix.) >+ if (!((position->mWidth.GetUnit() == eStyleUnit_Auto && >+ (position->mOffset.GetLeftUnit() != eStyleUnit_Auto || >+ position->mOffset.GetRightUnit() != eStyleUnit_Auto)) || >+ (position->mHeight.GetUnit() == eStyleUnit_Auto && >+ (position->mOffset.GetTopUnit() != eStyleUnit_Auto || >+ position->mOffset.GetBottomUnit() != eStyleUnit_Auto)))) { I think the tests of mOffset here should all go; you should just be checking mWidth and mHeight. Without that, I think you'll have the bug that a change from: width: auto; left: 300px; right: auto; to: width: auto; left: auto; right: auto; will fail to cause the necessary resizing (since both the before and after cases are shrink-to-fit, but with 300px of difference in the container width). Please fix and add a test for this case. + if (parentSize.height != NS_INTRINSICSIZE) + parentSize.height += margin.TopBottom(); + if (parentSize.width != NS_INTRINSICSIZE) + parentSize.width += margin.LeftRight(); I think you should instead assert that parentSize.width and parentSize.height are not NS_INTRINSICSIZE. Or did you actually hit this case? Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT (which is the same as NS_INTRINSICSIZE). Setting a constrained height in the nsSize passed to a reflow state indicates it's for pagination. (You should be able to construct a test that breaks, I suspect, by positioning an abs pos element so it overflows the bottom of its parent, and then triggering this code.) >+ nsHTMLReflowState parentReflowState(aFrame->PresContext(), parentFrame, >+ rc, >+ parentSize); Merge the second and third lines. >+ if (parentSize.width != NS_INTRINSICSIZE) >+ parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0)); >+ if (parentSize.height != NS_INTRINSICSIZE) >+ parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0)); These should be content-box sizes (and should use the real height, unlike the reflow state constructor). >+ parentReflowState.mComputedMargin.SizeTo(0, 0, 0, 0); This is reasonable (and simplifies things), but it means that you shouldn't have bothered inflating parentSize by the margin earlier on. (It's a pattern we use in nsFrame::BoxReflow, but the point of it is that we don't need to worry about anything outside the parent's border-box, so we pretend the parent's margins are 0.) >+ parentFrame->GetPadding(parentReflowState.mComputedPadding); >+ parentFrame->GetBorder(parentReflowState.mComputedBorderPadding); >+ parentReflowState.mComputedBorderPadding += >+ parentReflowState.mComputedPadding; Those are XUL box layout methods. Instead, use nsIFrame::GetUsedPadding and nsIFrame::GetUsedBorderAndPadding. (Same for GetUsedMargin earlier, except it needs removing.) >+ nsHTMLReflowState reflowState(aFrame->PresContext(), parentReflowState, aFrame, >+ availSize, -1, -1, false); I see no reason to use the delayed-init form (false for last argument). Just move this down to your Init call, wrap at less than 80 columns, and pass cbSize.width and cbSize.height instead of -1, -1. >+ // mComputedWidth and mComputedHeight are content-box, not >+ // border-box >+ if (parentSize.width != NS_INTRINSICSIZE) { >+ nscoord computedWidth = >+ parentSize.width - reflowState.mComputedBorderPadding.LeftRight(); >+ computedWidth = NS_MAX(computedWidth, 0); >+ reflowState.SetComputedWidth(computedWidth); >+ } Again, parentSize.width should never be NS_INTRINSICSIZE. Also, why is this needed? Shouldn't the reflow state constructor have set it correctly? And, also, I don't see how reflowState.ComputedWidth() is used later at any point, so I think it's pretty clearly unused. >+ // If we're solving for 'left' or 'top', then compute it now that we know the >+ // width/height. This comment is a little misleading. We always knew it. It's just that in the normal reflow codepath we don't, and thus in the normal reflow codepath it's done after reflow, so we have to do it ourselves here. Also wrap the comment at less than 80 columns. >+ nsPoint pos(border.left + reflowState.mComputedOffsets.left + reflowState.mComputedMargin.left, >+ border.top +reflowState.mComputedOffsets.top + reflowState.mComputedMargin.top); Fix the weird spacing after the first + on the second line. (Also, maybe |border| would be better called |parentBorder|?) r=dbaron with those things fixed Notes about tests: Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px 27px" tested separately for each of the 6) on the abs-pos element and on its parent. Need tests with absolutely positioned root element.

Comment on attachment 612095 [details] [diff] [review] Part 1: Add a test suite Why requestAnimationFrame rather than MozReftestInvalidate? And I think you need to add some tests per the last 2 paragraphs of my previous comment. But r=dbaron on these (though I can't claim anything close to reading the whole thing in detail) (Have you double-checked (e.g., with printfs) that these tests are hitting your new codepath.)

(In reply to David Baron [:dbaron] from comment #82) > Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px > 27px" tested separately for each of the 6) on the abs-pos element and on its > parent. Let me doubly-emphasize this part. I have low confidence in my ability to catch all mistakes relating to margin, border, and padding during review. You need tests for it. > Need tests with absolutely positioned root element. These don't necessarily need to be as thorough as the previous point, but you should have a few that actually trigger your new code (and, in particular, involving margin, border, and padding). It's possible some of these could be written as mochitests rather than reftests.

(In reply to David Baron [:dbaron] from comment #82) > Comment on attachment 611632 [details] [diff] [review] > Part 2: Implementation > > Sorry for dropping this for so long. > > >+ if ((hint & nsChangeHint_RecomputePosition) && !didReflow) { > > Actually, now that I look at this again, this is broken, as is the > existing code that does this for UpdateOverflow. If didReflow had a > variant local to an iteration of the while loop, it would be ok. Or we > could skip it. > > (We should add a testcase for the brokenness with UpdateOverflow. It > involves doing a reflow in one part of the tree and updating transform > in another.) I couldn't come up with a test case which would trigger this. Here's one of the things I tried for example: https://gist.github.com/2387812. The style changes would always end up being processed in two separate change lists. didReflow is also used outside of the loop, so moving it is not trivial. Can we do this in another bug? > >+ NS_ASSERTION(nsStyle##struct_::ForceCompare() || \ > >+ (int)NS_SubtractHint(nsStyle##struct_::MaxDifference(), \ > >+ nsChangeHint_NonInherited_Hints) == \ > >+ (int)nsStyle##struct_::MaxDifference(), \ > >+ "nsChangeHint_NonInherited_Hints set but ForceCompare() returns false"); \ > > Why the (int) casts? They both return nsChangeHint. operator== for nsChangeHint is declared to return void, which makes me think that is done on purpose to make sure people who compare nsChangeHints know what they're doing. :-) > >+ if (!((position->mWidth.GetUnit() == eStyleUnit_Auto && > >+ (position->mOffset.GetLeftUnit() != eStyleUnit_Auto || > >+ position->mOffset.GetRightUnit() != eStyleUnit_Auto)) || > >+ (position->mHeight.GetUnit() == eStyleUnit_Auto && > >+ (position->mOffset.GetTopUnit() != eStyleUnit_Auto || > >+ position->mOffset.GetBottomUnit() != eStyleUnit_Auto)))) { > > I think the tests of mOffset here should all go; you should just be checking > mWidth and mHeight. Without that, I think you'll have the bug that a change > from: > width: auto; > left: 300px; > right: auto; > to: > width: auto; > left: auto; > right: auto; > will fail to cause the necessary resizing (since both the before and after > cases are shrink-to-fit, but with 300px of difference in the container > width). Please fix and add a test for this case. Right, good catch. I fixed this and added a test. > + if (parentSize.height != NS_INTRINSICSIZE) > + parentSize.height += margin.TopBottom(); > + if (parentSize.width != NS_INTRINSICSIZE) > + parentSize.width += margin.LeftRight(); > > I think you should instead assert that parentSize.width and > parentSize.height are not NS_INTRINSICSIZE. Or did you actually > hit this case? No, I just took the code from here: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7697> > Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT > (which is the same as NS_INTRINSICSIZE). Setting a constrained height > in the nsSize passed to a reflow state indicates it's for pagination. > > (You should be able to construct a test that breaks, I suspect, by > positioning an abs pos element so it overflows the bottom of its parent, > and then triggering this code.) Hmm, I'm not sure what the breakage should look like. I tried constructing such a test case https://gist.github.com/2387996 but couldn't see any failures. > >+ if (parentSize.width != NS_INTRINSICSIZE) > >+ parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0)); > >+ if (parentSize.height != NS_INTRINSICSIZE) > >+ parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0)); > > These should be content-box sizes (and should use the real height, > unlike the reflow state constructor). I took this code from <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7712>. The border and padding on the parent RS is not computed, so I'm not sure how to set the content-box size on it. > Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px > 27px" tested separately for each of the 6) on the abs-pos element and on its > parent. Not sure what you mean by "each of the 6". I'm assuming you meant "each of the 3" (of course there's going to be 6 cases in total, 3 on the abs-pos element and 3 on its parent. Will submit the new patches soon.

(In reply to David Baron [:dbaron] from comment #83) > Comment on attachment 612095 [details] [diff] [review] > Part 1: Add a test suite > > Why requestAnimationFrame rather than MozReftestInvalidate? MozReftestInvalidate only fires once for these tests, so it's not useful. Also, I wanted the tests to be useful outside of the reftest framework as well. > (Have you double-checked (e.g., with printfs) that these tests are hitting > your new codepath.) Yeah, I've stepped through the code under the debugger to make sure that these tests do test the new code path.

(In reply to Ehsan Akhgari [:ehsan] from comment #85) > (In reply to David Baron [:dbaron] from comment #82) > > Comment on attachment 611632 [details] [diff] [review] > > Part 2: Implementation > > > > Sorry for dropping this for so long. > > > > >+ if ((hint & nsChangeHint_RecomputePosition) && !didReflow) { > > > > Actually, now that I look at this again, this is broken, as is the > > existing code that does this for UpdateOverflow. If didReflow had a > > variant local to an iteration of the while loop, it would be ok. Or we > > could skip it. > > > > (We should add a testcase for the brokenness with UpdateOverflow. It > > involves doing a reflow in one part of the tree and updating transform > > in another.) > > I couldn't come up with a test case which would trigger this. Here's one of > the things I tried for example: https://gist.github.com/2387812. The style > changes would always end up being processed in two separate change lists. Hmmm. It does have to be something where both changes end up in the same call to ProcessRestyledFrames. It's not clear to me why that case wouldn't be. (And, also, the reflow would have to happen first.) > didReflow is also used outside of the loop, so moving it is not trivial. > Can we do this in another bug? It's not moving it that's needed -- the fix is pretty trivial: have a |didReflowThisFrame| inside the loop and a |didReflow| outside the loop. > > Why the (int) casts? They both return nsChangeHint. > > operator== for nsChangeHint is declared to return void, which makes me think > that is done on purpose to make sure people who compare nsChangeHints know > what they're doing. :-) ok. (You could also write it using NS_IsHintSubset and ~, i.e., saying that MaxDifference() is a subset of ~NonInherited_Hints.) > > Except, also, you want to just set parentSize.height to NS_AUTOHEIGHT > > (which is the same as NS_INTRINSICSIZE). Setting a constrained height > > in the nsSize passed to a reflow state indicates it's for pagination. > > > > (You should be able to construct a test that breaks, I suspect, by > > positioning an abs pos element so it overflows the bottom of its parent, > > and then triggering this code.) > > Hmm, I'm not sure what the breakage should look like. I tried constructing > such a test case https://gist.github.com/2387996 but couldn't see any > failures. Yeah, it may well be asymptomatic because you manually construct the child's reflow state correctly. > > >+ if (parentSize.width != NS_INTRINSICSIZE) > > >+ parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0)); > > >+ if (parentSize.height != NS_INTRINSICSIZE) > > >+ parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0)); > > > > These should be content-box sizes (and should use the real height, > > unlike the reflow state constructor). > > I took this code from > <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame. > cpp#7712>. The border and padding on the parent RS is not computed, so I'm > not sure how to set the content-box size on it. Well, it still seems wrong, but maybe it's right, or maybe it just doesn't matter that it's wrong. > > Need tests with margin, padding, border (varying values, e.g., "1px 3px 9px > > 27px" tested separately for each of the 6) on the abs-pos element and on its > > parent. > > Not sure what you mean by "each of the 6". I'm assuming you meant "each of > the 3" (of course there's going to be 6 cases in total, 3 on the abs-pos > element and 3 on its parent. Exactly.

(In reply to David Baron [:dbaron] from comment #87) > (In reply to Ehsan Akhgari [:ehsan] from comment #85) > > (In reply to David Baron [:dbaron] from comment #82) > > > Comment on attachment 611632 [details] [diff] [review] > > > Part 2: Implementation > > > > > > Sorry for dropping this for so long. > > > > > > >+ if ((hint & nsChangeHint_RecomputePosition) && !didReflow) { > > > > > > Actually, now that I look at this again, this is broken, as is the > > > existing code that does this for UpdateOverflow. If didReflow had a > > > variant local to an iteration of the while loop, it would be ok. Or we > > > could skip it. > > > > > > (We should add a testcase for the brokenness with UpdateOverflow. It > > > involves doing a reflow in one part of the tree and updating transform > > > in another.) > > > > I couldn't come up with a test case which would trigger this. Here's one of > > the things I tried for example: https://gist.github.com/2387812. The style > > changes would always end up being processed in two separate change lists. > > Hmmm. It does have to be something where both changes end up in the same > call to ProcessRestyledFrames. It's not clear to me why that case wouldn't > be. (And, also, the reflow would have to happen first.) > > > didReflow is also used outside of the loop, so moving it is not trivial. > > Can we do this in another bug? > > It's not moving it that's needed -- the fix is pretty trivial: have a > |didReflowThisFrame| inside the loop and a |didReflow| outside the loop. Hmm, OK, filed bug 745493 to add a test case once we figure out how to trigger the broken scenario here, but I will fix this in my patch anyway. > > > Why the (int) casts? They both return nsChangeHint. > > > > operator== for nsChangeHint is declared to return void, which makes me think > > that is done on purpose to make sure people who compare nsChangeHints know > > what they're doing. :-) > > ok. (You could also write it using NS_IsHintSubset and ~, i.e., saying that > MaxDifference() is a subset of ~NonInherited_Hints.) OK, that is cleaner I think, I'll do that. > > > >+ if (parentSize.width != NS_INTRINSICSIZE) > > > >+ parentReflowState.SetComputedWidth(NS_MAX(parentSize.width, 0)); > > > >+ if (parentSize.height != NS_INTRINSICSIZE) > > > >+ parentReflowState.SetComputedHeight(NS_MAX(parentSize.height, 0)); > > > > > > These should be content-box sizes (and should use the real height, > > > unlike the reflow state constructor). > > > > I took this code from > > <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame. > > cpp#7712>. The border and padding on the parent RS is not computed, so I'm > > not sure how to set the content-box size on it. > > Well, it still seems wrong, but maybe it's right, or maybe it just doesn't > matter that it's wrong. I added tests for the border/padding/margin cases, and given that all of them pass, I think one of the two latter cases are true.

I submitted this to the try server: http://tbpl.mozilla.org/?tree=Try&rev=01e4ec2e01c3 David, please let me know if you want to look at the new patches again, or if you're OK with me landing them.

So, there's a test failure with this patch on OS X, which I debugged a bit: https://tbpl.mozilla.org/php/getParsedLog.php?id=10914771&tree=Try The bug is caused because the overflow event is not fired when it should be <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#3034>. As far as I know, this is the result of the overflow areas not be updated properly. I always set the RecomputePosition hint together with the UpdateOverflow hint, which _should_ cause the overflow area on the frame's parent to be updated correctly, right? Do we also need to update the overflow area on other descendants? Or do we need to do something differently here?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76) > Add padding to the tests and then file a bug about the overflow area issue > and assign it to Jonathan :-). Also, adding the padding does not seem to help with this problem. Moving the inline text frame still leaves a trail on the edge of the moved area. I'm not sure how to proceed here.

Try run for 01e4ec2e01c3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=01e4ec2e01c3 Results (out of 223 total builds): success: 168 warnings: 55 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-01e4ec2e01c3

(In reply to Ehsan Akhgari [:ehsan] from comment #92) > So, there's a test failure with this patch on OS X, which I debugged a bit: > https://tbpl.mozilla.org/php/getParsedLog.php?id=10914771&tree=Try > > The bug is caused because the overflow event is not fired when it should be > <http://mxr.mozilla.org/mozilla-central/source/layout/generic/ > nsGfxScrollFrame.cpp#3034>. As far as I know, this is the result of the > overflow areas not be updated properly. > > I always set the RecomputePosition hint together with the UpdateOverflow > hint, which _should_ cause the overflow area on the frame's parent to be > updated correctly, right? Do we also need to update the overflow area on > other descendants? Or do we need to do something differently here? Or it could be the general problem that we don't seem to fire the overflow/underflow events as a result of the UpdateOverflow hint as far as I can tell...

The previous test patch had a mistake in the relative tests which is fixed in this patch.

Pushed to try on a more recent m-c base revision: https://tbpl.mozilla.org/?tree=Try&rev=7476088374d6

Rebased version of the patch.

Rebased version of the patch.

Oops, wrong patch.

Try run for 7476088374d6 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7476088374d6 Results (out of 238 total builds): success: 194 warnings: 37 failure: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-7476088374d6

FWIW, here's a screenshot showing the failure that's happening in browser_tabview_bug696602.js. The testcase creates a series of about 9 new tabs in the window, and expects that the tab-strip will overflow; but as can be seen in the screenshot, with this patch applied, the new tabs seem to all be getting "stacked" behind the right-hand edge of the original tab.

Adjusting nsStylePosition::CalcDifference so that "auto" height or widths aren't treated as being the same seems to prevent the tab-strip test failure, although I don't really understand the underlying reason it was failing. Note that this patch triggers some additional nsCoord-overflow assertions in layout/generic/crashtests/265867-1.html (it's probably making us re-calculate stuff that we shouldn't need to), so I'm not sure it's necessarily a good idea, but it may give a clue where to track down the problem.

Comment on attachment 620820 [details] [diff] [review] patch, don't consider height/width the same if they're "auto" > if (mHeight != aOther.mHeight || > mMinHeight != aOther.mMinHeight || >- mMaxHeight != aOther.mMaxHeight) { >+ mMaxHeight != aOther.mMaxHeight || >+ mHeight.GetUnit() == eStyleUnit_Auto) { This causes us to reflow for *any* style change, as long as 'height' is auto (and we've previously looked at the position struct). That's not acceptable. (And also probably why it's capable of covering up all sorts of failures.) I also think it's better to keep the when-to-fallback-to-reflow logic concentrated in nsCSSFrameConstructor::RecomputePosition, where the logic already lives.

Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=b2c2a0f90b5c

So I looked into this a bit more. If we skip reflow and just update the overflow areas, the overflow logic in the scroll frame code does not get triggered at all, and I'm pretty sure that's a bug. But I don't know what the best way to fix that is.

(In reply to Ehsan Akhgari [:ehsan] from comment #106) > So I looked into this a bit more. If we skip reflow and just update the > overflow areas, the overflow logic in the scroll frame code does not get > triggered at all, and I'm pretty sure that's a bug. But I don't know what > the best way to fix that is. Hmm, actually perhaps that's not the case. It seems like my patch has a bug which causes us to skip reflowing if {max,min}-{width,height} also changes. Fixing that seems to fix the test failures. I'll attach the patch once I do a bit more local testing.

OK, this patch fixes the remaining test failures. I also added stand-alone unit tests for this. Basically we need to make sure that we still reflow when max-width and min-width changes, same as we did before this patch. I'm submitting this separately to make the review easier, but will squash this into the implementation patch upon landing.

(In reply to Ehsan Akhgari [:ehsan] from comment #109) > http://tbpl.mozilla.org/?tree=Try&rev=13382c3864f4 Pushed the wrong patch; http://tbpl.mozilla.org/?tree=Try&rev=e3ab610c8333

Comment on attachment 630241 [details] [diff] [review] Make sure to reflow when {min,max}-width changes r=dbaron, though I think it would be cleaner to restructure a little, by putting the *final* return earlier. Essentially to restructure all the code that comes after the (untouched): if (mHeight != aOther.mHeight || mMinHeight != aOther.mMinHeight || mMaxHeight != aOther.mMaxHeight) { // Height changes can affect descendant intrinsic sizes due to replaced // elements with percentage heights in descendants which also have // percentage heights. And due to our not-so-great computation of mVResize // in nsHTMLReflowState, they do need to force reflow of the whole subtree. // XXXbz due to XUL caching heights as well, height changes also need to // clear ancestor intrinsics! return NS_CombineHint(hint, nsChangeHint_ReflowFrame); } by starting what goes after that with a parallel clause: if (mWidth != aOther.mWidth || mMinWidth != aOther.mMinWidth || mMaxWidth != aOther.mMaxWidth) { // put what's now the final return here } and then continue with the check of offset (which I think can be condensed back into mOffset == aOther.mOffset -- I don't see why the patch this is on top of separates it out into the four pieces), i.e., the two things that are inside the check that you're modifying here. I think that's equivalent, but it ends up a good bit easier to understand.

(And it avoids having to duplicate the checks of the {min-,max,}{height,width} properties.)

Right, this makes the code a lot easier to read!

Now, on the new test that I added, there is an OS X failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=12391287&tree=Try#error0. I'll file a new bug for this and will mark this test as fuzzy on Mac.

(In reply to Ehsan Akhgari [:ehsan] from comment #114) > Now, on the new test that I added, there is an OS X failure: > https://tbpl.mozilla.org/php/getParsedLog.php?id=12391287&tree=Try#error0. > I'll file a new bug for this and will mark this test as fuzzy on Mac. -> bug 761770

Comment on attachment 630296 [details] [diff] [review] Simplify nsStylePosition::CalcDifference >+ if (mOffset.GetLeft() != aOther.mOffset.GetLeft() || >+ mOffset.GetRight() != aOther.mOffset.GetRight() || >+ mOffset.GetTop() != aOther.mOffset.GetTop() || >+ mOffset.GetBottom() != aOther.mOffset.GetBottom()) { But also please switch this back to just mOffset != aOther.mOffset (like it was before the first patch in this bug), unless I'm missing a reason not to do this.

(In reply to David Baron [:dbaron] from comment #116) > Comment on attachment 630296 [details] [diff] [review] > Simplify nsStylePosition::CalcDifference > > >+ if (mOffset.GetLeft() != aOther.mOffset.GetLeft() || > >+ mOffset.GetRight() != aOther.mOffset.GetRight() || > >+ mOffset.GetTop() != aOther.mOffset.GetTop() || > >+ mOffset.GetBottom() != aOther.mOffset.GetBottom()) { > > But also please switch this back to just mOffset != aOther.mOffset (like it > was before the first patch in this bug), unless I'm missing a reason not to > do this. Will do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59ff36c33e69 https://hg.mozilla.org/integration/mozilla-inbound/rev/df6702c41ddd

Thanks for getting this in. Please do get a followup bug filed on comment 68 -- I'm concerned that if we have the current behavior for any substantial period of time, authors will start sharing performance advice to prefer the cases that this happens to optimize, which is contradictory to good cross-browser and cross-device design practice.

(In reply to David Baron [:dbaron] from comment #119) > Thanks for getting this in. > > Please do get a followup bug filed on comment 68 -- I'm concerned that if we > have the current behavior for any substantial period of time, authors will > start sharing performance advice to prefer the cases that this happens to > optimize, which is contradictory to good cross-browser and cross-device > design practice. Already did: bug 745485.

https://hg.mozilla.org/mozilla-central/rev/59ff36c33e69 https://hg.mozilla.org/mozilla-central/rev/df6702c41ddd

This was backed out from mozilla-beta due to bug 775350. https://hg.mozilla.org/releases/mozilla-beta/rev/3d7907e9e5a9