Found in m-c: BuildID=20180511140745 SourceStamp=4303d49c53931385892231969e40048f096b4d4c Assertion failure: aRadiusY > 0, at src/layout/generic/nsFloatManager.cpp:2933 #0 nsFloatManager::ShapeInfo::XInterceptAtY(int, int, int) src/layout/generic/nsFloatManager.cpp:2933:3 #1 nsFloatManager::EllipseShapeInfo::EllipseShapeInfo(nsPoint const&, nsSize const&, int, int) src/layout/generic/nsFloatManager.cpp:872:9 #2 mozilla::detail::UniqueSelector<nsFloatManager::EllipseShapeInfo>::SingleObject mozilla::MakeUnique<nsFloatManager::EllipseShapeInfo, nsPoint&, nsSize&, int&, int&>(nsPoint&, nsSize&, int&, int&) src/obj-firefox/dist/include/mozilla/UniquePtr.h:680:27 #3 nsFloatManager::ShapeInfo::CreateCircleOrEllipse(mozilla::UniquePtr<mozilla::StyleBasicShape, mozilla::DefaultDelete<mozilla::StyleBasicShape> > const&, int, nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) src/layout/generic/nsFloatManager.cpp:2731:10 #4 nsFloatManager::ShapeInfo::CreateBasicShape(mozilla::UniquePtr<mozilla::StyleBasicShape, mozilla::DefaultDelete<mozilla::StyleBasicShape> > const&, int, nsIFrame*, mozilla::LogicalRect const&, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) src/layout/generic/nsFloatManager.cpp:2598:14 #5 nsFloatManager::FloatInfo::FloatInfo(nsIFrame*, int, int, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) src/layout/generic/nsFloatManager.cpp:2404:20 #6 nsFloatManager::AddFloat(nsIFrame*, mozilla::LogicalRect const&, mozilla::WritingMode, nsSize const&) src/layout/generic/nsFloatManager.cpp:260:13 #7 mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) src/layout/generic/BlockReflowInput.cpp:994:19 #8 mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) src/layout/generic/BlockReflowInput.cpp:627:14 #9 nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) src/layout/generic/nsLineLayout.cpp:966:11 #10 nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) src/layout/generic/nsBlockFrame.cpp:4158:15 #11 nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) src/layout/generic/nsBlockFrame.cpp:3958:5 #12 nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3832:9 #13 nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2816:5 #14 nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) src/layout/generic/nsBlockFrame.cpp:2352:7 #15 nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsBlockFrame.cpp:1225:3 #16 nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) src/layout/generic/nsBlockReflowContext.cpp:306:11 #17 nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3463:11 #18 nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2813:5 #19 nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) src/layout/generic/nsBlockFrame.cpp:2352:7 #20 nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsBlockFrame.cpp:1225:3 #21 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:951:14 #22 nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsCanvasFrame.cpp:713:5 #23 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:951:14 #24 nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) src/layout/generic/nsGfxScrollFrame.cpp:555:3 #25 nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) src/layout/generic/nsGfxScrollFrame.cpp:678:3 #26 nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsGfxScrollFrame.cpp:1055:3 #27 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:995:14 #28 mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/ViewportFrame.cpp:335:7 #29 mozilla::PresShell::DoReflow(nsIFrame*, bool) src/layout/base/PresShell.cpp:8981:11 #30 mozilla::PresShell::ProcessReflowCommands(bool) src/layout/base/PresShell.cpp:9154:24 #31 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4366:11 #32 mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType) src/layout/base/PresShell.cpp:4159:3 #33 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:980:12 #34 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7171:21 #35 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6964:7 #36 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp #37 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1315:3 #38 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:858:14 #39 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:747:9 #40 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:632:5 #41 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp #42 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28 #43 nsIDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:8397:18 #44 nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8319:9 #45 nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5299:3 #46 mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1216:13 #47 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32 #48 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14 #49 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10 #50 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21 #51 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10 #52 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3 #53 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27 #54 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22 #55 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9 #56 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10 #57 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3 #58 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34 #59 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30 #60 main src/browser/app/nsBrowserApp.cpp:282:18 #61 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291 #62 _start (firefox+0x423444)

