Last Comment Bug 665597 - Include margin calculations in FinishAndStoreOverflow
: Include margin calculations in FinishAndStoreOverflow
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-needed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 749386 439567 724352 724789 733711 748518 749935 749940 750293 751278
Blocks: 372498 524925
  Show dependency treegraph
 
Reported: 2011-06-20 10:02 PDT by Benjamin Stover (:stechz)
Modified: 2014-01-24 18:54 PST (History)
16 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Include margin calculations in FinishAndStoreOverflow (7.75 KB, patch)
2011-06-20 10:05 PDT, Benjamin Stover (:stechz)
dbaron: feedback+
roc: feedback+
Details | Diff | Splinter Review
Include margin calculations in FinishAndStoreOverflow (4.82 KB, patch)
2011-08-19 09:16 PDT, Chris Lord [:cwiiis]
roc: feedback+
Details | Diff | Splinter Review
Include margin calculations in FinishAndStoreOverflow (replaced FIXME with XXX) (4.81 KB, patch)
2011-08-25 03:23 PDT, Chris Lord [:cwiiis]
dbaron: review+
chrislord.net: feedback+
Details | Diff | Splinter Review
a test of padding and overflow interaction (798 bytes, text/html; charset=UTF-8)
2011-10-06 12:31 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Fix padding on scrollable frames (2.17 KB, patch)
2011-10-10 10:52 PDT, Chris Lord [:cwiiis]
dbaron: review+
Details | Diff | Splinter Review
Fix test_bug375003-2 (1.63 KB, patch)
2011-10-10 10:56 PDT, Chris Lord [:cwiiis]
dbaron: review+
Details | Diff | Splinter Review
Fix some reftests assumptions about margins and scrollable overflow (3.71 KB, patch)
2011-10-10 14:27 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix some reftests assumptions about margins and scrollable overflow (6.11 KB, patch)
2011-10-24 07:33 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix some reftests assumptions about margins and scrollable overflow, v2 (8.27 KB, patch)
2011-12-14 17:24 PST, Mats Palmgren (:mats)
dbaron: review+
Details | Diff | Splinter Review
prerequisite 1. Add nsRect::SaturatingUnion(Rect)(Edges) (2.46 KB, patch)
2012-01-03 11:59 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
prerequisite 2. Don't check overflow rects include border-box if the overflow rects are saturated. (1.71 KB, patch)
2012-01-03 12:04 PST, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
part 1, Include margin calculations in FinishAndStoreOverflow (3.38 KB, patch)
2012-01-03 12:44 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
Testcase, child overflows padding (490 bytes, text/html)
2012-01-03 12:46 PST, Mats Palmgren (:mats)
no flags Details
part 2, use ApplySkipSides on the margin (1.13 KB, patch)
2012-01-03 12:50 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 3, don't include margins in scrollable overflow for popup frames (1.70 KB, patch)
2012-01-03 12:55 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 4, use saturating calculations when adding the margin to the scrollable overflow rect (2.89 KB, patch)
2012-01-03 12:58 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 5, fix tests (23.17 KB, patch)
2012-01-03 13:06 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 3, don't include margins in scrollable overflow for popup frames (3.50 KB, patch)
2012-01-04 11:57 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
Assertion details for 265867-1.html (4.52 KB, text/plain)
2012-01-13 15:55 PST, Mats Palmgren (:mats)
no flags Details
Assertion details for 323386-1.html (4.16 KB, text/plain)
2012-01-13 15:57 PST, Mats Palmgren (:mats)
no flags Details
Assertion details for 436822-1.html (5.56 KB, text/plain)
2012-01-13 15:58 PST, Mats Palmgren (:mats)
no flags Details
Assertion details / frame dumps for 515811-1.html (17.48 KB, text/html)
2012-01-13 16:00 PST, Mats Palmgren (:mats)
no flags Details
Assertion details for 570289-1.html (6.57 KB, text/plain)
2012-01-13 16:02 PST, Mats Palmgren (:mats)
no flags Details

Description Benjamin Stover (:stechz) 2011-06-20 10:02:07 PDT
Currently both scrollable and visual overflow include margin calculations as a special case in nsBlockFrame.cpp. Instead, we can generalize margin into only scrollable overflow by adding (non-collapsed) margins into FinishAndStoreOverflow's calculations.
Comment 1 Benjamin Stover (:stechz) 2011-06-20 10:05:55 PDT
Created attachment 540514 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow
Comment 2 Benjamin Stover (:stechz) 2011-06-20 10:11:50 PDT
Comment on attachment 540514 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow

So, IIUC this is the suggestion made in bug 524925. The problem with this strategy is that it can overshoot the actual scrollable area. Unlike visual overflow, this affects layout and I don't know if that is acceptable or to the CSS spec.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 15:29:48 PDT
Comment on attachment 540514 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow

Review of attachment 540514 [details] [diff] [review]:
-----------------------------------------------------------------

I like this approach.

::: layout/generic/nsFrame.cpp
@@ +6369,5 @@
> +  // Include margin in scrollable overflow.
> +  nsMargin usedMargin = GetUsedMargin();
> +  aOverflowAreas.ScrollableOverflow().x -= usedMargin.left;
> +  aOverflowAreas.ScrollableOverflow().y -= usedMargin.top;
> +  aOverflowAreas.ScrollableOverflow().width += usedMargin.right;

LeftRight(), TopBottom()
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-07-01 11:49:43 PDT
Comment on attachment 540514 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow

