Closed Bug 1326406 Opened 3 years ago Closed 3 years ago

Implement the rendering of basic shape ellipse() for CSS shape-outside

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

This bug is to implement shape-outside: ellipse() after bug 1311244 is done.
Two design choices that might worth mention.

1) We could further make CircleShapeInfo inherit from EllipseShapeInfo. Though it might fall into the "Circle-ellipse problem" [1] at the first glance. However all the ShapeInfo classes are immutable, so it's fine in practice. The upside is all the CircleShapeInfo methods except the constructor can be eliminated by using the EllipseShapeInfo's version. The con is we'll waste the memory space by using nsSize to store the radius of the circle() instead of using nscoord. I don't do this in my first patchset because the code isn't too complex yet.

2) I choose EllipseShapeInfo::mRadii to be nsSize instead of LogicalSize because the writing mode of ShapeInfo isn't changed after the shape is constructed. We don't actually need aWM to calculate EllipseShapeInfo::LineLeft(), etc.  If using LogicalSize, we need to add aWM argument to ShapeInfo's BStart() and BEnd() as well. Currently, the aWM argument is used only by BoxShapeInfo's LineLeft() [2] and LineRight(), and I feel it might be possible to remove the aWM argument if we could store border radii in BoxShapeInfo's constructor.

[1] https://en.wikipedia.org/wiki/Circle-ellipse_problem
[2] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/generic/nsFloatManager.cpp#549,559
David, could you please take a look at this patchset if they are not too big?
Flags: needinfo?(dbaron)
Comment on attachment 8828243 [details]
Bug 1326406 Part 1 - Fix indentation of </body> for the shapes1 reftests.

https://reviewboard.mozilla.org/r/105706/#review107206

Ship It!
Comment on attachment 8828243 [details]
Bug 1326406 Part 1 - Fix indentation of </body> for the shapes1 reftests.

https://reviewboard.mozilla.org/r/105706/#review107208
Attachment #8828243 - Flags: review+
Comment on attachment 8828244 [details]
Bug 1326406 Part 2 - Extract the computation of ellipse radii as ComputeEllipseRadii().

https://reviewboard.mozilla.org/r/105708/#review107210

