Include margin calculations in FinishAndStoreOverflow

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: stechz, Assigned: mats)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla12
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(14 attachments, 9 obsolete attachments)

798 bytes, text/html; charset=UTF-8
Details
2.46 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.71 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
490 bytes, text/html
Details
1.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.17 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.52 KB, text/plain
Details
4.16 KB, text/plain
Details
5.56 KB, text/plain
Details
17.48 KB, text/html
Details
6.57 KB, text/plain
Details
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 540514 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow
(Reporter)

Comment 2

6 years ago
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.
Attachment #540514 - Flags: feedback?(roc)
Attachment #540514 - Flags: feedback?(dbaron)
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()
Attachment #540514 - Flags: feedback?(roc) → feedback+
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.
Attachment #540514 - Flags: feedback?(dbaron) → feedback+

Comment 5

6 years ago
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.
Attachment #540514 - Attachment is obsolete: true
Attachment #554444 - Flags: feedback?(roc)
Attachment #554444 - Flags: feedback?(dbaron)
Attachment #554444 - Flags: feedback?(roc) → feedback+

Updated

6 years ago
Blocks: 524925

Comment 6

6 years ago
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).

Comment 8

6 years ago
Created attachment 555686 [details] [diff] [review]
Include margin calculations in FinishAndStoreOverflow (replaced FIXME with XXX)

Carrying over roc's feedback+, adding r? for :dbaron.
Attachment #554444 - Attachment is obsolete: true
Attachment #555686 - Flags: review?(dbaron)
Attachment #555686 - Flags: feedback+
Attachment #554444 - Flags: feedback?(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.
Attachment #555686 - Flags: review?(dbaron) → review+
Though really this should also have some additional tests...
Flags: in-testsuite?
(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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
The main patch here has not landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
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?
Target Milestone: --- → mozilla9
Version: Trunk → Other Branch
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.
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.
(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.
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.
Assignee: nobody → chrislord.net
Attachment #565975 - Flags: review?(dbaron)
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.
Attachment #565978 - Flags: review?(dbaron)
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 :))
Attachment #566030 - Flags: feedback?(dbaron)
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
Attachment #565975 - Flags: review?(dbaron) → review+
Comment on attachment 565978 [details] [diff] [review]
Fix test_bug375003-2

r=dbaron
Attachment #565978 - Flags: review?(dbaron) → review+
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.
Attachment #566030 - Attachment is obsolete: true
Attachment #569061 - Flags: review?(dbaron)
Attachment #566030 - Flags: feedback?(dbaron)
(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.
Assignee: chrislord.net → matspal
(Assignee)

Updated

6 years ago
Blocks: 372498
(Assignee)

Comment 29

6 years ago
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?
Attachment #581846 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Attachment #569061 - Attachment is obsolete: true
Attachment #569061 - Flags: review?(dbaron)
Keywords: dev-doc-needed
(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
Attachment #581846 - Flags: review?(dbaron) → review+
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
(Assignee)

Comment 37

6 years ago
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.
(Assignee)

Comment 39

6 years ago
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.
Depends on: 439567
(Assignee)

Comment 43

6 years ago
Created attachment 585500 [details] [diff] [review]
prerequisite 1. Add nsRect::SaturatingUnion(Rect)(Edges)
(Assignee)

Comment 44

6 years ago
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.
(Assignee)

Comment 45

6 years ago
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)
Attachment #555686 - Attachment is obsolete: true
Attachment #565975 - Attachment is obsolete: true
Attachment #565978 - Attachment is obsolete: true
Attachment #585513 - Flags: review?(roc)
(Assignee)

Comment 46

6 years ago
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.
(Assignee)

Comment 47

6 years ago
Created attachment 585519 [details] [diff] [review]
part 2, use ApplySkipSides on the margin

This fixed most of the test failures...
Attachment #585519 - Flags: review?(roc)
(Assignee)

Comment 48

6 years ago
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.
Attachment #585524 - Flags: review?(roc)
(Assignee)

Comment 49

6 years ago
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.
Attachment #585525 - Flags: review?(roc)
(Assignee)

Comment 50

6 years ago
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.
Attachment #581846 - Attachment is obsolete: true
Attachment #585529 - Flags: review?(roc)
(Assignee)

Comment 51

6 years ago
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.
Attachment #585513 - Flags: review?(roc) → review+
(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.
Attachment #585525 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 56

6 years ago
> 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
(Assignee)

Comment 57

6 years ago
> 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.
(Assignee)

Comment 58

6 years ago
> 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.
(Assignee)

Comment 62

6 years ago
Created attachment 585839 [details] [diff] [review]
part 3, don't include margins in scrollable overflow for popup frames

Added the suggested IsPopup optimization.
Attachment #585524 - Attachment is obsolete: true
Attachment #585839 - Flags: review?(roc)
Attachment #585524 - Flags: review?(roc)
(Assignee)

Comment 63

6 years ago
(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.
Attachment #585839 - Flags: review?(roc) → review+
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 :-)
Attachment #585519 - Flags: review?(roc) → review-
(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.
(Assignee)

Comment 66

6 years ago
> 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.
Attachment #585519 - Flags: review- → review+
(Assignee)

Comment 69

5 years ago
Created attachment 588551 [details]
Assertion details for 265867-1.html
(Assignee)

Comment 70

5 years ago
Created attachment 588552 [details]
Assertion details for 323386-1.html
(Assignee)

Comment 71

5 years ago
Created attachment 588553 [details]
Assertion details for 436822-1.html
(Assignee)

Comment 72

5 years ago
Created attachment 588554 [details]
Assertion details / frame dumps for 515811-1.html
(Assignee)

Comment 73

5 years ago
Created attachment 588556 [details]
Assertion details for 570289-1.html
(Assignee)

Comment 74

5 years ago
>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
(Assignee)

Comment 75

5 years ago
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.
(Assignee)

Comment 76

5 years ago
(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.
Attachment #585529 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Attachment #585500 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #585503 - Flags: review+
(Assignee)

Comment 78

5 years ago
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
Flags: in-testsuite? → in-testsuite+
Whiteboard: [inbound]
Target Milestone: mozilla9 → mozilla12
Version: Other Branch → unspecified
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

Comment 80

5 years ago
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.
Depends on: 724352

Updated

5 years ago
Depends on: 724789

Updated

5 years ago
Depends on: 733711

Comment 82

5 years ago
dev-doc-info
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!
Depends on: 748518
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.

Updated

5 years ago
Depends on: 749386

Updated

5 years ago
Depends on: 749940

Updated

5 years ago
Depends on: 749935

Updated

5 years ago
Depends on: 751278
Depends on: 750293
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.