542595 - Separate "ink overflow area" from "scrollable overflow area"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

We've talked about this for a long time but I don't think we have a bug for it.

Comment 1

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

I've started working on this in order to fix bug 446693.

Comment 2

[fantasai]

Relevant threads on www-style:
  http://lists.w3.org/Archives/Public/www-style/2009Aug/0006.html
  http://lists.w3.org/Archives/Public/www-style/2009Aug/0039.html

Note that both box-shadow and outlines qualify as ink overflow.

Comment 3

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

I have patches that compile and work based on cursory manual testing; I still need to write some automated tests and run our existing tests.

Comment 4

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

Oh, and this should be marked as blocking since it blocks a blocker (bug 446693).

Comment 5

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

I'm going to attach a series of patches shortly.

This splits overflow areas into two concepts, the "visual overflow area"
and the "scrollable overflow area".  Right now the scrollable is a
subset of the visual, but it may not stay that way forever.

I removed all uses of the term "combined area" as a synonym for
"overflow area".

This introduces an nsOverflowAreas class that encapsulates the pair of
rectangles.

The differences I introduced between them are:
 * shadows are visual overflow but not scrollable overflow
 * outline is visual overflow but not scrollable overflow
 * SVG effects are visual overflow but not scrollable overflow (I'm not
   completely sure about this one, but I think it makes sense for
   filters to be consistent with shadows)
 * overflowing text decorations (i.e., spellcheck/IME underline) are
   visual but not scrollable
 * overflow of text outside its bounding box is visual but not scrollable

This last change should allow us to take the patch for fixing overflow
issues with ClearType, which I believe either landed or came close to
landing but couldn't stay in due to issues where it caused scrollbars.

Note that CSS transforms still affect both overflow areas.


There are also a few small API cleanups, such as conversion of a few
parameters from pointers to references, removal of some unneeded use of
|virtual| in the table code, and removal of an unused function or two
that would otherwise have needed conversion.

There are also a number of questions noted "REVIEW:" in the patches; I
think we'll be ok either way, but please let me know if you have an
opinion.


The patch series is split up in a somewhat fine-grained manner.
Intermediate states within the series do not compile (except for the
test patches at the end of the series).  The first patch introduces the
new and changed APIs.  The bulk of the patch series implements the
propagation in various parts of reflow.  The last two code patches are
the "miscellaneous" patches, largely distinguished in that the
next-to-last patch is about miscellaneous issues within reflow, and the
last patch is the conversion of callers that use overflow areas outside
of reflow.

Comment 6

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

Attachment: patch 1: the split overflow API

Comment 7

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

Attachment: patch 2: split in nsAbsoluteContainer::Reflow

Comment 8

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

Attachment: patch 3: split in ReflowOverflowContainerChildren

Comment 9

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

Attachment: patch 4: split in FinishAndStoreOverflow

Comment 10

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

Attachment: patch 5: remove unused nsLineBox method

Comment 11

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

Attachment: patch 6: split in nsLineBox

Comment 12

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

Attachment: patch 7: split in nsLineLayout

Comment 13

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

Attachment: patch 8: make visual/scrollable distinctions in text frames

Comment 14

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

Attachment: patch 9: split in nsBlockReflowState::mFloatCombinedArea

Comment 15

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

Attachment: patch 10: split in nsBlockFrame::ComputeCombinedArea

Comment 16

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

Attachment: patch 11: split in nsBlockFrame.cpp's static ComputeCombinedArea

Comment 17

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

Attachment: patch 12: split in float continuation reflow

Comment 18

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

Attachment: patch 13: split in nsBlockReflowContext::PlaceBlock

Comment 19

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

Attachment: patch 14: split in remaining parts of nsBlockFrame

Comment 20

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

Attachment: patch 15: make nsIFrame::ComputeTightBounds use scrollable overflow

Comment 21

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

Attachment: patch 16: fix table cell border overflow handling

Comment 22

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

Attachment: patch 17: make parameters to a bunch of table methods (leading to nsTableFrame::InvalidateFrame) be visual overflow

Comment 23

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

Attachment: patch 18: devirtualize and split overflow in table ReflowChildren methods

Comment 24

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

