Last Comment Bug 157681 - [DHTML]We reflow when we should just move
: [DHTML]We reflow when we should just move
Status: RESOLVED FIXED
[adt2][games:p3]
: perf, testcase, topembed+
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P2 major with 13 votes (vote)
: mozilla16
Assigned To: :Ehsan Akhgari
:
: Jet Villegas (:jet)
Mentors:
http://dbaron.org/css/test/2003/abs-p...
Depends on: 824952 884468 909667 1066917 1200585 158713 160936 171830 201897 reflow-refactor 502288 524925 761980 762180 762181 762546 763223 765597 766116 766843 775350 782062 844178 845837 854760 861174 872254 894629 1248727
Blocks: 21762 186465 500410 gecko-games 107746 117436 117601 124178 139986 143097 149216 178882 186442 188331 203448 286900 703873 729597 745485
  Show dependency treegraph
 
Reported: 2002-07-16 01:49 PDT by Markus Hübner
Modified: 2016-02-16 14:44 PST (History)
66 users (show)
dbaron: blocking1.3b-
asa: blocking1.4a-
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
disabled
disabled


Attachments
Profile (52.83 KB, application/x-gzip)
2003-03-01 09:16 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Part 1: Add a test suite (91.11 KB, patch)
2012-03-27 17:16 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: Add a test suite (95.73 KB, patch)
2012-03-30 14:38 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implementation (21.62 KB, patch)
2012-03-30 14:39 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implementation (25.50 KB, patch)
2012-04-02 15:55 PDT, :Ehsan Akhgari
dbaron: review+
Details | Diff | Splinter Review
Part 1: Add a test suite (95.84 KB, patch)
2012-04-03 21:18 PDT, :Ehsan Akhgari
dbaron: review+
Details | Diff | Splinter Review
Part 1: Add a test suite (122.91 KB, patch)
2012-04-14 17:49 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implementation (28.30 KB, patch)
2012-04-14 17:50 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: Add a test suite (122.91 KB, patch)
2012-04-18 13:09 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: Add a test suite (122.91 KB, patch)
2012-04-30 12:46 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implementation (122.91 KB, patch)
2012-04-30 12:47 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 2: Implementation (28.30 KB, patch)
2012-04-30 12:48 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
screenshot of mochitest-browser-chrome test failure (44.59 KB, image/png)
2012-05-01 04:03 PDT, Jonathan Kew (:jfkthame)
no flags Details
patch, don't consider height/width the same if they're "auto" (2.10 KB, patch)
2012-05-03 12:52 PDT, Jonathan Kew (:jfkthame)
dbaron: review-
Details | Diff | Splinter Review
Make sure to reflow when {min,max}-width changes (3.91 KB, patch)
2012-06-05 11:41 PDT, :Ehsan Akhgari
dbaron: review+
Details | Diff | Splinter Review
Simplify nsStylePosition::CalcDifference (3.07 KB, patch)
2012-06-05 13:08 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review

Description Markus Hübner 2002-07-16 01:49:30 PDT
results on win-xp pro,1.1ghz,512ram.
trunk 2002071508: 851ms
msie6:            411ms
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2002-07-16 01:56:23 PDT
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.
Comment 2 Zibi Braniecki [:gandalf][:zibi] 2002-07-16 02:26:24 PDT
Athlon 750

Moz trunk 2002070304 - 2340
IE6 - 428
Comment 3 Damir Perisa 2002-07-16 02:43:18 PDT
PII 366MHz 160MB win98

Mozilla 2002071508 (1.1+) : 3.680s
IE6                       : 1.040s
Comment 4 Jaime Rodriguez, Jr. 2002-07-24 07:28:16 PDT
Nomianting for nsbeta1 as this appears to improve performance.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2002-08-06 16:15:15 PDT
> 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.
Comment 6 Kevin McCluskey (gone) 2002-08-12 11:36:23 PDT
nsbeta1+
Comment 7 Kai Lahmann (is there, where MNG is) 2002-08-26 06:54:33 PDT
Athlon 1000, 512MB RAM, Debian+KDE3

