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 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 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 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.

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.

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?

It needs actual review, preferably from dbaron. I don't think we need to do the FIXME (it probably should be XXX or nothing).

Carrying over roc's feedback+, adding r? for :dbaron.

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 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.

Though really this should also have some additional tests...

(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.

The main patch here has not landed.

I've pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7c07f6e730ad Fingers crossed!

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?

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

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.

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.

(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.

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.

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.

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 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 on attachment 565978 [details] [diff] [review] Fix test_bug375003-2 r=dbaron

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.

(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?

(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 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.

(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?

(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 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

I pushed the patches here to try: https://tbpl.mozilla.org/?tree=Try&rev=459a6d2455e6

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.

(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.

+ 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...

Pushed some fixes to try: https://tbpl.mozilla.org/?tree=Try&rev=b1b2d0897bfc

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.

(In reply to Mats Palmgren [:mats] from comment #37) > Are you also working on this? Yes.

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?

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.

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

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.

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.

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)

Here's a testcase to highlight the change for padding. The two blocks should render exactly the same, as the bottom block.

This fixed most of the test failures...

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.

This is solely to fix assertions that stem from overflowed nscoord calculations.

(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.

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.

(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 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 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 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.

> 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

> 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.

> 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

(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.

(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.

(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.

Added the suggested IsPopup optimization.

(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 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 :-)

(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.

> 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.

(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 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.

>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

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.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68) > Please file a followup on fixing GetUsedMargin. Filed as bug 718110.

Comment on attachment 585529 [details] [diff] [review] part 5, fix tests Review of attachment 585529 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/961442fdd159 https://hg.mozilla.org/integration/mozilla-inbound/rev/e176daacecfc https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8cf8e934a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/53a6bfdd95fb https://hg.mozilla.org/integration/mozilla-inbound/rev/804c80adda0a https://hg.mozilla.org/integration/mozilla-inbound/rev/87536f378706 https://hg.mozilla.org/integration/mozilla-inbound/rev/905ef8691d96 https://hg.mozilla.org/integration/mozilla-inbound/rev/46f81aec317f

https://hg.mozilla.org/mozilla-central/rev/961442fdd159 https://hg.mozilla.org/mozilla-central/rev/e176daacecfc https://hg.mozilla.org/mozilla-central/rev/7f8cf8e934a5 https://hg.mozilla.org/mozilla-central/rev/53a6bfdd95fb https://hg.mozilla.org/mozilla-central/rev/804c80adda0a https://hg.mozilla.org/mozilla-central/rev/87536f378706 https://hg.mozilla.org/mozilla-central/rev/905ef8691d96 https://hg.mozilla.org/mozilla-central/rev/46f81aec317f

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.

Please file a new bug for that issue and make it block this one.

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!

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.