Attachment: patch 19: remaining changes to reflow code

Comment 25

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

Attachment: patch 20: changes to callers outside of reflow

Comment 26

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

Attachment: patch 21: adjust assertion counts for doubling of certain assertions

Comment 27

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

Attachment: patch 22: mark SVG effects test as passing

This one puzzles me; it's possible I broke something.  I'll try to investigate further.

Comment 28

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

Attachment: patch 23: fix reftests that assume text-shadow causes scrollbars

I couldn't see any way to keep test 3 without just turning it into test 2, so I removed it.  Is that ok, or is there some other way to keep the distinction?

Comment 29

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

Attachment: patch 24: reftests for text-shadow and box-shadow not causing scrollable overflow

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 477654 [details] [diff] [review]
patch 1: the split overflow API

>+nsIFrame::GetOverflowRect(nsOverflowType aType) const

I think giving the structs in mOverflow a name and having a const reference to mOverflow[aType] that you grab up front in this method would be a lot more readable.

sr=me with that

Comment 31

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

Is it worth having an nsRect in nsIFrame just for scrollable overflow? How often to frames have scrollable overflow (compared to how often they have visual overflow)?

I wonder whether passing a overflow type parameter and enumerating over overflow types is the way to go here. If there are never any new overflow types, is this better than just writing some statements/functions twice? At least in the public API?

Comment 32

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

