439567 - Overflow area computations should consider empty rects in unions

Status: RESOLVED DUPLICATE of bug 542595

(Core :: Layout, defect)

Description

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

Attachment: testcase

This is kind of a followup to bug 416168.

Bug 421436 caused a regression where the overflow area of a series of <br>s was empty, because each line box overflow area was zero-width. We need to fix that by calling nsRect::UnionRectIncludeEmpty to accumulate the overflow areas.

You can actually reproduce a related bug on trunk, where a series of white-space:pre newlines doesn't add to the overflow area.

Comment 1

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

I think in general we should always consider empty rects when accumulating overflow area, except for a few cases like text shadows and text decorations.

Comment 2

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

Attachment: fix

Here's the patch and some tests.

Comment 3

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

BTW, I audited the callers to UnionRect in layout and decided which ones needed to be converted to UnionRectIncludeEmpty. I also added UnionRectIncludeFirstEmpty for a few cases where we need to accumulate into the overflow area but the second rectangle parameter should be ignored if its empty.

I also wrote about the issue on my blog, hopefully that gives the philosophical background to make sense of these distinctions :-).
http://weblogs.mozillazine.org/roc/archives/2008/06/advanced_topics.html

Comment 4

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

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

Thank goodness the overflow area always includes (0,0) by definition.
This certainly depends on that.

This patch introduces a whole bunch of 80th-column violations.  I'd
prefer if it didn't.

It would be nice if we had a DEBUG-only way to track whether code was
wrong, since it seems like it's easy to get this wrong in the future.
Though perhaps adding DEBUG-only members to nsRect might be more
overhead than we want.

r+sr=dbaron with or without any of these


I don't pretend to understand the logic behind the text-decoration
stuff; it looks like it's always added to overflow from the text frame
even though that's not always where they're painted.

See also bug 434301.

I also noticed bug 428278 (incorrect use of old overflow area in 
nsTableFrame) in the context while going by.

Comment 5

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

I wonder whether it's worth introducing a new nsRect variant, say nsBoundsRect, as I suggested in my blog. That would help avoid getting this wrong in future --- in fact, we wouldn't even need different method names. nsBoundsRect would have no subclass relationship with nsRect. What do you think?

Comment 6

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

Actually, nsBoxRect is probably a better name.

So nsRect would represent a set of area units (e.g. appunit squares), so all zero width/height rects mean the empty set so they're identical, and taking the union with a zero width or height rect means adding the empty set so nothing needs to change.

nsBoxRect would represent a set of points (e.g. appunit coordinates), so zero width or height rects can cover different sets of points (all in the same horizontal or vertical line) so aren't necessarily identical, and taking the union with a zero width or height rect can require expansion.

Does that sound good? I'd add nsBoxRect to nsRect.h/nsRect.cpp and start by converting overflow areas to use it. Probably the frame's nsRect should also become an nsBoxRect but that's not needed yet.

Comment 7

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

It sounds reasonable to me, although I wonder whether it would be more or less confusing than this approach for people new to the code.  It does seem less error-prone, though.

Comment 8

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

Having to distinguish these concepts is suboptimal but I think we definitely have to distinguish them. Given that, I think it's ultimately less confusing to have decisions recorded in types than making fragile choices about method names. I wasn't sure of that originally but I've become more sure after looking again at the patch.

Comment 9

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

ok

Comment 10

[Jet Villegas (inactive)]

Bumping this up. We've got an r+ patch from 2008 and we'd really like to see bug 421436 get fixed.

Comment 11

[Karl Tomlinson (:karlt)]

The testcase shows a functional scrollbar as expected.
A new assignee is needed to check whether this is already in automated tests and, if not, land the tests in the patch here.

Comment 12

[Karl Tomlinson (:karlt)]

Fixed in bug 542595, I assume.