Comment on attachment 8975990 [details] Bug 1461046 Part 1: Change EllipseShapeInfo to tolerate empty circles/ellipses and treat them as singular points/lines (possibly expanded by shape-margin). https://reviewboard.mozilla.org/r/244200/#review250828 A few nits, though look at my "Why should empty ellipses stay empty" one first, because that one is the most substantial & is about the key premise of this patch. ::: layout/generic/nsFloatManager.cpp:691 (Diff revision 3) > EllipseShapeInfo(const nsPoint& aCenter, > const nsSize& aRadii, > - nscoord aShapeMargin); > + nscoord aShapeMargin, > + bool tolerateEmptiness); Two naming nits: (1) args should have an "a" prefix -- so s/tolerateEmptiness/aTolerateEmptiness/ (2) ...but "tolerate emptiness" is probably not a clear enough wording for the name. It's not superficially clear to me which behavior this would trigger (i.e. it's not clear whether "tolerate" means proceed-with-expansion vs. keep-empty-things-empty) Maybe "aApplyMarginWhenEmpty" would be clearer? ::: layout/generic/nsFloatManager.cpp:781 (Diff revision 3) > + // Empty ellipses stay empty, unless we've been asked to ignore that fact. > + // We tolerate emptiness when the EllipseShapeInfo is being used as part of > + // another non-empty shape. > + if (IsEmpty() && !tolerateEmptiness) { (Do we know for sure that the other shape is non-empty? What if we're invoked as part of a rounded-rect that happens to be empty?) ::: layout/generic/nsFloatManager.cpp:781 (Diff revision 3) > MOZ_ASSERT(RadiiAreRoughlyEqual(aRadii) || > ShapeMarginIsNegligible(aShapeMargin), > "This constructor should only be called when margin is " > "negligible or radii are roughly equal."); > > + // Empty ellipses stay empty, unless we've been asked to ignore that fact. Why should empty ellipses stay empty? It's not clear to me that that's the correct behavior. Conceptually, an empty ellipse is a single point (or a flat line, if it's only empty in one axis). And the definition of shape-margin[1] says that we *should* expand the float area outwards from that point/line. At first glance, I don't see any spec justification for making the shape emptiness "sticky" like you're doing here... As long as we have a shape with defined bounds, we can apply a margin to grow outwards from those bounds -- even when the original shape's bounds are a singularity, like a single point. [1] https://drafts.csswg.org/css-shapes-1/#shape-margin-property

From quick testing, it looks like Chrome treats shape-margin expand an empty ellipse (centered at a specific position), with markup like the following: shape-outside: ellipse(0px 0px at 100px 100px); shape-margin: 50px; Demo: https://jsfiddle.net/ua0fvp4g/ Intuitively, I would expect it should be the same for empty polygons (or empty "pieces of polygons", e.g. a horizontal line jutting out and back from a nonempty polygon), as well. Though I couldn't get Chrome to agree with my expectations there, from brief testing. The spec does say "polygons ...(...with...vertices arranged to enclose no area) result in an empty float area" -- but I don't know whether it means to say they result in an empty float are *even in the presence of shape-margin*... Empty polygons do still have a defined boundary (at least one single "sharp" point) from which shape-margin could conceptually expand the shape to create something nonempty, per https://drafts.csswg.org/css-shapes-1/#shape-margin-property (I can imagine this being harder to implement using the Chamfer Matrix technique, though, since that depends on finding points that are "inside" and working outwards from there...)

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #13) > From quick testing, it looks like Chrome treats shape-margin expand sorry, s/Chrome treats/Chrome lets/

Comment on attachment 8975990 [details] Bug 1461046 Part 1: Change EllipseShapeInfo to tolerate empty circles/ellipses and treat them as singular points/lines (possibly expanded by shape-margin). https://reviewboard.mozilla.org/r/244200/#review250828 > (Do we know for sure that the other shape is non-empty? What if we're invoked as part of a rounded-rect that happens to be empty?) (er, I see Part 2 addresses this review comment about the other shape being non-empty -- tapping "drop" for this one.)

Comment on attachment 8976360 [details] Bug 1461046 Part 3: Change RoundedBoxShapeInfo to tolerate empty rects. https://reviewboard.mozilla.org/r/244526/#review250864 ::: commit-message-427a8:6 (Diff revision 3) > +float area that has no effect on line boxes. The shape-margin value is > +relevant only if there is a non-empty float area. These tests define empty This quote here -- "The shape-margin value is relevant only if there is a non-empty float area" -- seems to be the key claim for this bug here. I don't see spec support for that claim right now, but it's entirely possible that's the intent of the spec and it just needs clarification. Could you seek clarification from the CSSWG? (or let me know if this has already been clarified somewhere that I'm not seeing)

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #16) > Comment on attachment 8976360 [details] > This quote here -- "The shape-margin value is relevant only if there is a > non-empty float area" -- seems to be the key claim for this bug here. > > I don't see spec support for that claim right now, but it's entirely > possible that's the intent of the spec and it just needs clarification. > > Could you seek clarification from the CSSWG? (or let me know if this has > already been clarified somewhere that I'm not seeing) There is a CSSWG thread at https://github.com/w3c/csswg-drafts/issues/2375 where I asked the narrower question about empty polygons with positive shape-margin. That got muddled into discussions on whether or not shape-outside + shape-margin should match SVG stroke behavior, and whether zero-area struts within the polygon should be considered part of the shape. However, the most relevant comments I think are comments 2 and 3 from astearns, which state: > There are also degenerate circles, ellipses and inset rects (which might need a larger value of shape-margin than merely >0 to > amount to non-empty) > I'm open to considering the change if there's a good reason to take it. We did consider this case in the early stages of the > spec, and went back and forth a few times over it (as I recall). Together, I read those two comments thusly: 1) The degenerate polygon in my example should be handled similarly to other degenerate shapes derived from circle, ellipse, and inset. 2) The proposal in my original post to expand the polygon shape area by the shape-margin for purposes of determining an empty float area is a "change" from the current intent of the spec. So if degenerate (empty) polygons are comparable to empty circles, ellipses, insets, and expanding the shape area by shape-margin for purposes of determining an empty float area would be a change to the spec, then I believe that this patch brings us into compliance with the spec. Certainly upstreaming the WPTs would give the spec owners a push to clarify the spec.