(In reply to comment #31)
> Is it worth having an nsRect in nsIFrame just for scrollable overflow? How
> often to frames have scrollable overflow (compared to how often they have
> visual overflow)?

I can try to gather data on this tomorow.

> I wonder whether passing a overflow type parameter and enumerating over
> overflow types is the way to go here. If there are never any new overflow
> types, is this better than just writing some statements/functions twice? At
> least in the public API?

I tried to make the public API offer both forms; the indexed form for callers that want to operate on both types, and the named form for other callers.  Are you suggesting I removed the indexed forms entirely?  Or something else?

Comment 33

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

Hmm, so you did!

Comment 34

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

(In reply to comment #32)
> (In reply to comment #31)
> > Is it worth having an nsRect in nsIFrame just for scrollable overflow? How
> > often to frames have scrollable overflow (compared to how often they have
> > visual overflow)?
> 
> I can try to gather data on this tomorow.

Some data points from my Linux build:

  maps.google.com has 72 frames with both types of overflow, none with only 1

  http://www.mozilla.com/en-US/ has 111 frames with scrollable overflow and 117 with visual overflow, out of 793 total

  zimbra, calendar view: scrollable: 147 visual: 147 total: 5461
  mxr, nsViewportFrame.cpp: scrollable: 9 visual: 9 total: 4697

  http://arewefastyet.com/ : scrollable: 10 visual: 10 total: 170

  http://www.nytimes.com/ : scrollable: 365 visual: 368 total: 2942

  http://www.asahi.com/ : scrollable: 212 visual: 212 total: 2356

  http://www.spiegel.de/ : scrollable: 328 visual: 328 total: 4048

  http://www.aljazeera.net/portal : scrollable: 420 visual: 427 total: 3510

  http://www.antibes-juanlespins.com/eng/culture/musees/picasso/index.html :
    scrollable: 29 visual: 58 total: 562

  http://hipmunk.com/ search results, SFO->SAN round trip, next 2 saturdays:
    scrollable: 45 visual: 45 total: 830
These numbers would probably be substantially different (visual would be higher) on Windows once we do proper text overflow for ClearType, though.

What I'm using to turn a frame dump into these numbers:

echo "scrollable: $(grep -v "line " out2 | grep scr-over | wc -l) visual: $(grep -v "line " out2 | grep vis-over | wc -l) total: $(grep sc= out2  | wc -l)"

Comment 35

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

Very interesting, thanks!

Comment 36

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

+   * Returns a rect that encompasses the area of this frame that the
+   * user should be able to scroll to reach.  This is similar to
+   * GetVisualOverflowRect, but does not include outline or shadows, and
+   * may in the future include some margins.

It already does include some margins, so this comment should be tweaked to reflect that.

The rest of the API looks fine.

Given that a frame usually has both a visual and scrollable overflow property or neither (which will probably still be true when we add to the visual overflow area for antialiased pixels, because that shouldn't trigger a property), it seems to me we should probably just have a single property which contains two rects --- an nsOverflowAreas.

It also seems to me that we probably don't need an inline mOverflow struct in nsIFrame for scrollable overflow. That only allows an overflow of about 4px (256/60) in each direction, which doesn't seem very likely to be useful for scrollable overflow.

So the invariant would be that if mOverflow is NS_FRAME_OVERFLOW_LARGE, there is a frame property containing visual and scrollable overflow rects. Otherwise the scrollable overflow rect is the frame size and the visual overflow rect is given by combining the offsets in mOverflow with the frame size.

+nsIFrame::GetOverflowAreaProperty(nsOverflowType aType,
+                                  PRBool aCreateIfNecessary) 

I think you can remove the aCreateIfNecessary parameter while you're here. We should always create the property if it's not found; the only caller passing PR_FALSE has already established that the property exists.

+  void SetZero() {
+    mRects[0].SetRect(0, 0, 0, 0);
+    mRects[1].SetRect(0, 0, 0, 0);
+  }

Maybe call this Clear()?

Maybe provide a way to construct an nsOverflowAreas given an nsIFrame*? Then you could implement ConsiderChildOverflow using
  aOverflows.UnionWith(aChildFrame->GetOverflowAreas() + aChildFrame->GetPosition());

Comment 37

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

Comment on attachment 477657 [details] [diff] [review]
patch 2: split in nsAbsoluteContainer::Reflow

+  // FIXME: Should we include their overflow areas in ours?

No, because nsPageFrame clips the nsPageContentFrames to their bounds.

Comment 38

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

Comment on attachment 477659 [details] [diff] [review]
patch 4: split in FinishAndStoreOverflow

   // 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)) {
-    aOverflowArea->UnionRectIncludeEmpty(*aOverflowArea,
-                                         nsRect(nsPoint(0, 0), aNewSize));
+    NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+      aOverflowAreas[otype].UnionRectIncludeEmpty(aOverflowAreas[otype],
+                                              nsRect(nsPoint(0, 0), aNewSize));
+    }

We only need to do this for scrollable overflow, not visual overflow. I think we should change this to reflect that.

+  // REVIEW: Nothing in here should affect scrollable overflow, except
+  // possibly SVG effects.  But I tend to think they shouldn't.

I agree.

+    // REVIEW: Transform affects both overflow areas.

I agree.

+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    nsRect bounds(0, 0, aDesiredSize.width, aDesiredSize.height);
+    overflow[otype].UnionRect(overflow[otype], bounds);
+  }

How about
  nsRect bounds(0, 0, aDesiredSize.width, aDesiredSize.height);
  overflow.UnionWith(nsOverflowAreas(bounds, bounds));
?

+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    overflow[otype].UnionRect(nsRect(0,0,rowRect.width, rowRect.height),
+                              overflow[otype]);
+  }
...
+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    nsRect& r = overflow[otype];
+    r.UnionRect(r, nsRect(0, 0, groupRect.width, groupRect.height));
+  }

Similar here. Or add a method to nsOverflowAreas that unions in a given const nsRect&.

Comment 39

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

Comment on attachment 477654 [details] [diff] [review]
patch 1: the split overflow API

Now that I've seen the [] operator on nsOverflowAreas in action, I think it's a little too magical. How about GetRect(aType) and SetRect(aType, const nsRect&) methods instead?

Comment 40

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

Comment on attachment 477661 [details] [diff] [review]
patch 6: split in nsLineBox

+      // REVIEW: should this use IsExactEqual?
+      mData->mOverflowAreas[eVisualOverflow] == mBounds &&
+      mData->mOverflowAreas[eScrollableOverflow] == mBounds) {

Yes, it definitely should for eScrollableOverflow. For eVisualOverflow it probably shouldn't. (The difference is because the scrollable overflow is a point-set rectangle and the visual overflow is a pixel-set rectangle, in the terminology of http://weblogs.mozillazine.org/roc/archives/2008/06/advanced_topics.html.)

Comment 41

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

Comment on attachment 477662 [details] [diff] [review]
patch 7: split in nsLineLayout

  // XXXldb This returned width as -15, 2001-06-12, Bugzilla

I think we can remove this now? :-)

     // The minimum combined area for the frames that are direct
     // children of the block starts at the upper left corner of the
     // line and is sized to match the size of the line's bounding box
     // (the same size as the values returned from VerticalAlignFrames)
