416168 - Layout regression at tryruby.hobix.com

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020708 Minefield/3.0b4pre ID:2008020708

1. New profile, start firefox
2. Visit http://tryruby.hobix.com/
3. Below the 'Interactive ruby ready.' text write:
     help
   and press return

Expected:
- Grey area under the prompt should resize, and the text 'For example, try typing some math. Like: 2 + 6' should be wholly visible.

Actual:
- Grey area under the prompt resizes and cuts the last line of text 'For example, try typing some math. Like: 2 + 6' in half.


Works: 20071204_1843_firefox-3.0b2pre.en-US.win32
Broken: 20071204_1909_firefox-3.0b2pre.en-US.win32

Checkins to module PhoenixTinderbox between 2007-12-04 18:43 and 2007-12-04 19:08 : 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-12-04+18%3A43&maxdate=2007-12-04+19%3A08&cvsroot=%2Fcvsroot

Looks like due to bug 375304 or bug 406568. CC'ng roc.

Comment 1

[Martijn Wargers (dead)]

Attachment: unminimized testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: unminimized testcase2

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

It turns out the scrollHeight property of the containing block isn't taking into account the margin-top and margin-bottom values when the containing block has height: 0px.

See the testcase, the scrollWidth and scrollHeight values should be the same before and after.
The 3rd part of the testcase also shows an issue with margin-left not really working well, but that's a known bug, I think. I guess it's part of what bug 47710 is about.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What happens is that the content-height of the scrolled block is zero, so scrollHeight is entirely determined by the overflow area of the scrolled block. Margins do not contribute to the overflow area so they don't show up in scrollHeight.

I wonder whether it really makes sense to include the bottom-margin in the scrollHeight. Should it be considered "overflowing content" that you can scroll to?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Boris says that it's unclear per spec whether you should be able to scroll to see the bottom margin edge. So I'm calling that not a bug.

The fact that margin-top is not taken into account is definitely a bug because you should be able to scroll down to see the real content. This bug is caused by FinishAndStoreOverflow, which takes the union of the frame's size and its specified overflow area. If the frame's size is empty, that's completely ignored so FinishAndStoreOverflow can actually set overflow areas that don't include the origin of the frame, i.e. with positive y. This confuses nsGfxScrollFrame and causes it to position the scrolled view with positive y and a height that's just the height of the visible content, i.e. not including the top margin.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Urgh, it's not FinishAndStoreOverflow that's at fault, it's everywhere that uses UnionRect to blindly compute the overflow area. But I think I'll fix it in FinishAndStoreOverflow.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes the top-margin issue described above. It also happens to fix the site.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304321 [details] [diff] [review]
fix

You need to grow to accommodate the width and height of the frame as well, since UnionRect ignores any rectangles that have either of width or height empty.  If one but not the other is 0, we still ought to count the dimensions of the frame in overflow.

(Then again, maybe we should have a second version of UnionRect...)

(And if you can, add a test for that too...)

r+sr=dbaron with that fixed

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Note that ConsiderChildOverflow and all the other places where we add children's overflow area to the overflow area for a frame are going to drop zero-width or zero-height children's overflow. Is that what we want? I tend to think it is, although treating the scrolled direct child differently from its descendants is a bit disturbing.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated fix

Handling width:0/height:Npx or the reverse turned out to be a bit more work.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 305141 [details] [diff] [review]
updated fix

>+  // Computes the smallest rectangle that contains both aRect1 and aRect2,
>+  // where empty input rectangles are allowed to affect the result; the
>+  // origin of an empty input rectangle will be inside or on the edge of
>+  // the result.

I'm not sure if this comment about "origin" makes sense given that empty rectangles can have one dimension.

>+  //
>+  // 'this' can be the same object as either aRect1 or aRect2
>+  void UnionRectIncludeEmpty(const nsRect& aRect1, const nsRect& aRect2);


In the long run we might actually want two different rect classes; one with emptiness and one without.  Although I'm not actually sure where we need the former.

>-      // We can't use nsRect::UnionRect since it drops empty rects on
>-      // the floor, and we need to include them.  (Thus we need
>-      // aHaveRect to know when to drop the initial value on the floor.)

Might want to leave this comment.


r+sr=dbaron

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> (From update of attachment 305141 [details] [diff] [review])
> >+  // Computes the smallest rectangle that contains both aRect1 and aRect2,
> >+  // where empty input rectangles are allowed to affect the result; the
> >+  // origin of an empty input rectangle will be inside or on the edge of
> >+  // the result.
> 
> I'm not sure if this comment about "origin" makes sense given that empty
> rectangles can have one dimension.

I should say "top-left"

> >+  //
> >+  // 'this' can be the same object as either aRect1 or aRect2
> >+  void UnionRectIncludeEmpty(const nsRect& aRect1, const nsRect& aRect2);
> 
> In the long run we might actually want two different rect classes; one with
> emptiness and one without.

Maybe, although that might be annoying as well.

> Although I'm not actually sure where we need the
> former.

Situations where we care about the area inside the rectangle more than the rectangle itself. E.g. managing invalidation rects, you want "Union" to discard empty rects and you want empty rects to compare equal. Perhaps more precisely, situations where a rectangle is used as an abstraction for a set of pixels (or other areal unit).

> >-      // We can't use nsRect::UnionRect since it drops empty rects on
> >-      // the floor, and we need to include them.  (Thus we need
> >-      // aHaveRect to know when to drop the initial value on the floor.)
> 
> Might want to leave this comment.

Good idea.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> Situations where we care about the area inside the rectangle more than the
> rectangle itself. E.g. managing invalidation rects, you want "Union" to discard
> empty rects and you want empty rects to compare equal. Perhaps more precisely,
> situations where a rectangle is used as an abstraction for a set of pixels (or
> other areal unit).

I think I just convinced myself that two types really would be a good idea. Although that raises developer-barrier-to-entry in a way that maybe outweights the benefits.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 15

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022702 Minefield/3.0b4pre ID:2008022702
http://tryruby.hobix.com/ now certainly WFM. Not sure what the testcases are meant to be showing, so I'll defer marking this VERIFIED

Comment 16

[Paul-Kenji Cahier]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022703 Minefield/3.0b4pre

Fixed here too.

Comment 17

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050810 Minefield/3.0pre

Verified.