::: layout/base/ShapeUtils.cpp:90
(Diff revision 1)
> +  const nsTArray<nsStyleCoord>& coords = aBasicShape->Coordinates();
> +  MOZ_ASSERT(coords.Length() == 2, "wrong number of arguments");
> +  nsSize radii;
> +
> +  if (coords[0].GetUnit() == eStyleUnit_Enumerated) {
> +    const auto styleShapeRadius = coords[0].GetEnumValue<StyleShapeRadius>();

Instead of calling this styleShapeRadius (which is just its type), call it radiusX.  (And maybe use StyleShapeRadius instead of auto?)

::: layout/base/ShapeUtils.cpp:98
(Diff revision 1)
> +  } else {
> +    radii.width = nsRuleNode::ComputeCoordPercentCalc(coords[0], aRefBox.width);
> +  }
> +
> +  if (coords[1].GetUnit() == eStyleUnit_Enumerated) {
> +    const auto styleShapeRadius = coords[1].GetEnumValue<StyleShapeRadius>();

same here, but radiusY
Attachment #8828244 - Flags: review+
Comment on attachment 8828245 [details]
Bug 1326406 Part 3 - Add ShapeInfo::Translate() for moving the origin of ShapeInfo.

https://reviewboard.mozilla.org/r/105710/#review107218
Attachment #8828245 - Flags: review+
Comment on attachment 8828246 [details]
Bug 1326406 Part 4 - Implement shape-outside: ellipse().

https://reviewboard.mozilla.org/r/105712/#review107220

I tend to think it probably would be a good idea to do a followup patch to make circle derive from ellipse (or even just use the ellipse class for circles).


I also didn't review the tests very carefully.

::: layout/generic/nsFloatManager.h:378
(Diff revision 1)
>      static nscoord XInterceptAtY(const nscoord aY, const nscoord aRadiusX,
>                                   const nscoord aRadiusY);
> +
> +    // Convert the coordinate space from physical to the logical space used
> +    // in nsFloatManager, which is the same as FloatInfo::mRect. The
> +    // returned (x, y) are in frame manager's line-axis and real block-axis,

I don't think "frame manager's line-axis" makes sense; I don't know what axes have to do with the frame manager.  Did you mean float manager?

(And what's the "real"?)

::: layout/generic/nsFloatManager.cpp:765
(Diff revision 1)
>      StyleBasicShape* const basicShape = shapeOutside.GetBasicShape();
>  
>      if (basicShape->GetShapeType() == StyleBasicShapeType::Circle) {
>        mShapeInfo =
>          MakeUnique<CircleShapeInfo>(basicShape, rect, aWM, aContainerSize);
> +    } else if (basicShape->GetShapeType() == StyleBasicShapeType::Ellipse) {

Maybe either (a) put basicShape->GetShapeType() in a variable or (b) use a switch() over it instead of an if-else chain.
Attachment #8828246 - Flags: review+
Flags: needinfo?(dbaron)
Comment on attachment 8828246 [details]
Bug 1326406 Part 4 - Implement shape-outside: ellipse().

https://reviewboard.mozilla.org/r/105712/#review107220

> I don't think "frame manager's line-axis" makes sense; I don't know what axes have to do with the frame manager.  Did you mean float manager?
> 
> (And what's the "real"?)

I blindly followed the document of the FloatInfo::mRect[1]. But yes, I think I mean float manager here, and I feel maybe we could use "float manager's coordinate space" to refer to the special coordinate space, i.e. the line-axis and block-axis combination, used in nsFloatManager. I'll add a followup patch to update the document about this.

The "real" block-axis should mean the same block-axis of the containting block's writing-mode. I'll drop it to avoid confusion.

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/generic/nsFloatManager.h#466-472
Addressed the review comments, and added two followup patches. Please help take a look.
Flags: needinfo?(dbaron)
I marked all the patches as review+, which means you can land after addressing the comments, and I don't need to look at them again.

Or is there something here that I hadn't reviewed before?  Patches 5 and 6, I suppose?  Anything else?  It's not clear to me why the review+ is gone.

(For things that might require re-review, I really prefer patches as attachments rather than MozReview requests.)
Flags: needinfo?(dbaron)
Comment on attachment 8829354 [details]
Bug 1326406 Part 5 - Update document about float manager's coordinate space.

https://reviewboard.mozilla.org/r/106478/#review108064

::: layout/generic/nsFloatManager.h:67
(Diff revision 1)
> + * nsFloatManager uses a special logical coordinate space that its inline
> + * coordinates are on the line-axis where its block coordinates are on the
> + * block-axis of the writing mode of the block formatting context. All the

s/that its inline coordinates are on/with inline coordinates on/
s/where its block coordinates are on/and block coordinates on/
s/of the writing mode/based on the writing mode/
Attachment #8829354 - Flags: review+
Comment on attachment 8829355 [details]
Bug 1326406 Part 6 - Make CircleShapeInfo inherit from EllipseShapeInfo.

https://reviewboard.mozilla.org/r/106480/#review108066

I suppose the constructors could share a little more code (e.g., with a common protected constructor in the ellipse class, and a helper method on each class to get an appropriate nsSize for the radii, so that the public constructors are just calls to the base class constructor along with a call to the appropriate helper method), but this is probably good enough.  Though I wouldn't mind a followup doing that.
Attachment #8829355 - Flags: review+
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #20)
> Or is there something here that I hadn't reviewed before?  Patches 5 and 6,
> I suppose?  Anything else?  It's not clear to me why the review+ is gone.

Yes, only patch 5 and 6 need the review. Thank you for the review.
(It seems the r+ flag in bugzilla will be cleared if the reviewer's name is not associated with the patch in mozreview. However, the patch is still autolandable in mozreview.)
Blocks: 1333685
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16c24b3a1e5c
Part 1 - Fix indentation of </body> for the shapes1 reftests. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/166f7bf0f407
Part 2 - Extract the computation of ellipse radii as ComputeEllipseRadii(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/1557fe7d766b
Part 3 - Add ShapeInfo::Translate() for moving the origin of ShapeInfo. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/4e7f8bffe949
Part 4 - Implement shape-outside: ellipse(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/3058735a4893
Part 5 - Update document about float manager's coordinate space. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/98dfda01dcba
Part 6 - Make CircleShapeInfo inherit from EllipseShapeInfo. r=dbaron
Depends on: 1334227
No longer depends on: 1334227
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfad3d738fed02f68223568f8ceb43df5a4e575
Rename (renumber) new mozilla-central-reftests shapes1 tests to avoid filename collisions with existing tests.  Followup to bug 1311244, bug 1326406, and bug 1326407.
You need to log in before you can comment on or make changes to this bug.