Closed
Bug 542595
Opened 14 years ago
Closed 14 years ago
Separate "ink overflow area" from "scrollable overflow area"
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta8+ |
People
(Reporter: roc, Assigned: dbaron)
References
(Depends on 2 open bugs)
Details
Attachments
(24 files, 4 obsolete files)
We've talked about this for a long time but I don't think we have a bug for it.
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 1•14 years ago
|
||
I've started working on this in order to fix bug 446693.
Assignee: karlt → dbaron
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.
Assignee | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 years ago
|
||
Oh, and this should be marked as blocking since it blocks a blocker (bug 446693).
blocking2.0: --- → beta8+
Assignee | ||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #477654 -
Flags: superreview?(bzbarsky)
Attachment #477654 -
Flags: review?(roc)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #477657 -
Flags: review?(roc)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #477658 -
Flags: review?(roc)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #477659 -
Flags: review?(roc)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #477660 -
Flags: review?(roc)
Assignee | ||
Comment 11•14 years ago
|
||
Attachment #477661 -
Flags: review?(roc)
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #477662 -
Flags: review?(roc)
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #477664 -
Flags: review?(roc)
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #477665 -
Flags: review?(roc)
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #477666 -
Flags: review?(roc)
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #477667 -
Flags: review?(roc)
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #477668 -
Flags: review?(roc)
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #477670 -
Flags: review?(roc)
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #477671 -
Flags: review?(roc)
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #477672 -
Flags: review?(roc)
Assignee | ||
Comment 21•14 years ago
|
||
Attachment #477674 -
Flags: review?(roc)
Assignee | ||
Comment 22•14 years ago
|
||
Attachment #477677 -
Flags: review?(roc)
Assignee | ||
Comment 23•14 years ago
|
||
Attachment #477678 -
Flags: review?(roc)
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #477679 -
Flags: review?(roc)
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #477680 -
Flags: review?(roc)
Assignee | ||
Comment 26•14 years ago
|
||
Assignee | ||
Comment 27•14 years ago
|
||
This one puzzles me; it's possible I broke something. I'll try to investigate further.
Attachment #477683 -
Flags: review?(roc)
Assignee | ||
Comment 28•14 years ago
|
||
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?
Attachment #477684 -
Flags: review?(matspal)
Assignee | ||
Comment 29•14 years ago
|
||
Comment 30•14 years ago
|
||
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
Attachment #477654 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Reporter | ||
Comment 31•14 years ago
|
||
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?
Assignee | ||
Comment 32•14 years ago
|
||
(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?
Reporter | ||
Comment 33•14 years ago
|
||
Hmm, so you did!
Assignee | ||
Comment 34•14 years ago
|
||
(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)"
Reporter | ||
Comment 35•14 years ago
|
||
Very interesting, thanks!
Reporter | ||
Comment 36•14 years ago
|
||
+ * 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());
Reporter | ||
Comment 37•14 years ago
|
||
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.
Attachment #477657 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477658 -
Flags: review?(roc) → review+
Reporter | ||
Comment 38•14 years ago
|
||
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&.
Reporter | ||
Updated•14 years ago
|
Attachment #477660 -
Flags: review?(roc) → review+
Reporter | ||
Comment 39•14 years ago
|
||
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?
Reporter | ||
Comment 40•14 years ago
|
||
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.)
Attachment #477661 -
Flags: review?(roc) → review+
Reporter | ||
Comment 41•14 years ago
|
||
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.
Attachment #477662 -
Flags: review?(roc) → review+
Reporter | ||
Comment 42•14 years ago
|
||
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.
Attachment #477664 -
Flags: review?(roc) → review+
Reporter | ||
Comment 43•14 years ago
|
||
(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.
Reporter | ||
Comment 44•14 years ago
|
||
(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.
Reporter | ||
Updated•14 years ago
|
Attachment #477665 -
Flags: review?(roc) → review+
Reporter | ||
Comment 45•14 years ago
|
||
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.
Attachment #477666 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477667 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477668 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477670 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477671 -
Flags: review?(roc) → review+
Reporter | ||
Comment 46•14 years ago
|
||
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.
Reporter | ||
Updated•14 years ago
|
Attachment #477674 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477677 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #477678 -
Flags: review?(roc) → review+
Reporter | ||
Comment 47•14 years ago
|
||
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.
Attachment #477679 -
Flags: review?(roc) → review+
Reporter | ||
Comment 48•14 years ago
|
||
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.
Reporter | ||
Comment 49•14 years ago
|
||
(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.
Reporter | ||
Updated•14 years ago
|
Attachment #477683 -
Flags: review?(roc) → review+
Assignee | ||
Comment 50•14 years ago
|
||
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.
Assignee | ||
Comment 51•14 years ago
|
||
(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.
Assignee | ||
Comment 52•14 years ago
|
||
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.
Assignee | ||
Comment 53•14 years ago
|
||
(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?
Assignee | ||
Comment 54•14 years ago
|
||
(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)?
Assignee | ||
Comment 55•14 years ago
|
||
(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).
Assignee | ||
Comment 56•14 years ago
|
||
(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.
Assignee | ||
Comment 57•14 years ago
|
||
(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?
Assignee | ||
Comment 58•14 years ago
|
||
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
Attachment #477654 -
Attachment is obsolete: true
Attachment #478552 -
Flags: review?(roc)
Attachment #477654 -
Flags: review?(roc)
Assignee | ||
Comment 59•14 years ago
|
||
Attachment #477659 -
Attachment is obsolete: true
Attachment #478553 -
Flags: review?(roc)
Attachment #477659 -
Flags: review?(roc)
Assignee | ||
Comment 60•14 years ago
|
||
Attachment #477672 -
Attachment is obsolete: true
Attachment #478554 -
Flags: review?(roc)
Attachment #477672 -
Flags: review?(roc)
Assignee | ||
Updated•14 years ago
|
Attachment #478552 -
Attachment description: the split overflow API → patch 1: the split overflow API
Assignee | ||
Comment 61•14 years ago
|
||
Attachment #477680 -
Attachment is obsolete: true
Attachment #478555 -
Flags: review?(roc)
Attachment #477680 -
Flags: review?(roc)
Reporter | ||
Comment 62•14 years ago
|
||
(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.
Reporter | ||
Comment 63•14 years ago
|
||
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.
Attachment #478552 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #478553 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #478554 -
Flags: review?(roc) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #478555 -
Flags: review?(roc) → review+
Assignee | ||
Comment 64•14 years ago
|
||
(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.
Assignee | ||
Comment 65•14 years ago
|
||
Actually, I'll do Overflow since OverflowAt doesn't make a whole lot of sense.
Assignee | ||
Comment 66•14 years ago
|
||
I filed bug 599650 as followup on SVG effects and bug 599652 as followup on outlines.
Whiteboard: [needs review] → [waiting to land until after 4.0b7 freeze]
Assignee | ||
Comment 67•14 years ago
|
||
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.
Assignee | ||
Comment 68•14 years ago
|
||
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
Assignee | ||
Comment 69•14 years ago
|
||
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.
Assignee | ||
Comment 70•14 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [waiting to land until after 4.0b7 freeze]
Target Milestone: --- → mozilla2.0b8
Depends on: 603220
Comment 71•14 years ago
|
||
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)
Attachment #477684 -
Flags: review?(matspal) → review+
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•