1.1: 2676
konqueror 3.0.2: 1373
Comment 8 Kai Lahmann (is there, where MNG is) 2002-08-26 06:57:34 PDT
oh.. Opera 6.03/linux: 556 (!)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-03 10:18:14 PDT
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.
Comment 10 Markus Hübner 2002-09-08 17:24:20 PDT
This would be a major performance boost on overal DHTML and highly desired for 
1.2 - let's get things rockin'
Comment 11 David Baron :dbaron: ⌚️UTC-10 2002-09-10 19:06:02 PDT
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. :-)
Comment 12 David Baron :dbaron: ⌚️UTC-10 2002-09-10 19:07:46 PDT
(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.)
Comment 13 David Baron :dbaron: ⌚️UTC-10 2002-09-10 19:09:48 PDT
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"?
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-10 19:21:24 PDT
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.)
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-10 19:41:52 PDT
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.
Comment 16 Jaime Rodriguez, Jr. 2002-09-23 09:05:55 PDT
Nominating for topembed as DHTML perf is something we should address in the next
milestone for embedding customers.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 09:54:25 PDT
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.
Comment 18 David Baron :dbaron: ⌚️UTC-10 2002-09-23 09:55:37 PDT
Why do you think that?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 10:22:17 PDT
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?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2002-09-23 10:53:57 PDT
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.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 11:33:55 PDT
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.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 11:35:34 PDT
But yes, we definitely need a way to detect that style changes don't require
reflowing of children, and we need it *everywhere*.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2002-09-23 11:40:47 PDT
Yeah, dbaron was thinking of not using the parser hint there in any case...  We
should just do it. 
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 11:59:54 PDT
Do you feel like doing it, Mr Style System Peer? :-)
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2002-09-23 12:09:51 PDT
Once I have a tree again (eta: 2.5 weeks) I'll give it a shot, sure.  See bug
158713 for that, probably.
Comment 26 David Baron :dbaron: ⌚️UTC-10 2002-09-23 13:08:56 PDT
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.)
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 13:48:24 PDT
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.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2002-09-23 14:11:58 PDT
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.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-23 14:21:49 PDT
Always treating that as a size change probably isn't too bad.
Comment 30 David Baron :dbaron: ⌚️UTC-10 2002-09-23 16:42:33 PDT
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.)
Comment 31 David Baron :dbaron: ⌚️UTC-10 2002-09-23 16:44:07 PDT
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.
Comment 32 Jaime Rodriguez, Jr. 2002-10-16 10:15:55 PDT
Marking as topembed+, as it is desired by major embedding partners.
Comment 33 Zibi Braniecki [:gandalf][:zibi] 2002-12-13 12:42:46 PST
Moz 20021210: 3405
IE6         : 350
Opera7      : 10205
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-12-13 13:27:45 PST
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.
Comment 35 Christopher Blizzard (:blizzard) 2003-02-15 06:42:24 PST
This is 1.4 material?
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-02-15 09:04:39 PST
could be, although I think for many DHTML benchmarks painting is our bottleneck now
Comment 37 Markus Hübner 2003-03-01 01:49:18 PST
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]?
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-03-01 06:05:56 PST
I wonder why in this testcase we need to reflow at all between assignments to
style.left.
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2003-03-01 09:16:08 PST
Created attachment 115984 [details]
Profile

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.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-03-01 11:11:01 PST
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.
Comment 41 David Baron :dbaron: ⌚️UTC-10 2003-04-07 12:14:17 PDT
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.)
Comment 42 chris hofmann 2003-05-05 10:59:09 PDT
is this still on track for 1.5a?
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-05-07 20:39:02 PDT
This isn't a priority for me. nsIFrame deCOMtamination and bug fixing is all I
have on my radar.
Comment 44 Kevin McCluskey (gone) 2003-05-28 16:38:08 PDT
Moving to 1.6a. 1.5a is not realistic.
Comment 45 Markus Hübner 2003-05-29 13:04:33 PDT
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).
Comment 46 Markus Hübner 2003-06-23 04:51:39 PDT
roc/dbaron - anyone of you willing to take this ... kin seems to be able to 
get to this in the near future.
Comment 47 David Baron :dbaron: ⌚️UTC-10 2003-09-05 15:17:31 PDT
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.)
Comment 48 Markus Hübner 2003-11-20 05:32:37 PST
I guess this again will have to be retargeted ... 1.7a ?
Comment 49 David Baron :dbaron: ⌚️UTC-10 2003-11-20 11:42:24 PST
taking bug
Comment 51 Cees T. 2007-04-18 04:28:59 PDT
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.
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2009-07-03 15:16:30 PDT
Just to update on the state of things here, bug 300030 fixed the issue David talks about in comment 47.
Comment 53 Zack Weinberg (:zwol) 2011-04-01 08:49:45 PDT
Is it still worth keeping this bug open?
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-01 12:54:23 PDT
Yes, I think so.
Comment 55 :Ehsan Akhgari 2012-03-26 18:21:15 PDT
*** Bug 729597 has been marked as a duplicate of this bug. ***
Comment 56 Boris Zbarsky [:bz] (still a bit busy) 2012-03-27 09:09:13 PDT
*** Bug 729597 has been marked as a duplicate of this bug. ***
Comment 57 :Ehsan Akhgari 2012-03-27 14:16:43 PDT
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.)
Comment 58 David Baron :dbaron: ⌚️UTC-10 2012-03-27 14:24:39 PDT
(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.)
Comment 59 :Ehsan Akhgari 2012-03-27 17:16:22 PDT
Created attachment 609955 [details] [diff] [review]
Part 1: Add a test suite
Comment 61 :Ehsan Akhgari 2012-03-27 22:34:01 PDT
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.
Comment 62 :Ehsan Akhgari 2012-03-30 14:38:59 PDT
Created attachment 611041 [details] [diff] [review]
Part 1: Add a test suite