I come away from that github issue (and the spec text) with the impression that: - This simply isn't a case the spec editors considered thoroughly, when writing the current spec text. It's not that they intended to require tolerateEmptiness=false, but rather that they didn't consider this scenario or didn't think to spec it clearly. - AFAICT, everyone on that spec issue is advocating for **at least** amending the spec to clarify that shape-margin does apply to degenerate shapes. (The two options that astearns laid out[a] there are (1) that and (2) something more extreme than that) So I don't think it would be a good idea to check in web platform tests that require something that the spec doesn't currently require, and that the spec will [presumably] soon require something different from. And it's probably also undesirable to add complexity to our code (the tolerateEmptiness switch) to do that as well, if it's not aligned with the current spec & particularly not aligned with where the spec is going. If it's not much more complicated, I'd rather resolve this bug in the direction of effectively doing tolerateEmptiness=true everywhere, since I think (?) that's closer to where things are going. Even if we're not entirely consistent between degenerate polygons vs. degenerate circles, we'll be more interoperable with Chrome and closer to what the spec is apparently going to require (rather than further away from it). [a] https://github.com/w3c/csswg-drafts/issues/2375#issuecomment-370053992

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #18) > So I don't think it would be a good idea to check in web platform tests that > require something that the spec doesn't currently require, and that the spec > will [presumably] soon require something different from. Obviously I'm not saying we shouldn't check in tests :) but rather that any tests we check in should either: - ...be in a Mozilla-specific test directory (if their requirements aren't grounded in the spec, & if the test is intended mostly to document/enforce our current behavior & not to impose conformance requirements on other rendering engines) Or: - ...have their requirements grounded pretty clearly in spec text, and/or link to a github issue where there's support for what the test is requiring & where that spec text will presumably be added.

I don't think there are good answers here. We could make the code tolerate an empty ellipse (the triggering issue for this bug), but I don't think that leads to self-consistency across the shapes. Consider this example modified from the inset test in Part 3: > #test-shape { > float: right; > width: 200px; > height: 200px; > shape-margin: 50px; > shape-outside: inset(100px 100px 125px 125px); > } Obviously, disregarding shape-margin, the inset rect is empty. It's also in some sense smaller than empty because its X() is more than its XMost() and the same for Y() and YMost(). Let's say we want to define the float area by expanding that rect by the shape-margin value of 50px. Where is the center of the inverted rect? It could be at the center of the "inside-out" rect I suppose, but that's certainly not in the spec. I'm not sure that we'll be well-served by making web-compat our goal on this. Chrome does weird things with shape-margin on insets that are not justified by the spec: https://jsfiddle.net/w4sobynp/1/. That jsfiddle has an inset singularity, though it works similarly with a small positive inset area. shape-margin should be inflating that into a round float area, not a rectangular one. So I'll rework the patch in favor of edges, not areas, and allow empty shapes to define a float area. I laid out a proposal in https://github.com/w3c/csswg-drafts/issues/2375#issuecomment-390324305. I don't know what I'll do exactly with insets, but I don't think we have to worry about web-compat with Chrome because their implementation is pretty indefensible.