>+  // Include margin in scrollable overflow.
>+  nsMargin usedMargin = GetUsedMargin();
>+  aOverflowAreas.ScrollableOverflow().x -= usedMargin.left;
>+  aOverflowAreas.ScrollableOverflow().y -= usedMargin.top;
>+  aOverflowAreas.ScrollableOverflow().width += usedMargin.right;
>+  aOverflowAreas.ScrollableOverflow().height += usedMargin.bottom;

roc's comment about using LeftRight() and TopBottom() doesn't work here.  Additionally, you're inflating a rectangle that could already extend past the margin edge, so you don't want to expand it in that case.  So what you really want is:

  nsRect marginBounds(bounds);
  marginBounds.Inflate(GetUsedMargin());
  nsRect &so = aOverflowAreas.ScrollableOverflow();
  so.UnionRectEdges(so, marginBounds);

(I also have mixed feelings about whether you should do this before or after the abs-pos clip stuff... but I'm leaning towards thinking you picked the correct place.)

Also, could you add a comment:
  // FIXME: In theory this should consider margin collapsing.

However, I also noticed this bit:

  // Overflow area must always include the frame's top-left and bottom-right,
  // even if the frame rect is empty.
  // Pending a real fix for bug 426879, don't do this for inline frames
  // with zero width.
  if (aNewSize.width != 0 || !IsInlineFrame(this)) {
    NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
      nsRect& o = aOverflowAreas.Overflow(otype);
      o.UnionRectEdges(o, bounds);
    }
  }

so I think what you really want to do is put your new code *in* that condition, because otherwise it would essentially undo that check.  Moving it up there doesn't actually change anything relative to where you currently have the code.  But you should *leave* the existing loop because we also want to include the border box in scrollable overflow in case the margins are negative (and there should be a comment saying that).

I think, with that, we're in good shape... though I hope the collapsing margins thing isn't too noticeable.

Sorry for taking so long to get to this.
Comment 5 Chris Lord [:cwiiis] 2011-08-19 09:16:05 PDT
Created attachment 554444 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow

Here's the patch with the advice from comment #4 taken into account. I don't really understand what the effects of doing this are, so if I've misinterpreted anything, any feedback/elaboration is much appreciated.
Comment 6 Chris Lord [:cwiiis] 2011-08-24 03:00:36 PDT
Is this patch landable with the included FIXME, or do we need to sort that out beforehand? If so, any pointers on how to do that?
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-24 20:38:23 PDT
It needs actual review, preferably from dbaron. I don't think we need to do the FIXME (it probably should be XXX or nothing).
Comment 8 Chris Lord [:cwiiis] 2011-08-25 03:23:36 PDT
Created attachment 555686 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow (replaced FIXME with XXX)

Carrying over roc's feedback+, adding r? for :dbaron.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-09 11:51:15 PDT
Comment on attachment 555686 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow (replaced FIXME with XXX)

># HG changeset patch
># User Benjamin Stover <bstover@mozilla.com>

Is Ben the only author, or should you be listed as well?

># Date 1308589389 25200
># Node ID cf1b903389ced356e4b56ccd984ebddad4273fd8
># Parent c40a265eb1700d041331b9265ebef8bd3e0aceb7
>Bug 665597 Include margin calculations in FinishAndStoreOverflow

This isn't a good commit message; it's way too specific to code and doesn't say what behavior is being changed.

Instead, I suggest:

Include margins of all frames in scrollable overflow areas (replacing a special case for block margins only, which included block bottom margins in both scrollable and visual overflow areas).  (Bug 665597)  r=dbaron


For future patches, please add the [diff] settings to your ~/.hgrc as described in:
https://developer.mozilla.org/en/Installing_Mercurial#Configuration
(in particular, you do not currently have unified=8 or showfunc=1, but you should).

>-    // here. If we're a scrolled block then we also need to account
>-    // for the scrollframe's padding, which is logically below the
>-    // bottom margins of the children.

Nothing seems to replace this code.  Have you investigated whether this is actually taken care of elsewhere, or do I need to do that?

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>+
>+    // Include margin in scrollable overflow.
>+    // XXX In theory this should consider margin collapsing

I prefer "FIXME" to "XXX"; not sure why you changed it.

Otherwise I think this looks good, still holding on the "Nothing seems to replace..."
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-09 12:56:04 PDT
Comment on attachment 555686 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow (replaced FIXME with XXX)

I added a test for the case I was worried about in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29cdde4d16f3

If that test passes, then r=dbaron with the comments above.