This is the same as the previous patch, except that I added a couple of test cases for relative positioning as well.
Comment 63 :Ehsan Akhgari 2012-03-30 14:39:45 PDT
Created attachment 611042 [details] [diff] [review]
Part 2: Implementation

This is the implementation patch, the comments at the beginning of the patch explain what it does.
Comment 65 David Baron :dbaron: ⌚️UTC-10 2012-03-30 23:17:24 PDT
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.)
Comment 66 David Baron :dbaron: ⌚️UTC-10 2012-03-31 10:30:16 PDT
(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.)
Comment 67 David Baron :dbaron: ⌚️UTC-10 2012-03-31 22:35:13 PDT
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.
Comment 68 :Ehsan Akhgari 2012-04-02 11:24:35 PDT
(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.
Comment 69 :Ehsan Akhgari 2012-04-02 15:55:27 PDT
Created attachment 611632 [details] [diff] [review]
Part 2: Implementation

Addressed the review comments so far.
Comment 71 :Ehsan Akhgari 2012-04-03 09:43:35 PDT
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?
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-03 13:54:46 PDT
Invalidating the overflow rect should be enough. Make sure both the old and new areas are invalidated.
Comment 73 :Ehsan Akhgari 2012-04-03 14:53:21 PDT
(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?
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-03 16:20:21 PDT
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.
Comment 75 :Ehsan Akhgari 2012-04-03 20:58:10 PDT
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!
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-03 21:05:48 PDT
Add padding to the tests and then file a bug about the overflow area issue and assign it to Jonathan :-).
Comment 77 :Ehsan Akhgari 2012-04-03 21:15:11 PDT
Heh, OK, filed bug 742176.
Comment 78 :Ehsan Akhgari 2012-04-03 21:18:54 PDT
Created attachment 612095 [details] [diff] [review]
Part 1: Add a test suite
Comment 79 Mozilla RelEng Bot 2012-04-04 11:31:54 PDT
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
Comment 80 :Ehsan Akhgari 2012-04-04 11:46:02 PDT
Strangely, adding the padding did not fix the reftest failure on Linux!
Comment 81 :Ehsan Akhgari 2012-04-04 13:15:50 PDT
(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 82 David Baron :dbaron: ⌚️UTC-10 2012-04-13 15:29:05 PDT
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 83 David Baron :dbaron: ⌚️UTC-10 2012-04-13 15:33:10 PDT
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.)
Comment 84 David Baron :dbaron: ⌚️UTC-10 2012-04-13 15:37:09 PDT
(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.
Comment 85 :Ehsan Akhgari 2012-04-14 15:16:42 PDT
(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.
Comment 86 :Ehsan Akhgari 2012-04-14 15:18:04 PDT
(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.
Comment 87 David Baron :dbaron: ⌚️UTC-10 2012-04-14 16:01:43 PDT
(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.
Comment 88 :Ehsan Akhgari 2012-04-14 17:18:58 PDT
(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.
Comment 89 :Ehsan Akhgari 2012-04-14 17:49:48 PDT
Created attachment 615094 [details] [diff] [review]
Part 1: Add a test suite
Comment 90 :Ehsan Akhgari 2012-04-14 17:50:18 PDT
Created attachment 615095 [details] [diff] [review]
Part 2: Implementation
Comment 91 :Ehsan Akhgari 2012-04-14 17:51:03 PDT
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.
Comment 92 :Ehsan Akhgari 2012-04-14 21:04:20 PDT
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?
Comment 93 :Ehsan Akhgari 2012-04-14 21:06:19 PDT
(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.
Comment 94 Mozilla RelEng Bot 2012-04-14 21:15:21 PDT
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
Comment 95 :Ehsan Akhgari 2012-04-14 21:18:28 PDT
(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...
Comment 96 :Ehsan Akhgari 2012-04-18 13:09:20 PDT
Created attachment 616263 [details] [diff] [review]
Part 1: Add a test suite

The previous test patch had a mistake in the relative tests which is fixed in this patch.
Comment 97 :Ehsan Akhgari 2012-04-30 10:33:55 PDT
Pushed to try on a more recent m-c base revision: https://tbpl.mozilla.org/?tree=Try&rev=7476088374d6
Comment 98 :Ehsan Akhgari 2012-04-30 12:46:42 PDT
Created attachment 619650 [details] [diff] [review]
Part 1: Add a test suite

Rebased version of the patch.
Comment 99 :Ehsan Akhgari 2012-04-30 12:47:48 PDT
Created attachment 619651 [details] [diff] [review]
Part 2: Implementation

Rebased version of the patch.
Comment 100 :Ehsan Akhgari 2012-04-30 12:48:53 PDT
Created attachment 619653 [details] [diff] [review]
Part 2: Implementation

Oops, wrong patch.
Comment 101 Mozilla RelEng Bot 2012-04-30 20:15:22 PDT
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
Comment 102 Jonathan Kew (:jfkthame) 2012-05-01 04:03:40 PDT
Created attachment 619887 [details]
screenshot of mochitest-browser-chrome test failure

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.
Comment 103 Jonathan Kew (:jfkthame) 2012-05-03 12:52:13 PDT
Created attachment 620820 [details] [diff] [review]
patch, don't consider height/width the same if they're "auto"

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 104 David Baron :dbaron: ⌚️UTC-10 2012-05-03 13:31:11 PDT
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.
Comment 105 :Ehsan Akhgari 2012-06-04 13:43:20 PDT
Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=b2c2a0f90b5c
Comment 106 :Ehsan Akhgari 2012-06-05 07:51:11 PDT
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.
Comment 107 :Ehsan Akhgari 2012-06-05 09:03:48 PDT
(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.
Comment 108 :Ehsan Akhgari 2012-06-05 11:41:27 PDT
Created attachment 630241 [details] [diff] [review]
Make sure to reflow when {min,max}-width changes

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.
Comment 110 :Ehsan Akhgari 2012-06-05 11:54:29 PDT
(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 111 David Baron :dbaron: ⌚️UTC-10 2012-06-05 12:14:08 PDT
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.
Comment 112 David Baron :dbaron: ⌚️UTC-10 2012-06-05 12:15:39 PDT
(And it avoids having to duplicate the checks of the {min-,max,}{height,width} properties.)
Comment 113 :Ehsan Akhgari 2012-06-05 13:08:18 PDT
Created attachment 630296 [details] [diff] [review]
Simplify nsStylePosition::CalcDifference

Right, this makes the code a lot easier to read!
Comment 114 :Ehsan Akhgari 2012-06-05 13:11:41 PDT
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.
Comment 115 :Ehsan Akhgari 2012-06-05 13:18:41 PDT
(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 116 David Baron :dbaron: ⌚️UTC-10 2012-06-05 13:27:46 PDT
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.
Comment 117 :Ehsan Akhgari 2012-06-05 13:56:11 PDT
(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.
Comment 119 David Baron :dbaron: ⌚️UTC-10 2012-06-05 22:50:29 PDT
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.
Comment 120 :Ehsan Akhgari 2012-06-06 06:21:26 PDT
(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.
Comment 122 Ryan VanderMeulen [:RyanVM] 2012-08-27 17:49:09 PDT
This was backed out from mozilla-beta due to bug 775350.
https://hg.mozilla.org/releases/mozilla-beta/rev/3d7907e9e5a9

Note You need to log in before you can comment on or make changes to this bug.