-    combinedAreaResult.x = psd->mLeftEdge;
+    overflowAreas[eVisualOverflow].x = psd->mLeftEdge;
     // If this turns out to be negative, the rect will be treated as empty.
     // Which is just fine.
-    combinedAreaResult.width = psd->mX - combinedAreaResult.x;
-    combinedAreaResult.y = mTopEdge;
-    combinedAreaResult.height = mFinalLineHeight;
+    overflowAreas[eVisualOverflow].width =
+      psd->mX - overflowAreas[eVisualOverflow].x;
+    overflowAreas[eVisualOverflow].y = mTopEdge;
+    overflowAreas[eVisualOverflow].height = mFinalLineHeight;
+
+    overflowAreas[eScrollableOverflow] = overflowAreas[eVisualOverflow];

Seems like the visual overflow here can just be empty? I don't really mind. I guess your patches are preserving the invariant that the visual overflow area contains the frame bounds, and doing something similar for lines is kinda consistent, but I think we don't actually need that invariant anymore.

Comment 42

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

Comment on attachment 477664 [details] [diff] [review]
patch 8: make visual/scrollable distinctions in text frames

+  aMetrics.SetOverflowAreasToRect();

Probably would be better to call this method SetOverflowAreasToDesiredBounds()? "Rect" is a bit ambiguous.

Comment 43

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

(In reply to comment #39)
> Comment on attachment 477654 [details] [diff] [review]
> patch 1: the split overflow API
> 
> Now that I've seen the [] operator on nsOverflowAreas in action, I think it's a
> little too magical. How about GetRect(aType) and SetRect(aType, const nsRect&)
> methods instead?

Actually nsRect& Rect(aType) would be nice.

Comment 44

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

(In reply to comment #36)
> Maybe provide a way to construct an nsOverflowAreas given an nsIFrame*? Then
> you could implement ConsiderChildOverflow using
>   aOverflows.UnionWith(aChildFrame->GetOverflowAreas() +
> aChildFrame->GetPosition());

This would be useful in part 9 nsBlockReflowState::FlowAndPlaceFloat as well.

Comment 45

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

Comment on attachment 477666 [details] [diff] [review]
patch 10: split in nsBlockFrame::ComputeCombinedArea

+      nsRect r = mBullet->GetRect();
+      NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+        areas[otype].UnionRect(areas[otype], r);
+      }

Another place it would be useful to have an nsOverflowAreas method that unions in a given rect.

Comment 46

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

Comment on attachment 477672 [details] [diff] [review]
patch 15: make nsIFrame::ComputeTightBounds use scrollable overflow

 nsTextFrame::ComputeTightBounds(gfxContext* aContext) const
 {
   if ((GetStyleContext()->HasTextDecorations() &&
        eCompatibility_NavQuirks == PresContext()->CompatibilityMode()) ||
       (GetStateBits() & TEXT_HYPHEN_BREAK)) {
     // This is conservative, but OK.
-    return GetOverflowRect();
+    return GetScrollableOverflowRect();

This is actually not what we want. MathML definitely wants ComputeTightBounds on text frames to return glyph bounds.

I'm OK with having ComputeTightBounds return scrollable overflow when decorations are involved. I think it's good for visual bounds to normally not affect layout; that will let us do optimizations later. But glyph bounds will have to be an exception.

Comment 47

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

Comment on attachment 477679 [details] [diff] [review]
patch 19: remaining changes to reflow code

+  // REVIEW: Maybe this should contribute only to scrollable overflow
+  // and not visual?

I'm not sure. I get the feeling mBoundingBox was simply trying to account for glyph extents. We may not need it anymore. 

+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    nsRect &r = aMetrics->mOverflowAreas[otype];
+    r.UnionRect(r, boundingBox);
+  }
...
+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    nsRect &r = aDesiredSize.mOverflowAreas[otype];
+    r.UnionRect(r, tableRect);
+  }

More candidates for a method that adds a rect to the overflow areas.

+  NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
+    aDesiredSize.mOverflowAreas[otype] = GetOverflowRect(otype);
+  }