Otherwise, we need to figure out what to do about it.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-09-09 12:56:44 PDT
Though really this should also have some additional tests...
Comment 12 Justin Wood (:Callek) 2011-09-09 22:43:49 PDT
(In reply to David Baron [:dbaron] from comment #10)
> I added a test for the case I was worried about in:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/29cdde4d16f3

And that is not on m-c.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-10 06:32:34 PDT
The main patch here has not landed.
Comment 14 Chris Lord [:cwiiis] 2011-09-14 15:42:49 PDT
I've pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7c07f6e730ad

Fingers crossed!
Comment 15 Chris Lord [:cwiiis] 2011-10-03 17:28:36 PDT
After a different try (which I checked was green beforehand), it seems this change breaks this - https://bugzilla.mozilla.org/show_bug.cgi?id=295977

Quote:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | g should have scrolled vertically
Stack trace:
    JS frame :: chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js :: <TOP_LEVEL> :: line 44

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | g should have scrolled horizontally
Stack trace:
    JS frame :: chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js :: <TOP_LEVEL> :: line 48

I don't really know enough about layout to understand what this part of the test is testing, so I don't really have any ideas as to why it's broken - Any suggestions?
Comment 16 Chris Lord [:cwiiis] 2011-10-03 18:16:15 PDT
I also spoke prematurely, it seems some related ref-tests fail too. The build in question: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=74bc388aa07b
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-03 18:18:22 PDT
Div g is:
<div id="g" style="width: 99px; height: 99px; padding: 10px; border: 10px solid black; margin: 10px; overflow: auto;"><div style="width: 100px; height: 100px;"></div></div>
The test expects it to be scrollable horizontally and vertically (which makes sense), but it isn't.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-06 12:31:30 PDT
Created attachment 565313 [details]
a test of padding and overflow interaction

This tests how padding and overflow interact, in order to try to figure out what to do with the removed scrolledContent case in nsBlockFrame::ComputeOverflowAreas.

In Gecko and WebKit, the padding creates extra scrollable space at the bottom on the first test (where the overflow is in-flow) but not the second (where the overflow is out-of-flow).

In Opera, the padding doesn't create extra scrollable space for either.  I think Opera matches the spec.

I think it would also be a sensible behavior -- probably better -- to create extra scrollable space on all sides of the children's overflow.  (This would mean creating a similar scrolledContent case in FinishAndStoreOverflow -- easy enough to do.)  But I'm not sure how Web-compatible that is (the main risk being potentially causing scrollbars where there weren't any before).  I'm not sure if it's worth trying to get a spec change.


I'm already somewhat concerned about the Web-compatibility of *this* change.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-06 13:11:53 PDT
(In reply to David Baron [:dbaron] from comment #18)
> Created attachment 565313 [details]
> a test of padding and overflow interaction
> 
> This tests how padding and overflow interact, in order to try to figure out
> what to do with the removed scrolledContent case in
> nsBlockFrame::ComputeOverflowAreas.
> 
> In Gecko and WebKit, the padding creates extra scrollable space at the
> bottom on the first test (where the overflow is in-flow) but not the second
> (where the overflow is out-of-flow).
> 
> In Opera, the padding doesn't create extra scrollable space for either.  I
> think Opera matches the spec.

It looks like IE8 matches Opera.  I don't have access to a newer IE right now.
Comment 20 Chris Lord [:cwiiis] 2011-10-10 10:52:36 PDT
Created attachment 565975 [details] [diff] [review]
Fix padding on scrollable frames

Including margin in scrollable overflow breaks the handling of padding on scrollable containers (such as in this test: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/458296-1a.html)

After discussing with dbaron on IRC, this is the solution we came up with. This in turn breaks a test and some ref-tests, which I'm looking at now.
Comment 21 Chris Lord [:cwiiis] 2011-10-10 10:56:47 PDT
Created attachment 565978 [details] [diff] [review]
Fix test_bug375003-2

test_bug375003-2 did not expect padding to be taken into account in scrollable overflow. This patch changes the expected values so that it passes again.
Comment 22 Chris Lord [:cwiiis] 2011-10-10 14:27:10 PDT
Created attachment 566030 [details] [diff] [review]
Fix some reftests assumptions about margins and scrollable overflow

This patch fixes some reftests assumptions about margins not extending scrollable overflow.

After this patch (assuming this patch is correct, which I've not finished testing yet), there are still failures in the inline-borderpadding tests. It seems the margin incorrectly adds to the right instead of pushing to the left for rtl frames.

Feedback appreciated, I'll see if I can fix that (but advice will only help :))
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-18 19:33:12 PDT
Comment on attachment 565975 [details] [diff] [review]
Fix padding on scrollable frames

>+  // If we have a scrolledContent pseudo, add the padding to the scrollable
>+  // overflow to ensure it remains accessible via scrolling.

I think this needs a little further explanation, perhaps:

  This adds the padding around the edges of the overflow that we initialized in the similar special case in ComputeOverflowAreas without the padding.  So it means that we're adding the padding *around* the edges of the overflow, rather than only around the edges of the content.

and probably also:

  Note that the border should always be zero, but we subtract/add border and padding in both places to be consistent.

r=dbaron with those additional comments, though I'd have preferred to see a commit message as part of the review
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-18 19:37:23 PDT
Comment on attachment 565978 [details] [diff] [review]
Fix test_bug375003-2

r=dbaron
Comment 25 Chris Lord [:cwiiis] 2011-10-24 07:33:15 PDT
Created attachment 569061 [details] [diff] [review]
Fix some reftests assumptions about margins and scrollable overflow

Tests now all pass with this patch - this alters some of the reftests so that the reference mark-up matches the tests with respect to margins.

I don't think I've changed any of the tests in such a way that they don't still test what they were originally testing, and I guess they now double as tests of margin behaviour with scrollable overflow.
Comment 26 Chris Lord [:cwiiis] 2011-10-24 07:36:12 PDT
(In reply to David Baron [:dbaron] from comment #23)
> r=dbaron with those additional comments, though I'd have preferred to see a
> commit message as part of the review

I was thinking that the fix-padding-with-scrollable-frames patch would be squashed with the include-margin-calculations-with-scrollable-overflow patch, as I'm not sure if it makes much sense on its own (other than to fix a bug introduced in the former patch).

Do you think they should be separate?
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-28 11:00:16 PDT
(In reply to Chris Lord [:cwiiis] from comment #26)
> I was thinking that the fix-padding-with-scrollable-frames patch would be
> squashed with the include-margin-calculations-with-scrollable-overflow
> patch, as I'm not sure if it makes much sense on its own (other than to fix
> a bug introduced in the former patch).
> 
> Do you think they should be separate?

Squashing them makes sense.



Is your patch queue published somewhere (e.g., on hg.mozilla.org/users/ ?)
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-10-28 12:03:49 PDT
Comment on attachment 569061 [details] [diff] [review]
Fix some reftests assumptions about margins and scrollable overflow

layout/reftests/box-properties/abspos-non-replaced-width-offset-margin-ref.html
layout/reftests/box-properties/abspos-replaced-width-offset-margin-ref.html

  These changes seem fishy to me, but I don't understand.  Could you
  explain?

  (It might be easier if I could play with the testcase in a build with
  the patches; that's why I asked about your patch queue.)

layout/reftests/bugs/403519-2-ref.html
layout/reftests/bugs/403519-2.html

  Looks good.

layout/reftests/bugs/458296-1-ref.html
layout/reftests/bugs/458296-1c.html
layout/reftests/bugs/458296-1d.html

  Looks good.

layout/reftests/forms/progress/bar-pseudo-element-vertical.html

  Instead, I'd suggest putting a <br> before the last test (and changing the
  selector from :nth-child to :nth-of-type) -- for both the ltr and rtl
  variants.
Comment 29 Mats Palmgren (:mats) 2011-12-14 17:24:23 PST
Created attachment 581846 [details] [diff] [review]
Fix some reftests assumptions about margins and scrollable overflow, v2

(In reply to David Baron [:dbaron] from comment #28)
> layout/reftests/box-properties/abspos-non-replaced-width-offset-margin-ref.
> html
> layout/reftests/box-properties/abspos-replaced-width-offset-margin-ref.html
> 
>   These changes seem fishy to me, but I don't understand.  Could you
>   explain?

There's no difference in the rendering of the boxes, other than the
content doesn't fit the width of the reftest window, triggering a
scrollbar for the viewport.  The difference is in the scrollbar.
(See also the testcases in bug 372498, which the patches here fix.)

>-html, body { margin: 0; padding: 0; border: none; }
>+html { margin: 0 3px 0 0; padding: 0 4px 0 0; border: none; }
>+body { margin: 0 6px 0 0; padding: 0 7px 0 0; border: none; }

These changes are unnecessary and I think it's beneficial for
the test to NOT use the same values in the -ref as in the test,
so I reverted that.

>-div { height: 1px; background: navy; }
>+div { height: 1px; background: navy; margin-right: 19px; }

This is needed to account for the additional scrollable overflow.

> layout/reftests/forms/progress/bar-pseudo-element-vertical.html
> 
>   Instead, I'd suggest putting a <br> before the last test (and changing the
>   selector from :nth-child to :nth-of-type) -- for both the ltr and rtl
>   variants.

Updated as suggested.

There's still plenty of other failed tests though (with or
without bug 524925) so I'm wondering if there's more patches
I'm missing?
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-22 08:58:50 PST
(In reply to Mats Palmgren [:mats] from comment #29)
> There's still plenty of other failed tests though (with or
> without bug 524925) so I'm wondering if there's more patches
> I'm missing?

Which tests are failing?
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-22 09:04:46 PST
Comment on attachment 581846 [details] [diff] [review]
Fix some reftests assumptions about margins and scrollable overflow, v2


>diff --git a/layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html b/layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html
>--- a/layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html
>+++ b/layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html

Don't you also need to change the reference for this test?

>diff --git a/layout/reftests/forms/progress/bar-pseudo-element-vertical.html b/layout/reftests/forms/progress/bar-pseudo-element-vertical.html
>--- a/layout/reftests/forms/progress/bar-pseudo-element-vertical.html
>+++ b/layout/reftests/forms/progress/bar-pseudo-element-vertical.html

Likewise here.

>@@ -16,17 +16,17 @@
>     body > progress:nth-child(8)::-moz-progress-bar { margin: 0px; padding: 10px 0px 0px 0px; }
>     body > progress:nth-child(9)::-moz-progress-bar { margin: 0px; padding: 0px 10px 0px 0px; }
>     body > progress:nth-child(10)::-moz-progress-bar { margin: 0px; padding: 0px 0px 10px 0px; }
>     body > progress:nth-child(11)::-moz-progress-bar { margin: 0px; padding: 0px 0px 0px 10px; }
>     body > progress:nth-child(12)::-moz-progress-bar { height: 1000px; }
>     body > progress:nth-child(13)::-moz-progress-bar { height: 10px; }
>     body > progress:nth-child(14)::-moz-progress-bar { height: 10%; }
>     body > progress:nth-child(15)::-moz-progress-bar { height: 200%; }
>-    body > progress:nth-child(16)::-moz-progress-bar { margin: 64px; padding: 64px; }
>+    body > progress:nth-of-type(16)::-moz-progress-bar { margin: 64px 64px; padding: 64px; }

And also keep this "64px" rather than "64px 64px", though it doesn't really make a difference.


r=dbaron with that
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-30 16:11:28 PST
I pushed the patches here to try:
https://tbpl.mozilla.org/?tree=Try&rev=459a6d2455e6
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 03:52:15 PST
There are many new occurrences of
###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || aOverflowAreas.Overflow(otype).Contains(nsRect(nsPoint(0,0), aNewSize))', file e:/builds/moz2_slave/try-w32-dbg/build/layout/generic/nsFrame.cpp, line 6575

I debugged layout/base/crashtests/149014-1.html, it looks like an nscoord overflow problem due to maxed-out layout coordinates. Not sure why this doesn't fail already. I'm not sure what to do about it.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 14:42:50 PST
(In reply to David Baron [:dbaron] from comment #31)
> Don't you also need to change the reference for this test?
> Likewise here.

That was the cause of a couple of the failures. I've fixed it in my tree.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 14:47:37 PST
+  if (GetStyleContext()->GetPseudo() == nsCSSAnonBoxes::scrolledContent) {
+    // For scrolledContent, initialise the scrollable overflow with the
+    // content-box instead of the border-box. This will be inflated later
+    // with the padding. This is to ensure that padding remains
+    // accessible via scrolling.
+    bounds.Deflate(aReflowState.mComputedBorderPadding);
+    areas.ScrollableOverflow() = bounds;

This causes a problem. After Deflate, the rect may have zero height but nonzero width (or vice versa). Then calling Union on 'areas' will destroy the width information in the rect. This would be fixed by using UnionRectEdges for scrollable overflow but we don't want to do that here if possible...
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 15:02:26 PST
Pushed some fixes to try:
https://tbpl.mozilla.org/?tree=Try&rev=b1b2d0897bfc
Comment 37 Mats Palmgren (:mats) 2012-01-02 15:09:08 PST
Are you also working on this?
I think I have a series of patches that works now, except for some remaining
assertions that stems from calculations with nscoord_MIN/MAX by the Inflate.
https://tbpl.mozilla.org/?tree=Try&rev=ee8eeeb40e35

Maybe I should attach what I have for review now? and leave the fix for the
assertions after I've gone through them.
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 15:47:34 PST
(In reply to Mats Palmgren [:mats] from comment #37)
> Are you also working on this?

Yes.
Comment 39 Mats Palmgren (:mats) 2012-01-02 15:54:46 PST
Ok, I'm looking at the patches in your try push and it seems to be dealing
with the nscoord overflow assertions, right?  so, that's the bit I haven't
done yet, should I just integrate those with the fixes I have in my queue?
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 16:00:46 PST
I'll leave this up to you I guess, but you might want to take the first three patches from https://tbpl.mozilla.org/?tree=Try&rev=b1b2d0897bfc.

The fourth one there I'm less sure about. It addresses comment #35 by choosing to keep the current overflow area area rect when combining two empty rects, but that's not always the right thing to do (e.g. if the current rect is 0,0,0,0 we should probably take the new rect). I suspect that's the reason for the new test failures on my try push. The "best" thing to do would be to make nsOverflowAreas use SaturingUnionRectEdges for the scrollable areas. It would be better to not have to bite that off here, but maybe we have to.
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 16:13:55 PST
Just for fun I pushed a patch to do the SaturatingUnionRectEdges thing for scrollable overflow --- on top of the Saturing changes, but without the other patches for this bug:
https://tbpl.mozilla.org/?tree=Try&rev=783bfbb63d19
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-02 19:18:33 PST
The results aren't too bad. Just some assertions in crashtests (and some assertions fixed, too). No reftest failures. I'll post those changes to bug 439567.
Comment 43 Mats Palmgren (:mats) 2012-01-03 11:59:10 PST
Created attachment 585500 [details] [diff] [review]
prerequisite 1. Add nsRect::SaturatingUnion(Rect)(Edges)
Comment 44 Mats Palmgren (:mats) 2012-01-03 12:04:05 PST
Created attachment 585503 [details] [diff] [review]
prerequisite 2. Don't check overflow rects include border-box if the overflow rects are saturated.

roc, I decided to use these two of your patches (part 1 and 3 in your set
I believe) but not the other two.  I'm just attaching them here two
document what I was using at the time - you may have newer versions
elsewhere(?)  Let me know if you want me to push these as part of this
bug or if you'll handle it in a different bug.
Comment 45 Mats Palmgren (:mats) 2012-01-03 12:44:14 PST
Created attachment 585513 [details] [diff] [review]
part 1, Include margin calculations in FinishAndStoreOverflow

This is slightly different from the last version since I decided
to not do the proposed changes for handling of padding inside
the scroll frame.  This instead makes us handle padding the same
as IE10 which I believe is the correct rendering per CSS.
(the margin changes are the same as before though)
Comment 46 Mats Palmgren (:mats) 2012-01-03 12:46:39 PST
Created attachment 585514 [details]
Testcase, child overflows padding

Here's a testcase to highlight the change for padding.
The two blocks should render exactly the same, as the bottom block.
Comment 47 Mats Palmgren (:mats) 2012-01-03 12:50:07 PST
Created attachment 585519 [details] [diff] [review]
part 2, use ApplySkipSides on the margin

This fixed most of the test failures...
Comment 48 Mats Palmgren (:mats) 2012-01-03 12:55:53 PST
Created attachment 585524 [details] [diff] [review]
part 3, don't include margins in scrollable overflow for popup frames

This fixed toolkit/content/tests/chrome/test_tooltip.xul
It might be possible to fix it also by changing 
nsLayoutUtils::GetPopupFrameForEventCoordinates to not use
scrollable overflow rect, but it might be difficult to separate the margin
contribution to the overflow rect from the child overflow at that point.
But I don't see why margins on tooltips needs to be scrollable really.
Comment 49 Mats Palmgren (:mats) 2012-01-03 12:58:17 PST
Created attachment 585525 [details] [diff] [review]
part 4, use saturating calculations when adding the margin to the scrollable overflow rect

This is solely to fix assertions that stem from overflowed nscoord calculations.
Comment 50 Mats Palmgren (:mats) 2012-01-03 13:06:47 PST
Created attachment 585529 [details] [diff] [review]
part 5, fix tests

(In reply to David Baron [:dbaron] from comment #31)
>>+++ b/layout/reftests/forms/progress/bar-pseudo-element-vertical-rtl.html
>Don't you also need to change the reference for this test?

Yes (I did), I'm not sure why they were missing in the patch I attached...
Fixed the other nits from comment 31 too.

I also bumped up the assertion counts for a number of tests that used
huge margin values to fix oranges.  I suppose your patches will help
fixing some of those, maybe.
Comment 51 Mats Palmgren (:mats) 2012-01-03 13:09:09 PST
Ok, that's all the patches for this bug.  I'm also attaching updated patches
in bug 524925.  The whole patch series is green on Try.
Comment 52 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 13:17:13 PST
(In reply to Mats Palmgren [:mats] from comment #46)
> Here's a testcase to highlight the change for padding.
> The two blocks should render exactly the same, as the bottom block.

Why do you think so?

And this also means we don't get right padding included in the scrollable overflow either, right?
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 13:18:30 PST
Comment on attachment 585519 [details] [diff] [review]
part 2, use ApplySkipSides on the margin

Review of attachment 585519 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +6605,5 @@
>      // XXX In theory this should consider margin collapsing
>      nsRect marginBounds(bounds);
> +    nsMargin margin = GetUsedMargin();
> +    ApplySkipSides(margin);
> +    marginBounds.Inflate(margin);

Seems to me that GetUsedMargin should take ApplySkipSides into account already. If it doesn't, we should fix that in GetUsedMargin or where we save the used margin.
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 13:20:37 PST
Comment on attachment 585524 [details] [diff] [review]
part 3, don't include margins in scrollable overflow for popup frames

Review of attachment 585524 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +6614,5 @@
> +      ApplySkipSides(margin);
> +      marginBounds.Inflate(margin);
> +      nsRect& so = aOverflowAreas.ScrollableOverflow();
> +      so.UnionRectEdges(so, marginBounds);
> +    }

Can we instead have popups return 0 for GetUsedMargin? That seems slightly cleaner and more efficient to me.
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 13:29:53 PST
Comment on attachment 585529 [details] [diff] [review]
part 5, fix tests

Review of attachment 585529 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not 100% comfortable adding asserts for the tests that don't currently have any (the 11 in layout/generic/crashtests).

I ran into the problem that in some tests we're hitting nscoord overflows that don't currently trigger any assertions, but the new code makes the overflows detectable. If that's what's happening here, it might be better to start with a patch that adds a new assertion that detects the overflows and adjust the assertion counts to account for that new assertion. Then with the rest of the patches we shouldn't need to add assertions for tests that don't have any. Make sense?

David has argued recently that nscoord overflows should be warnings instead of assertions. On one hand, they should be assertions because they're bugs in our code. On the other hand, we have no plan to fix them so when you do something good and useful (like this patch) and hit those assertions, you're a bit stuck :-(. So I do think adjusting assertion counts on patches that hit these assertions is the best thing to do for now.
Comment 56 Mats Palmgren (:mats) 2012-01-03 13:33:44 PST
> Can we instead have popups return 0 for GetUsedMargin?

I'll investigate, but I suspect that the margin is what is used to displace
a tooltip below the hover point.
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/popup.css#114
Comment 57 Mats Palmgren (:mats) 2012-01-03 14:00:28 PST
> but the new code makes the overflows detectable

Yes, that is what is happening here, we're exposing existing nscoord calculations
that should've have used saturating methods rather than +/-/Inflate/Deflate etc...
We have *a lot* of those and I think it's futile to try to fix them all without
changing nscoord to float, or making nscoord a class with overloaded operators
or something systematic like that.  I really don't think adding Saturating* calls
is a good solution...

> David has argued recently that nscoord overflows should be warnings instead

Yes please, I'm strongly in favor of that!  I think we should go one step
further and mute them completely behind a #ifdef NOISY_NSCOORD_OVERFLOW
or some such.  Warnings like that does more harm than good.  I see the
motivation for detecting the occasional bug with math on nscoord_MAX but
that should be rare, at least in a way that is undetectable by our reftests.
People who are interested can simply switch them on in their debug builds
if they think it will benefit what they are working on, it shouldn't be a
burden on everyone all time.
Comment 58 Mats Palmgren (:mats) 2012-01-03 17:39:01 PST
> Can we instead have popups return 0 for GetUsedMargin?

The 'Moth' test failures on Linux indicates this will not work:
https://tbpl.mozilla.org/?tree=Try&rev=8854f9308594
for "test_arrowpanel.xul" the diff between actual/expected is 23px
and for "test_tooltip.xul" it's 21px which equals the margins for these elements:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/popup.css
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 17:48:35 PST
(In reply to Mats Palmgren [:mats] from comment #56)
> > Can we instead have popups return 0 for GetUsedMargin?
> 
> I'll investigate, but I suspect that the margin is what is used to displace
> a tooltip below the hover point.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/
> global/popup.css#114

OK. Then lets go with your original patch. But, can you add to nsLayoutUtils::IsPopup an early exit if !NS_FRAME_HAS_VIEW? That should keep this efficient.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 17:51:52 PST
(In reply to Mats Palmgren [:mats] from comment #57)
> Yes please, I'm strongly in favor of that!  I think we should go one step
> further and mute them completely behind a #ifdef NOISY_NSCOORD_OVERFLOW
> or some such.  Warnings like that does more harm than good.  I see the
> motivation for detecting the occasional bug with math on nscoord_MAX but
> that should be rare, at least in a way that is undetectable by our reftests.
> People who are interested can simply switch them on in their debug builds
> if they think it will benefit what they are working on, it shouldn't be a
> burden on everyone all time.

OK. I think we should probably introduce a new assertion macro just for this kind of check rather than sprinkling #ifdefs all over the place.

For this bug, I think we should do what I suggested in comment #55.
Comment 61 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-03 18:06:30 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> (In reply to Mats Palmgren [:mats] from comment #46)
> > Here's a testcase to highlight the change for padding.
> > The two blocks should render exactly the same, as the bottom block.
> 
> Why do you think so?

I think I understand now. 'overflow' shouldn't affect the location of the element's padding.

IE9 and Opera also agree with that. Chrome behaves like we currently do, but (like us) only for bottom padding, not right padding, which is clearly bogus. So I'm happy now :-). Please report a Webkit bug though if there isn't one already filed. We should add a reftest for the new behavior.
Comment 62 Mats Palmgren (:mats) 2012-01-04 11:57:44 PST
Created attachment 585839 [details] [diff] [review]
part 3, don't include margins in scrollable overflow for popup frames

Added the suggested IsPopup optimization.
Comment 63 Mats Palmgren (:mats) 2012-01-04 12:12:02 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> For this bug, I think we should do what I suggested in comment #55.

Sorry, I think there's a misunderstanding.  The patch that adds the margin to the
scrollable overflow rect is the *cause* of the increased number of assertions.
So it's not possible to do what you suggest in comment #55 since there's nothing
to detect (or I'm not understanding you).

The reason for the added assertion counts is that these tests have huge margins
causing them to trip more of these assertions than before.
Comment 64 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-04 13:29:58 PST
Comment on attachment 585519 [details] [diff] [review]
part 2, use ApplySkipSides on the margin

Review of attachment 585519 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to address this comment :-)
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-04 13:32:59 PST
(In reply to Mats Palmgren [:mats] from comment #63)
> Sorry, I think there's a misunderstanding.  The patch that adds the margin
> to the
> scrollable overflow rect is the *cause* of the increased number of
> assertions.
> So it's not possible to do what you suggest in comment #55 since there's
> nothing
> to detect (or I'm not understanding you).

What if you added content, such as an extra character, at the bottom of the test document? Then the huge margins would lead to an overflow in the height of the body element and we'd get some assertions on trunk, right? Then adding including the margin in the scrollable overflow rect either wouldn't add more assertions or would only add assertions where we already have some assertions.
Comment 66 Mats Palmgren (:mats) 2012-01-04 14:27:58 PST
> Then the huge margins would lead to an overflow in the height of the body element
> and we'd get some assertions on trunk, right?

I think a nscoord_MAX frame rect height is different from having a reasonable height
with a nscoord_MAX height in the scrollable overflow rect, so I would expect those
to take slightly different code paths.  But, I'll investigate a few of these tests
as you say and see what happens.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> Seems to me that GetUsedMargin should take ApplySkipSides into account
> already. If it doesn't, we should fix that in GetUsedMargin or where we save
> the used margin.

It doesn't and I agree it should.  I think it's a separate issue though
and not something we need to fix in this patch set.  I'll file a bug
if there isn't one already.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-04 15:10:05 PST
(In reply to Mats Palmgren [:mats] from comment #66)
> I think it's a separate issue though and not something we need to fix in this
> patch set.

It's not completely separate since it would reduce the overhead of the code you're adding here. So I think we should do it, but I'm OK with not doing it in this bug, as long as it gets done.
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-04 15:10:22 PST
Comment on attachment 585519 [details] [diff] [review]
part 2, use ApplySkipSides on the margin

Review of attachment 585519 [details] [diff] [review]:
-----------------------------------------------------------------

Please file a followup on fixing GetUsedMargin.
Comment 69 Mats Palmgren (:mats) 2012-01-13 15:55:52 PST
Created attachment 588551 [details]
Assertion details for 265867-1.html
Comment 70 Mats Palmgren (:mats) 2012-01-13 15:57:31 PST
Created attachment 588552 [details]
Assertion details for 323386-1.html
Comment 71 Mats Palmgren (:mats) 2012-01-13 15:58:36 PST
Created attachment 588553 [details]
Assertion details for 436822-1.html
Comment 72 Mats Palmgren (:mats) 2012-01-13 16:00:25 PST
Created attachment 588554 [details]
Assertion details / frame dumps for 515811-1.html
Comment 73 Mats Palmgren (:mats) 2012-01-13 16:02:49 PST
Created attachment 588556 [details]
Assertion details for 570289-1.html
Comment 74 Mats Palmgren (:mats) 2012-01-13 16:04:04 PST
>layout/generic/crashtests/crashtests.list
-asserts(9) load 265867-1.html # Bug 575011
+asserts(13) load 265867-1.html # Bug 575011
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/265867-1.html?force=1
  ASSERTION: illegal width for combined area: 'aOverflowAreas.Overflow(otype).width >= 0', layout/generic/nsLineBox.cpp:498
  nscoord_MAX margin that now propagates into line overflow calc which doesn't use saturating methods
  (see attachment #588551 [details])

-asserts(2) asserts-if(gtk2Widget,8) load 323386-1.html # Bug 575011
+asserts(2) asserts-if(gtk2Widget,25) load 323386-1.html # Bug 575011
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/323386-1.html?force=1
    new assertions:
      ASSERTION: Scrolled rect smaller than scrollport?  layout/generic/nsGfxScrollFrame.cpp:3540
      ASSERTION: No integers in range; 0 is supposed to be in range: 'low <= high' layout/generic/nsGfxScrollFrame.cpp:1797
      ASSERTION: non-root reflow roots must not have scrollable overflow: 'target == rootFrame || desiredSize.ScrollableOverflow().IsEqualEdges(boundsRelativeToTarget)' layout/base/nsPresShell.cpp:7341
  (see attachment #588552 [details])

+asserts(1-2) load 436822-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/436822-1.html?force=1
  ASSERTION: Computed overflow area must contain frame bounds ...
  The testcase has "font-size: 1600000px;" which makes the em-size margins == nscoord_MAX
  (see attachment #588553 [details])

+asserts(11-12) load 467493-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/467493-1.html?force=1
  same as for 323386-1.html

+asserts(10-11) load 467493-2.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/467493-2.html?force=1
  same as for 323386-1.html

+asserts(4) load 478170-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/478170-1.html?force=1
  ASSERTION: Computed overflow area must contain frame bounds...
  huge font-size, <p> has 1em top/bottom margin => nscoord_MAX, same as for 436822-1.html

+asserts(1) load 515811-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/515811-1.html?force=1
  ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.: 'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE' layout/generic/nsBlockFrame.cpp:1422
  Huge font-size that affects the 1em margin.  Since we now include
  the margin in the scrollable overflow it affects column layout here:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#624
  we used to have:
    nsColumnSetFrame: child->GetScrollableOverflowRect().YMost()=1139
  we now have:
    nsColumnSetFrame: child->GetScrollableOverflowRect().YMost()=1073741824
  The new layout seems correct to me (and the old wrong),
  I have included frame dumps with and without the patches:
  (see attachment #588554 [details])

+asserts(4) load 541714-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/541714-1.html?force=1
  3 "ASSERTION: Scrolled rect smaller than scrollport?"
  1 "ASSERTION: No integers in range; 0 is supposed to be in range: 'low <= high'"
  same as for 323386-1.html

+asserts(5-6) load 541714-2.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/541714-2.html?force=1
  5 "ASSERTION: Scrolled rect smaller than scrollport?"
  1 "ASSERTION: No integers in range; 0 is supposed to be in range: 'low <= high'"
  same as for 323386-1.html

+asserts(2) load 570289-1.html
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/570289-1.html?force=1
  2 "ASSERTION: illegal width for combined area: 'aOverflowAreas.Overflow(otype).width >= 0'"
  (see attachment #588556 [details])
    
+asserts(0-1) load text-overflow-bug670564.xhtml
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/text-overflow-bug670564.xhtml?force=1
  no assertion on Linux

>layout/reftests/bugs/reftest.list
-asserts(0-1) == 582146-1.html about:blank
+asserts(0-11) == 582146-1.html about:blank
  http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/582146-1.html?force=1
  1 "ASSERTION: Shouldn't have unconstrained stuff here thanks to ComputeAutoSize: 'NS_INTRINSICSIZE != aReflowState.ComputedHeight()'"
  10 "ASSERTION: Scrolled rect smaller than scrollport?: 'result.height >= mScrollPort.height'"
  (the latter are new, I get the first one in an unpatched build)
  same as for 323386-1.html

>layout/base/crashtests/crashtests.list
-asserts(2-4) load 265999-1.html # bug 575011
+asserts(2-6) load 265999-1.html # bug 575011
  http://mxr.mozilla.org/mozilla-central/source/layout/base/crashtests/265999-1.html?force=1
  ASSERTION: Computed overflow area must contain frame bounds...
  same as for 436822-1.html
Comment 75 Mats Palmgren (:mats) 2012-01-13 16:16:52 PST
To sum up, the new assertions are all related to nscoord overflows
and I don't think they will affect any real web sites unless they
use crazy margins, in which case the layout is pretty much random
anyway.  So I think bumping up the assertion count for these tests
is the appropriate action.
Comment 76 Mats Palmgren (:mats) 2012-01-13 16:25:48 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> Please file a followup on fixing GetUsedMargin.

Filed as bug 718110.
Comment 77 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-14 03:13:18 PST
Comment on attachment 585529 [details] [diff] [review]
part 5, fix tests

Review of attachment 585529 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing that.
Comment 80 Yuan Pengfei 2012-01-29 02:15:52 PST
In Google Reader, vertical scrollbar sometimes appears in "div.entry-main", which has "overflow: auto" style. If I change "overflow: auto" to "overflow: visible" via Firebug, the scrollbar disappears.
However, the scrollbar should not appear and it did not appear before the patches.
Comment 81 Timothy Nikkel (:tnikkel) 2012-01-29 11:55:29 PST
Please file a new bug for that issue and make it block this one.
Comment 82 Jean-Yves Perrier [:teoli] 2012-04-24 14:42:45 PDT
I'm trying to document this, but I can't deduce the original problem that this fix try to solve :-( 

Is it that margins are not being taken in account when an overflow (and so the scroll area was too small)?

Does this bug solve the whole problem or are their other similar cases?

At first sight, it looks to me that a simple note into Firefox 12 for developers should be enough.

Help!
Comment 83 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-24 23:40:55 PDT
The effect of this fix is to add CSS margins to every CSS border-box when determining what can be scrolled into view. I.e., especially if an element has a right or bottom margin, you can always scroll that right or bottom margin area into view. This means overflow:auto scrollbars may appear in some situations where they didn't before.

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