(In reply to Brad Werth [:bradwerth] from comment #20) > > #test-shape { > > float: right; > > width: 200px; > > height: 200px; > > shape-margin: 50px; > > shape-outside: inset(100px 100px 125px 125px); > > } > > Obviously, disregarding shape-margin, the inset rect is empty. It's also in > some sense smaller than empty because its X() is more than its XMost() and > the same for Y() and YMost(). Let's say we want to define the float area by > expanding that rect by the shape-margin value of 50px. Where is the center > of the inverted rect? It could be at the center of the "inside-out" rect I > suppose, but that's certainly not in the spec. What our code does today when "over-deflating" a rect is determined by BaseRect::Deflate() at https://searchfox.org/mozilla-central/source/gfx/2d/BaseRect.h#292. Basically it deflates the top and the left of the rect, and then collapses it to a point. So in the example above, we'll have a point rect at (125px, 100px) which will then be inflated by shape-margin into a 50px radius circle centered at that point. Though we don't have an intuitive outcome for this case, I feel this is one of the more puzzling solutions. My patch will instead invert the over-deflated rect and then expand that. That seems like the closest analog to an inside-out polygon where we would still care about the edges of the shape.

Comment on attachment 8975990 [details] Bug 1461046 Part 1: Change EllipseShapeInfo to tolerate empty circles/ellipses and treat them as singular points/lines (possibly expanded by shape-margin). https://reviewboard.mozilla.org/r/244200/#review252390 This seems fine, if we're going with astearns "option 2" from https://github.com/w3c/csswg-drafts/issues/2375#issuecomment-370053992 ::: layout/generic/nsFloatManager.cpp:732 (Diff revision 5) > return mCenter.y - mRadii.height - mShapeMargin; > } > nscoord BEnd() const override { > return mCenter.y + mRadii.height + mShapeMargin; > } > - bool IsEmpty() const override { > + bool IsEmpty() const override { return false; } This "return false" might merit a short comment to explain why this is correct. Maybe something like: // Even a degenerate ellipse has a single point, // and hence is non-empty for our purposes. ::: layout/generic/nsFloatManager.cpp:868 (Diff revision 5) > - bIsMoreThanEllipseBEnd) ? nscoord_MIN : > + // height. In that case we treat the bInAppUnits == 0 case as > + // as intercepting at the width of the ellipse. All other cases solve typo: "as as" (duplicated at end of one line & beginning of next) ::: layout/generic/nsFloatManager.cpp:874 (Diff revision 5) > + // as intercepting at the width of the ellipse. All other cases solve > + // the intersection mathematically. > + const int32_t iIntercept = > + (bIsInExpandedRegion || bIsMoreThanEllipseBEnd) ? nscoord_MIN : > iExpand + NSAppUnitsToIntPixels( > - XInterceptAtY(bInAppUnits, mRadii.width, mRadii.height), > + (mRadii.height || bInAppUnits) ? If you like, maybe s/mRadii.height/!!mRadii.height/, to make it more explicit that we're intentionally coercing a non-boolean-thing to a bool here (to check it for zeroness). ("!!foo" is shorthand for coerce-foo-to-bool)

Comment on attachment 8975991 [details] Bug 1461046 Part 2: Change ShapeUtils::ComputeInsetRect to return the inverse of a rect deflated more than its bounds can tolerate. https://reviewboard.mozilla.org/r/244202/#review252404 ::: layout/base/ShapeUtils.h:60 (Diff revision 5) > - // Compute the rect for an inset. > + // Compute the rect for an inset. If the inset amount is larger than > + // aRefBox itself, this will return a rect the same shape as the inverse > + // rect that would be created by insetting aRefBox by the inset amount. Notably, this seems to go *against* the current spec text: "A pair of insets in either dimension that add up to more than the used dimension (such as left and right insets of 75% apiece) define a shape enclosing no area" https://drafts.csswg.org/css-shapes-1/#supported-basic-shapes We might still want to do this anyway for consistency & with the expectation that the spec will change on this, but it'd definitely be worth calling out in a code-comment that this is implementing something other than what's (currently) specced. So: please add a comment to that effect, possibly linking to the spec chunk that we're known to violate and/or the github comment from astearns that you're matching in spirit here? ::: layout/base/ShapeUtils.cpp:132 (Diff revision 5) > nsRect insetRect(aRefBox); > - insetRect.Deflate(inset); > + insetRect.x += inset.left; > + insetRect.y += inset.top; > + insetRect.width -= inset.LeftRight(); > + insetRect.height -= inset.TopBottom(); > + > + // Invert left and right, if necessary. > + if (insetRect.width < 0) { > + insetRect.width *= -1; > + insetRect.x -= insetRect.width; > + } > + > + // Invert top and bottom, if necessary. > + if (insetRect.height < 0) { > + insetRect.height *= -1; > + insetRect.y -= insetRect.height; > + } > We are aiming to deprecate direct access to BaseRect member variables, and we've removed most of the usages, so let's not add more -- see bug 1424382 and friends. Could you rewrite this so that it uses BaseRect APIs instead? Maybe even just declaring nscoord x1 = aRefBox.X(); nscoord x2 = aRefBox.XMost(); nscoord y1 = aRefBox.Y(); nscoord y2 = aRefBox.YMost(); ...and then working with those, and then returning a nsRect generated based on those?

Comment on attachment 8976360 [details] Bug 1461046 Part 3: Change RoundedBoxShapeInfo to tolerate empty rects. https://reviewboard.mozilla.org/r/244526/#review252414 ::: layout/generic/nsFloatManager.cpp:1080 (Diff revision 5) > const nscoord aBEnd) const override; > nscoord LineRight(const nscoord aBStart, > const nscoord aBEnd) const override; > nscoord BStart() const override { return mRect.y; } > nscoord BEnd() const override { return mRect.YMost(); } > - bool IsEmpty() const override { return mRect.IsEmpty(); } > + bool IsEmpty() const override { return false; } As in part 1, it'd be useful to include a brief justification for why this is never considered empty.

Comment on attachment 8979769 [details] Bug 1461046 Part 4: Change PolygonShapeInfo to tolerate polygons with only 1 or 2 vertices. https://reviewboard.mozilla.org/r/245916/#review252730 ::: layout/generic/nsFloatManager.cpp:1333 (Diff revision 2) > - // If we're empty, then the float area stays empty, even with a positive > + // If we're empty, then there's nothing to apply a shape-margin against. > - // shape-margin. > if (mEmpty) { > return; Per note below about 0 vertices, I don't think this state (mEmpty==true) is a possible state for us to be in. ::: layout/generic/nsFloatManager.cpp:1651 (Diff revision 2) > - // Polygons with fewer than three vertices result in an empty area. > - // https://drafts.csswg.org/css-shapes/#funcdef-polygon > - if (mVertices.Length() < 3) { > - mEmpty = true; > + // Polygons with 0 vertices define an empty float area. If there is only > + // 1 vertex, the PolygonShapeInfo will act as a point. With 2 vertices, it > + // will act as a line. > + if (mVertices.Length() == 0) { > - return; > - } > - > - auto Determinant = [] (const nsPoint& aP0, const nsPoint& aP1) { > - // Returns the determinant of the 2x2 matrix [aP0 aP1]. > - // https://en.wikipedia.org/wiki/Determinant#2_.C3.97_2_matrices > - return aP0.x * aP1.y - aP0.y * aP1.x; > - }; > - > - // See if we have any vertices that are non-collinear with the first two. > - // (If a polygon's vertices are all collinear, it encloses no area.) > - bool isEntirelyCollinear = true; > - const nsPoint& p0 = mVertices[0]; > - const nsPoint& p1 = mVertices[1]; > - for (size_t i = 2; i < mVertices.Length(); ++i) { > - const nsPoint& p2 = mVertices[i]; > - > - // If the determinant of the matrix formed by two points is 0, that > - // means they're collinear with respect to the origin. Here, if it's > - // nonzero, then p1 and p2 are non-collinear with respect to p0, i.e. > - // the three points are non-collinear. > - if (Determinant(p2 - p0, p1 - p0) != 0) { > - isEntirelyCollinear = false; > - break; > - } > - } > - > - if (isEntirelyCollinear) { > mEmpty = true; > return; Is it even possible for us to get a polygon with 0 vertices? It looks to me like that's forbidden (at the parser level) in the spec, and our parser indeed seems to forbid it. Specifically, the polygon(...) syntax has a "#" at the end of the coordinate-pair part of its syntax, here: https://drafts.csswg.org/css-shapes/#funcdef-polygon ...and that "#" is defined here as meaning "occurs one or more times": https://drafts.csswg.org/css-values/#mult-comma So I don't know that it's possible for us to get a polygon with 0 vertices. So, that means mEmpty==true doesn't seem like a coherent/possible state that's worthy of consideration here. Maybe IsEmpty() should just return true like it does for the other shapes in this patch stack? (BTW, I tried the following quick testcase just to be sure our parser rejected empty polygon expressions -- the style is indeed shown as crossed-out in devtools: data:text/html,<div style="shape-outside:polygon()">Inspect me )

Comment on attachment 8980126 [details] Bug 1461046 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. https://reviewboard.mozilla.org/r/246286/#review252736 ::: layout/reftests/css-shapes/reftest.list:3 (Diff revision 1) > +== shape-outside-empty-circle-1.html shape-outside-empty-point.ref.html > +== shape-outside-empty-circle-2.html shape-outside-empty-circle.ref.html s/.ref/-ref/ (period --> hyphen), throughout this file (It looks like the files themselves are named correctly, but they're just misspelled in this reftest.list file) ::: layout/reftests/css-shapes/shape-outside-empty-point-ref.html:24 (Diff revision 1) > +bibendum turpis at mi dapibus dictum. > +<span style="margin-left: 100px">Donec id lorem arcu.</span> Pellentesque > +tortor nunc, semper a dui vel, maximus varius orci. Maecenas posuere > +enim in tempor imperdiet.</p> I don't think this reliably renders how you want it to... I suspect you're intending to mimic a float at this point in the text, and this might be where the float ends up in the text on your system, but the precise piece of text will depend on font metrics, linewrapping, etc, all of which are system-dependent. Here's how this reference case looks on my machine, for example (with this margin-left just being dropped into the middle of a line of text, probably not where you want it): https://screenshots.firefox.com/xxmcMzN8XGyxH1mb/null I think you could get this to work predictably if you use "Ahem" as the font, or use fixed-size inline-blocks (with a colorful background/border) instead of text altogether. You might also want to manually force linebreaks after each character (or inline-block) with <br>, so that you don't need as much text here. (That might make things a bit more compact & predictable, but I'm not sure.) ::: layout/reftests/css-shapes/shape-outside-empty-polygon-2.html:4 (Diff revision 1) > +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> > +<title>Shape-outside polygon with 1 vertex, with shape-margin acts like a circle (with some aberration)</title> The "like a circle (with some abberation)" and corresponding line in reftest.list needs a bit of explanation I think... Specifically: (1) If it's close but not perfect, that's fine, but it'd be worth hinting at what's not right (and creating a "close" reference case if that's not prohibitively hard -- maybe using fuzzy()? (2) Right now you're using "!= [testcase] [reference case]", but I think you should really be using "fails == [testcase] [reference case]". I think you mean to express "These things *should* render the same, though currently they do not." ::: layout/reftests/css-shapes/shape-outside-empty-polygon-4.html:5 (Diff revision 1) > +<!DOCTYPE html> > +<html> > +<head> > +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> > +<title>Shape-outside polygon with 2 vertices, with shape-margin acts like a line</title> Is "acts like a line" correct here? I'd think that under the model we're going with here, the shape-margin should give it thickness, and make it act more like a rectangle with rounded ends... (This question applies to shape-outside-empty-polygon-3.html as well which has the same <title>.) ::: layout/reftests/css-shapes/shape-outside-empty-polygon-4.html:18 (Diff revision 1) > + shape-outside: polygon(100px 0px, 100px 200px); > + shape-margin: 90px; mis-indentation here (tab characters I think?)

Comment on attachment 8975991 [details] Bug 1461046 Part 2: Change ShapeUtils::ComputeInsetRect to return the inverse of a rect deflated more than its bounds can tolerate. https://reviewboard.mozilla.org/r/244202/#review252946

Comment on attachment 8979769 [details] Bug 1461046 Part 4: Change PolygonShapeInfo to tolerate polygons with only 1 or 2 vertices. https://reviewboard.mozilla.org/r/245916/#review252950

Comment on attachment 8980126 [details] Bug 1461046 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. https://reviewboard.mozilla.org/r/246286/#review252952 I'm not reviewing too thoroughly since it looks like some of these tests failed on Try & might need a bit more tweaking, but here's what I've got from a quick skim: ::: layout/reftests/css-shapes/shape-outside-empty-circle-1.html:16 (Diff revision 3) > + width: 100px; > + height: calc(200px / 9); "calc(200px / 9)" feels a bit odd & fragile -- and notably, it's not precisely representable with nscoord units. (200px / 9 * 60nscoord/px = 1333.333333 nscoord units, which will round down to 1333 nscoord units and might cause a bit of rounding error, though realistically with 9 boxes there'll only be ~3 nscoord units of error (0.33333*9) which might not be enough to make a visual difference since it's only 1/20th of a pixel) You might consider increasing the height of .shape (and the numerator in this .calc expression) to, say, 360px instead of 200px, so that we don't have to care about fractional pixels and/or nscoords... And then box would just be 40px tall (360px/9). ::: layout/reftests/css-shapes/shape-outside-empty-circle-1.html:30 (Diff revision 3) > +</style> > +</head> > +<body> > +<div class="container"> > +<div class="shape"></div> > +<box></box><br/> Not ticking "open an issue" since nothing needs to change, but for future reference, you don't need the trailing slash in your br tags. "<br>" is a self-closing tag & is correctly spelled "<br>" in HTML. (If this were XHTML, we'd need the slash to make it valid XML, of course.) Having said that, I think the slash is harmless so this is fine as-is, too.

This is finally passing tests and is ready for final review. Because this patch now goes against the current CSS Shapes 1 specification, there are lots of changes to tests. Here's a summary: Part 5: These local tests (not submitted as WPT reftests) codify our approach to zero-area shapes, and establishes these principles: a) Zero-area shapes still have edges and those edges impact float layout. b) Zero-area shapes can be expanded by shape-margin values. c) Zero-area shapes will not affect floated elements that are block adjacent -- there is no minimum of 1 pixel area applied to the shape. d) Insets deflated beyond a point singularity are treated as rectangles in the same shape as the inverted rectangle. Part 6: This part withdraws all our submitted WPT reftests that rely on the as-written treatment of "empty float areas". It also withdraws two of our tests that treated empty horizontal spurs on polygons as not contributing to the polygon shape -- something not in the spec. Part 7: This marks as failing an existing spec-example WPT test based on Example 3 in https://www.w3.org/TR/css-shapes-1/#relation-to-box-model-and-float-behavior.

Comment on attachment 8980787 [details] Bug 1461046 Part 6: Remove submitted WPT reftests that checked for empty float areas (which are no longer empty), or relied on ignoring horizontal spurs in polygons. https://reviewboard.mozilla.org/r/246964/#review254252

Comment on attachment 8980788 [details] Bug 1461046 Part 7: Mark an existing WPT reftest that checks empty float areas as failing. https://reviewboard.mozilla.org/r/246966/#review254254 ::: commit-message-fd992:1 (Diff revision 5) > +Bug 1461046 Part 7: Mark existing WPT reftests that check empty float areas as failing. > + Nit: Looks like this is only affecting one (singular) test, so maybe adjust pluralization accordingly: s/reftests/reftest/, s/that check/that checks/

Comment on attachment 8980126 [details] Bug 1461046 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. MozReview got confused. Would you please confirm your review?

Comment on attachment 8980126 [details] Bug 1461046 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. https://reviewboard.mozilla.org/r/246286/#review254256 Thank you for these comprehensive tests! ::: layout/reftests/css-shapes/reftest.list:26 (Diff revision 7) > +== shape-outside-empty-inset-6.html shape-outside-empty-line-ref.html > +== shape-outside-empty-inset-7.html shape-outside-empty-nothing-ref.html > +== shape-outside-empty-inset-8.html shape-outside-empty-nothing-ref.html > + > +== shape-outside-empty-polygon-1.html shape-outside-empty-point-ref.html > +fuzzy(255,520) == shape-outside-empty-polygon-2.html shape-outside-empty-circle-ref.html Could you add an explanation for the fuzziness ("abberation") here in a #-comment within the manifest file, for this line & the similar one for ellipses? Right now, "fuzzy(255,520)" seems like an awkwardly extreme level of "fuzziness" to have unexplained here. (It's saying 520 pixels can be entirely different between the two files, which feels pretty lenient, though maybe it's necessary. Seems worth explaining inline, at least. Is it just that we're using a Chamfer-matrix approximation here, and that's not a perfect match for a circle? That seems reasonable.) ::: layout/reftests/css-shapes/shape-outside-empty-ellipse-6.html:5 (Diff revision 7) > +<!DOCTYPE html> > +<html> > +<head> > +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> > +<title>Shape-outside empty vertical ellipse, with shape-margin acts like a line with rounded endpoints</title> Maybe put "with rounded endpoints" in parenthesis here, since AFAIK the rounded endpoints aren't part of what you're actually testing here. (reftest.list just asserts that this file renders like shape-outside-empty-line-ref.html -- it doesn't test for any roundedness. I'm guessing the rounded endpoints are outside of the float impact area, maybe?) ::: layout/reftests/css-shapes/shape-outside-empty-inset-6.html:5 (Diff revision 7) > +<!DOCTYPE html> > +<html> > +<head> > +<meta http-equiv="content-type" content="text/html; charset=UTF-8"> > +<title>Shape-outside inside-out vertical flat inset, acts like a line with rounded endpoints</title> As noted above for the ellipse test: maybe put "with rounded endpoints" in parenthesis here, since AFAIK the rounded endpoints aren't part of what you're actually testing here.

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e990d4d07b0e Part 1: Change EllipseShapeInfo to tolerate empty circles/ellipses and treat them as singular points/lines (possibly expanded by shape-margin). r=dholbert https://hg.mozilla.org/integration/autoland/rev/157bbc74460a Part 2: Change ShapeUtils::ComputeInsetRect to return the inverse of a rect deflated more than its bounds can tolerate. r=dholbert https://hg.mozilla.org/integration/autoland/rev/5c8648bcf6bb Part 3: Change RoundedBoxShapeInfo to tolerate empty rects. r=dholbert https://hg.mozilla.org/integration/autoland/rev/48057a6ba3d6 Part 4: Change PolygonShapeInfo to tolerate polygons with only 1 or 2 vertices. r=dholbert https://hg.mozilla.org/integration/autoland/rev/fbfe1d5b94e1 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. r=dholbert https://hg.mozilla.org/integration/autoland/rev/8e0c340b9700 Part 6: Remove submitted WPT reftests that checked for empty float areas (which are no longer empty), or relied on ignoring horizontal spurs in polygons. r=dholbert https://hg.mozilla.org/integration/autoland/rev/f5990eb1eb0d Part 7: Mark an existing WPT reftest that checks empty float areas as failing. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/e990d4d07b0e https://hg.mozilla.org/mozilla-central/rev/157bbc74460a https://hg.mozilla.org/mozilla-central/rev/5c8648bcf6bb https://hg.mozilla.org/mozilla-central/rev/48057a6ba3d6 https://hg.mozilla.org/mozilla-central/rev/fbfe1d5b94e1 https://hg.mozilla.org/mozilla-central/rev/8e0c340b9700 https://hg.mozilla.org/mozilla-central/rev/f5990eb1eb0d

Upstreamed tests in https://github.com/web-platform-tests/wpt/pull/11283

Backed out 7 changesets (bug 1461046) for permafailing reftest on /reftests/css-invalid/select/select-disabled-fieldset-1.html a=backout Backout link: https://hg.mozilla.org/mozilla-central/rev/8c926373039374cd1a47d92215e9efb4d5557983 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f5990eb1eb0d2f89f082783a377785e95b40d541 Pushed the backout to Try and it was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=902ec9bcc5adf88c98521dc83bca77226dff40ce Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=181221411&repo=autoland&lineNumber=1022 Log snippet: [task 2018-05-31T21:40:30.320Z] 21:40:30 INFO - REFTEST TEST-START | http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-disabled-fieldset-1.html == http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-fieldset-ref.html [task 2018-05-31T21:40:30.320Z] 21:40:30 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-disabled-fieldset-1.html | 0 / 537 (0%) [task 2018-05-31T21:40:40.840Z] 21:40:40 INFO - REFTEST INFO | drawWindow flags = DRAWWINDOW_DRAW_CARET | DRAWWINDOW_DRAW_VIEW | DRAWWINDOW_USE_WIDGET_LAYERS; window size = 800,1118; test browser size = 800,1000 [task 2018-05-31T21:40:40.843Z] 21:40:40 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-fieldset-ref.html | 0 / 537 (0%) [task 2018-05-31T21:40:51.263Z] 21:40:51 INFO - REFTEST INFO | REFTEST fuzzy test (0, 0) <= (9, 1) <= (8, 1) [task 2018-05-31T21:40:51.265Z] 21:40:51 INFO - REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-disabled-fieldset-1.html == http://10.0.2.2:8888/tests/layout/reftests/css-invalid/select/select-fieldset-ref.html | image comparison, max difference: 9, number of differing pixels: 1

The test /reftests/css-invalid/select/select-disabled-fieldset-1.html appears to be unrelated to changes in this patch. There's no use of the shape-outside or clip-path CSS properties, and all the changes here only affect the handling of those two properties. Is this a new intermittent orange?

Ah, I see that whatever it is, it's not intermittent. I'll double check that this isn't hitting any of the changed code.

With or without the patch applied, I can't get the test to hit any of the functions modified by this patch. I note that the test has an existing fuzzy-if(8,1) on it. It looks like this test will pass if it moves to fuzzy-if(9,1). I'll add that to this patch.

Comment on attachment 8983079 [details] Bug 1461046 Part 8: Update reftest fuzzy expectations for a seemingly-unrelated Android test. https://reviewboard.mozilla.org/r/248932/#review255128

(In reply to Brad Werth [:bradwerth] from comment #101) > The test /reftests/css-invalid/select/select-disabled-fieldset-1.html > appears to be unrelated to changes in this patch. (This push could've still "caused" the fuzzy reftest-failure by simply causing reftests to be rebucketed. If the test has some unstable dependency on tests that get run before it, or on being early/late in its bucket, then any push that introduces a new reftest could hypothetically cause breakage.)

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9619991fb38d Part 1: Change EllipseShapeInfo to tolerate empty circles/ellipses and treat them as singular points/lines (possibly expanded by shape-margin). r=dholbert https://hg.mozilla.org/integration/autoland/rev/3ebfdd60f59f Part 2: Change ShapeUtils::ComputeInsetRect to return the inverse of a rect deflated more than its bounds can tolerate. r=dholbert https://hg.mozilla.org/integration/autoland/rev/b3375f24897b Part 3: Change RoundedBoxShapeInfo to tolerate empty rects. r=dholbert https://hg.mozilla.org/integration/autoland/rev/128f8e1b8dcd Part 4: Change PolygonShapeInfo to tolerate polygons with only 1 or 2 vertices. r=dholbert https://hg.mozilla.org/integration/autoland/rev/b818a0c11897 Part 5: Add reftests that verify empty shapes still contribute their edges to float areas. r=dholbert https://hg.mozilla.org/integration/autoland/rev/1975410c6195 Part 6: Remove submitted WPT reftests that checked for empty float areas (which are no longer empty), or relied on ignoring horizontal spurs in polygons. r=dholbert https://hg.mozilla.org/integration/autoland/rev/3056b4258866 Part 7: Mark an existing WPT reftest that checks empty float areas as failing. r=dholbert https://hg.mozilla.org/integration/autoland/rev/bd46d28842ff Part 8: Update reftest fuzzy expectations for a seemingly-unrelated Android test. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/9619991fb38d https://hg.mozilla.org/mozilla-central/rev/3ebfdd60f59f https://hg.mozilla.org/mozilla-central/rev/b3375f24897b https://hg.mozilla.org/mozilla-central/rev/128f8e1b8dcd https://hg.mozilla.org/mozilla-central/rev/b818a0c11897 https://hg.mozilla.org/mozilla-central/rev/1975410c6195 https://hg.mozilla.org/mozilla-central/rev/3056b4258866 https://hg.mozilla.org/mozilla-central/rev/bd46d28842ff

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11771 for changes under testing/web-platform/tests

Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/11771 * continuous-integration/appveyor/pr

(In reply to Web Platform Test Sync Bot from comment #119) > Can't merge web-platform-tests PR due to failing upstream checks: > Github PR https://github.com/web-platform-tests/wpt/pull/11771 jgraham commented on (& closed) that PR saying "This seems to end up as a no-op", so I suspect that means everything is fine and we can disregard comment 119.

Upstream PR was closed without merging