A number of occurrences of this; all would be simplified by a helper which returns an nsOverflowAreas from a frame.

Comment 48

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

Comment on attachment 477680 [details] [diff] [review]
patch 20: changes to callers outside of reflow

+  // REVIEW: Do we really want the visual overflow rect here?
+  return aFrame->GetVisualOverflowRect();

No, we want the scrollable rect. (Or maybe we should even use the border-box?)

@@ -844,17 +844,17 @@ nsLayoutUtils::GetPopupFrameForEventCoor
     return nsnull;
   }
   nsTArray<nsIFrame*> popups = pm->GetVisiblePopups();
   PRUint32 i;
   // Search from top to bottom
   for (i = 0; i < popups.Length(); i++) {
     nsIFrame* popup = popups[i];
     if (popup->PresContext()->GetRootPresContext() == aPresContext &&
-        popup->GetOverflowRect().Contains(
+        popup->GetVisualOverflowRect().Contains(

I think you should use the scrollable overflow here (or anywhere else involving events). We should not be receiving events in decorations that expand the visual overflow. If we ever allow the visual overflow to shrink below the border-box, using the visual overflow for events would break.

+  // REVIEW: do we want visual here?
+  nsRect result(GetVisualOverflowRect());

I think so (although it probably doesn't matter).

+    // This is for shrink-to-fit, and therefore we want to use the
+    // scrollable overflow, since the purpose of shrink to fit is to
+    // make the content that ought to be reachable (represented by the
+    // scrollable overflow) fit in the page.
+    if (frame->HasScrollableOverflowRect()) {

Actually I'm not convinced. Wouldn't it make sense to shrink enough to avoid cutting off decorations?

 GetPreEffectsOverflowRect(nsIFrame* aFrame)
 {
   nsRect* r = static_cast<nsRect*>
     (aFrame->Properties().Get(nsIFrame::PreEffectsBBoxProperty()));
   if (r)
     return *r;
-  return aFrame->GetOverflowRect();
+  return aFrame->GetVisualOverflowRect();

This feeds into the calculation of the "SVG bbox" for non-SVG content, which in turn affects the interpretation of "bbox-relative" SVG coordinates in filter, mask and clip-path. So I think this should actually use the scrollable overflow.

In fact, I don't think we need PreEffectsBBoxProperty anymore; we can just use the scrollable overflow instead, since it excludes effects.

Comment 49

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

(In reply to comment #41)
> Seems like the visual overflow here can just be empty? I don't really mind. I
> guess your patches are preserving the invariant that the visual overflow area
> contains the frame bounds, and doing something similar for lines is kinda
> consistent, but I think we don't actually need that invariant anymore.

Except that if we don't routinely include the frame bounds in the visual overflow area, our space optimization that depends on the visual overflow area being close to the frame bounds won't work. So never mind.

We probably don't care about this for lines, though.

Comment 50

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

While working on comment 36, I think I've determined that the current deltas code is pretty broken.  In particular, FinishAndStoreOverflow is called during reflow *before* SetRect is called, but is passed aNewSize.  We store deltas relative to aNewSize.  Then SetRect calls GetOverflowRect, which isn't actually valid yet because the rect hasn't yet been set to the rect in question.

I'm tempted to make SetRect and SetSize just not do anything about overflow; I think that's more likely to be correct than the current code.  But it still could be wrong for callers who don't honor the frame's desired size.  Maybe, instead, I should add a boolean to SetRect and SetSize, or maybe just rip out the deltas code until we can do it right.

Comment 51

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

(In reply to comment #50)
> While working on comment 36, I think I've determined that the current deltas
> code is pretty broken.  In particular, FinishAndStoreOverflow is called during
> reflow *before* SetRect is called, but is passed aNewSize.  We store deltas
> relative to aNewSize.  Then SetRect calls GetOverflowRect, which isn't actually
> valid yet because the rect hasn't yet been set to the rect in question.

Er, no, we store deltas relative to the current size.  So it just means that we allocate a large number of very temporary properties.

Comment 52

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

Er, except they're not very temporary since SetRect doesn't get rid of them.

Anyway, the current code for deltas doesn't look like it's effective at all, but it's not actually functionally broken, so I'll just try to leave it as-is.

Comment 53

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

(In reply to comment #38)
> Comment on attachment 477659 [details] [diff] [review]
> patch 4: split in FinishAndStoreOverflow
> 
>    // 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)) {
> -    aOverflowArea->UnionRectIncludeEmpty(*aOverflowArea,
> -                                         nsRect(nsPoint(0, 0), aNewSize));
> +    NS_FOR_FRAME_OVERFLOW_TYPES(otype) {
> +      aOverflowAreas[otype].UnionRectIncludeEmpty(aOverflowAreas[otype],
> +                                              nsRect(nsPoint(0, 0),
> aNewSize));
> +    }
> 
> We only need to do this for scrollable overflow, not visual overflow. I think
> we should change this to reflect that.

Does "this" mean the unioning, or the excluding of inline frames?  If the unioning, then this is what you changed your mind on in comment 49, right?  If the inline frames, why, as it seems like it's better to have less overflow when we can?

Comment 54

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

(In reply to comment #39)
> Comment on attachment 477654 [details] [diff] [review]
> patch 1: the split overflow API
> 
> Now that I've seen the [] operator on nsOverflowAreas in action, I think it's a
> little too magical. How about GetRect(aType) and SetRect(aType, const nsRect&)
> methods instead?

I'd prefer something more parallel to ScrollableOverflow() and VisualOverflow().  How about OverflowAt(aType)?

Comment 55

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

(In reply to comment #47)
> Comment on attachment 477679 [details] [diff] [review]
> patch 19: remaining changes to reflow code
> 
> +  // REVIEW: Maybe this should contribute only to scrollable overflow
> +  // and not visual?
> 
> I'm not sure. I get the feeling mBoundingBox was simply trying to account for
> glyph extents. We may not need it anymore. 

I'm inclined to leave the comment in, but fix the fact that I wrote it backwards (should be visual but not scrollable, I think).

Comment 56

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

(In reply to comment #48)
> Comment on attachment 477680 [details] [diff] [review]
> patch 20: changes to callers outside of reflow
> 
> +  // REVIEW: Do we really want the visual overflow rect here?
> +  return aFrame->GetVisualOverflowRect();
> 
> No, we want the scrollable rect. (Or maybe we should even use the border-box?)

Actually, I think we want to keep it visual for now, since we want it to be consistent with the property storing which is in ComputeOutlineAndEffectsRect, which currently includes shadows in the rect that we draw the outline around.

But I should probably file a followup bug on changing this.

> +    // This is for shrink-to-fit, and therefore we want to use the
> +    // scrollable overflow, since the purpose of shrink to fit is to
> +    // make the content that ought to be reachable (represented by the
> +    // scrollable overflow) fit in the page.
> +    if (frame->HasScrollableOverflowRect()) {
> 
> Actually I'm not convinced. Wouldn't it make sense to shrink enough to avoid
> cutting off decorations?

Did you mean text-decoration specifically, or decorations including things like shadows?  I think shrinking the size of the page contents to ensure that all of the not-even-visible parts of a shadow blur are on the page probably isn't what we want.  And I think if that content is important, the author will probably have figured something else out for the screen display as well.

>  GetPreEffectsOverflowRect(nsIFrame* aFrame)
>  {
>    nsRect* r = static_cast<nsRect*>
>      (aFrame->Properties().Get(nsIFrame::PreEffectsBBoxProperty()));
>    if (r)
>      return *r;
> -  return aFrame->GetOverflowRect();
> +  return aFrame->GetVisualOverflowRect();
> 
> This feeds into the calculation of the "SVG bbox" for non-SVG content, which in
> turn affects the interpretation of "bbox-relative" SVG coordinates in filter,
> mask and clip-path. So I think this should actually use the scrollable
> overflow.
> 
> In fact, I don't think we need PreEffectsBBoxProperty anymore; we can just use
> the scrollable overflow instead, since it excludes effects.

OK; I'll change that in an extra patch on top.

Comment 57

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

(In reply to comment #48)
>  GetPreEffectsOverflowRect(nsIFrame* aFrame)
>  {
>    nsRect* r = static_cast<nsRect*>
>      (aFrame->Properties().Get(nsIFrame::PreEffectsBBoxProperty()));
>    if (r)
>      return *r;
> -  return aFrame->GetOverflowRect();
> +  return aFrame->GetVisualOverflowRect();
> 
> This feeds into the calculation of the "SVG bbox" for non-SVG content, which in
> turn affects the interpretation of "bbox-relative" SVG coordinates in filter,
> mask and clip-path. So I think this should actually use the scrollable
> overflow.
> 
> In fact, I don't think we need PreEffectsBBoxProperty anymore; we can just use
> the scrollable overflow instead, since it excludes effects.

Actually, wouldn't making this change break the application of SVG effects to content with shadows?  The PreEffectsBBoxProperty includes the effects of shadows, and when the shadows aren't present it's therefore more like the visual overflow, so I think this should also stay visual to be consistent.  Or does it really not need to include everything that you want to run through the effects?

Comment 58

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

Attachment: patch 1: the split overflow API

Many of the patches have been revised per the comments above; I'm only going to reattach the ones that need another review pass.  All of them are in:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/44b2cd05a102

Comment 59

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

Attachment: patch 4: split in FinishAndStoreOverflow

Comment 60

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

Attachment: patch 15: make nsIFrame::ComputeTightBounds use scrollable overflow

Comment 61

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

Attachment: patch 20: changes to callers outside of reflow

Comment 62

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

(In reply to comment #53)
> Does "this" mean the unioning, or the excluding of inline frames?  If the
> unioning, then this is what you changed your mind on in comment 49, right?

Right. So disregard that part of comment #38.

(In reply to comment #54)
> I'd prefer something more parallel to ScrollableOverflow() and
> VisualOverflow().  How about OverflowAt(aType)?

OK, or just Overflow(aType).

(In reply to comment #55)
> I'm inclined to leave the comment in, but fix the fact that I wrote it
> backwards (should be visual but not scrollable, I think).

Sounds fine.

(In reply to comment #56)
> Actually, I think we want to keep it visual for now, since we want it to be
> consistent with the property storing which is in ComputeOutlineAndEffectsRect,
> which currently includes shadows in the rect that we draw the outline around.

OK.

> But I should probably file a followup bug on changing this.

Please! We should probably just rework outline drawing completely.

> > +    // This is for shrink-to-fit, and therefore we want to use the
> > +    // scrollable overflow, since the purpose of shrink to fit is to
> > +    // make the content that ought to be reachable (represented by the
> > +    // scrollable overflow) fit in the page.
> > +    if (frame->HasScrollableOverflowRect()) {
> > 
> > Actually I'm not convinced. Wouldn't it make sense to shrink enough to avoid
> > cutting off decorations?
> 
> Did you mean text-decoration specifically, or decorations including things
> like shadows?

I meant decorations including shadows.

> I think shrinking the size of the page contents to ensure that
> all of the not-even-visible parts of a shadow blur are on the page probably
> isn't what we want.  And I think if that content is important, the author
> will probably have figured something else out for the screen display as well.

Maybe. It just seems to me that for printing shrink-to-fit it's harmless to make the page a bit smaller than optimal. But I'm OK with either way.

(In reply to comment #57)
> (In reply to comment #48)
> Actually, wouldn't making this change break the application of SVG effects to
> content with shadows?  The PreEffectsBBoxProperty includes the effects of
> shadows, and when the shadows aren't present it's therefore more like the
> visual overflow, so I think this should also stay visual to be consistent.  Or
> does it really not need to include everything that you want to run through the
> effects?

It does. Currently SVG filter code does not distinguish between the visual overflow area of the source image and its "SVG bbox" that defines filter coordinates, but it could. Obviously you don't want to fix that here so the only question is what we're going to live with in the interim. I suppose letting the coordinates be based on the visual overflow is better than cutting off some content, so OK, let's leave your code as-is.

Comment 63

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

Comment on attachment 478552 [details] [diff] [review]
patch 1: the split overflow API

+  // If mOverflow[i].mType == NS_FRAME_OVERFLOW_LARGE, then the

This change is now obsolete.

Comment 64

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

(In reply to comment #62)
> (In reply to comment #54)
> > I'd prefer something more parallel to ScrollableOverflow() and
> > VisualOverflow().  How about OverflowAt(aType)?
> 
> OK, or just Overflow(aType).

I'm inclined to keep OverflowAt since it's a little more searchable.

Comment 65

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

Actually, I'll do Overflow since OverflowAt doesn't make a whole lot of sense.

Comment 66

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

I filed bug 599650 as followup on SVG effects and bug 599652 as followup on outlines.

Comment 67

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

I also filed bug 599689 to make nsBlockReflowContext::ReflowBlock stop covering up for frames that fail to set overflow correctly.  In the most recent patch 20, I'd changed this to an assertion, but I had to change it back because it fired a bunch of times in crashtests.

Comment 68

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

I actually needed to revert a bunch of the changes I made on Friday in order to get a single (!) reftest to pass again.  It was a rather serious mistake with the mDeltas stuff:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/a21aafa765e4

Comment 69

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

I looked a little further into patch 22:

The SVG reftest in question was failing due to bug 541270, incorrect computation of filter effects region.  In one case, the failure was because the region ended up too large, and therefore caused a scrollbar.  This case is fixed by the patch, because the incorrect region no longer causes a scrollbar.  In the other case, the region ends up too small, and therefore an area is not painted.  This test still fails.

So I think that reftest (svg-effects-area-zoomed-in.xhtml) starting to pass as a result of these changes is just fine.

Comment 70

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

http://hg.mozilla.org/mozilla-central/rev/4abdb488ea62
http://hg.mozilla.org/mozilla-central/rev/28874ce55ee1
http://hg.mozilla.org/mozilla-central/rev/1b5cdef4b9d6
http://hg.mozilla.org/mozilla-central/rev/ce61761d254d
http://hg.mozilla.org/mozilla-central/rev/1302a184ae9c
http://hg.mozilla.org/mozilla-central/rev/6e1cf319e5b2
http://hg.mozilla.org/mozilla-central/rev/a8f0130ffa83
http://hg.mozilla.org/mozilla-central/rev/04e34daab502
http://hg.mozilla.org/mozilla-central/rev/ca6684b7a11c
http://hg.mozilla.org/mozilla-central/rev/96a52ac05006
http://hg.mozilla.org/mozilla-central/rev/ed9ebff7469d
http://hg.mozilla.org/mozilla-central/rev/25b99ff44f9c
http://hg.mozilla.org/mozilla-central/rev/0de9480228c0
http://hg.mozilla.org/mozilla-central/rev/9292ed4f13c7
http://hg.mozilla.org/mozilla-central/rev/89551308e585
http://hg.mozilla.org/mozilla-central/rev/0a7a368d748f
http://hg.mozilla.org/mozilla-central/rev/40788d629f3c
http://hg.mozilla.org/mozilla-central/rev/061f5e6d58be
http://hg.mozilla.org/mozilla-central/rev/08860c83bf88
http://hg.mozilla.org/mozilla-central/rev/070567151424
http://hg.mozilla.org/mozilla-central/rev/7fa3c82dc40f
http://hg.mozilla.org/mozilla-central/rev/b9d3ba34dc5e
http://hg.mozilla.org/mozilla-central/rev/b492a3d8bc1c
http://hg.mozilla.org/mozilla-central/rev/58e3696a0dad
Please note that this is NOT in beta 7.

Comment 71

[Mats Palmgren (inactive)]

Comment on attachment 477684 [details] [diff] [review]
patch 23: fix reftests that assume text-shadow causes scrollbars

> patch 23: fix reftests that assume text-shadow causes scrollbars

The whole point of these two tests were to check that text-shadow
did cause a scrollbar. One test for HTML in XUL, one for plain XUL.

Your change makes it test something completely different, which is
fine, but why didn't you just change "!=" to "==" in the manifest
if that's the new expected behavior? (ie. that a overflowing
text-shadow should NOT cause a scrollbar)