Closed Bug 1326407 Opened 3 years ago Closed 3 years ago

Implement the rendering of basic shape inset() 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

(9 files, 8 obsolete files)

5.68 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.45 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
2.70 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
3.31 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
3.65 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
4.60 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
12.20 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
6.74 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
55.09 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
This bug is for implementing shape-outside: inset().
Assignee: nobody → tlin
Status: NEW → ASSIGNED
MozReview-Commit-ID: 3XbfW40AJCI
Make the interface consistent with the ConvertPhysicalToLogical() method
added in the previous part.

MozReview-Commit-ID: 1YARDzI3cyr
The radii can be computed when creating BoxShapeInfo. No need to compute
them every time in the LineLeft() and LineRight().

MozReview-Commit-ID: GIDSLgickCT
The radii has been cached in the BoxShapeInfo in the previous part. Hence
the rename.

This class will be used to implement inset() in the next part, so the rect
stored doesn't necessary be the rect of the <shape-box>. It could be the
inset rectangle. Therefore I rename mShapeBoxRect to mRect to avoid any
confusion.

MozReview-Commit-ID: J0hpQDsbMyN
MozReview-Commit-ID: JZk1fo8SxgV
The reftests have passed
layout/reftests/w3c-css/submitted/check-for-references.sh.

MozReview-Commit-ID: JZk1fo8SxgV
Attachment #8835299 - Attachment is obsolete: true
David, the shape-outside: inset() implementation is ready for review. Would you please take a look at Part 1 through Part 8?
Flags: needinfo?(dbaron)
Comment on attachment 8835292 [details] [diff] [review]
Part 1 - Extract a function to compute inset rect. (v1)

>+/* static */ nsRect
>+ShapeUtils::ComputeInsetRect(StyleBasicShape* const aBasicShape,
>+                             const nsRect& aRefBox)
>+{
>+  const nsTArray<nsStyleCoord>& coords = aBasicShape->Coordinates();
>+  MOZ_ASSERT(coords.Length() == 4, "wrong number of arguments");

Perhaps also assert that the shape is the expected type?

> struct ShapeUtils final
> {
>   // Compute the length of a keyword <shape-radius>, i.e. closest-side or
>   // farthest-side, for a circle or an ellipse on a single dimension. The
>   // caller needs to call for both dimensions and combine the result.
>   // https://drafts.csswg.org/css-shapes/#typedef-shape-radius.
>-  //
>   // @return The length of the radius in app units.
>   static nscoord ComputeShapeRadius(const StyleShapeRadius aType,
>                                     const nscoord aCenter,
>                                     const nscoord aPosMin,
>                                     const nscoord aPosMax);
> 
>   // Compute the center of a circle or an ellipse.
>-  //
>   // @param aRefBox The reference box of the basic shape.
>   // @return The point of the center.

I prefer the style with the blank line; it seems more readable to me.  But I guess this is ok if you prefer without...
Attachment #8835292 - Flags: review+
Comment on attachment 8835295 [details] [diff] [review]
Part 4 - Extract a function to convert a rect to float manager's logical coordinate.

>+  , mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize))
> {
>   MOZ_COUNT_CTOR(nsFloatManager::FloatInfo);
> 
>+  mRect.MoveBy(aLineLeft, aBlockStart);

Probably better to just put this all in the constructor, as:

  mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize) +
        nsPoint(aLineLeft, aBlockStart))

r=dbaron with that
Attachment #8835295 - Flags: review+
Comment on attachment 8835296 [details] [diff] [review]
Part 5 - Rewrite ConvertPhysicalToLogical().

It's not clear to me why you want the caller to construct the LogicalPoint, but maybe that will become clear in later patches.

If not, maybe it's better to leave the construction of the LogicalPoint inside the function?
Attachment #8835296 - Flags: review+
Comment on attachment 8835297 [details] [diff] [review]
Part 6 - Cache the border radii in BoxShapeInfo.

nsFloatManager.h:


>+    BoxShapeInfo(const nsRect& aShapeBoxRect,
>+                 mozilla::UniquePtr<nscoord[]> aRadii)

The implications of a UniquePtr<T> parameter aren't immediately clear
to me, but is there a reason to prefer this over the more-common
UniquePtr<T>&& ?


I also wonder whether it would be worth creating a struct type containing
an nscoord[8] array to make this clearer and perhaps safer.


+    // in the float manager's coordinate space. If there's no radii, it's

"there's" -> "there are"


nsFloatManager.cpp:

>+    // This happens only if aWM is vertical-lr. We need to swap the top and
>+    // bottom corners.

I know you're only moving it, but maybe it's worth explaining this
comment a little more:  when IsLineInverted() is true, the relationship
reverses between which corner comes first going clockwise, and which
corner is block-start versus block-end.

r=dbaron with that
Attachment #8835297 - Flags: review+
Comment on attachment 8835298 [details] [diff] [review]
Part 7 - Rename BoxShapeInfo to RoundedBoxShapeInfo.

In the commit message:
>stored doesn't necessary be the rect of the <shape-box>. It could be the 

"doesn't necessary be" -> "isn't necessarily"

r=dbaron
Attachment #8835298 - Flags: review+
Comment on attachment 8835293 [details] [diff] [review]
Part 2 - Extract a function to compute inset radii.

Actually, one further comment here:

I think you should reverse the order of the aRefBox and aInsetRect parameters so that they match ComputeBorderRadii.
Comment on attachment 8835305 [details] [diff] [review]
Part 8 - Implement shape-outside: inset(). (v2)

>+  nsRect logicalInsetRect =
>+    ConvertToFloatLogical(LogicalRect(aWM, insetRect, aContainerSize),
>+                          aWM, aContainerSize);

Unless I missed something, this makes me think ConvertToFloatLogical
is still better-off constructing the LogicalRect inside the function.

r=dbaron
Flags: needinfo?(dbaron)
Attachment #8835305 - Flags: review+
> Perhaps also assert that the shape is the expected type?

Addedd assert in this patch. I'll also add some assertions to other
ShapeUtils functions in Part 0.

MozReview-Commit-ID: DmVOVArtzBZ
Attachment #8837448 - Flags: review+
Attachment #8835292 - Attachment is obsolete: true
Addressed comment 16: Reverse the order of the aRefBox and aInsetRect
parameters so that they match ComputeBorderRadii.

MozReview-Commit-ID: 3XbfW40AJCI
Attachment #8837458 - Flags: review+
Attachment #8835293 - Attachment is obsolete: true
>  mRect(ShapeInfo::ConvertToFloatLogical(aMarginRect, aWM, aContainerSize) +
>        nsPoint(aLineLeft, aBlockStart))

Addressed comment 12.

MozReview-Commit-ID: Ag6V6XmlHIU
Attachment #8837459 - Flags: review+
Attachment #8835295 - Attachment is obsolete: true
Re comment 13:
> It's not clear to me why you want the caller to construct the LogicalPoint,
> but maybe that will become clear in later patches.
>
> If not, maybe it's better to leave the construction of the LogicalPoint
> inside the function?

I was thinking about match the interface of ConvertPhysicalToLogical() added
in Part 4, but it's also good to leaves construction of the LogicalPoint
inside the function. As a result, this new patch becomes a simple rename and
reorder of the arguments.
Attachment #8837461 - Flags: review+
Attachment #8835296 - Attachment is obsolete: true
Addressed comment 14: correct the typo, and update comments.

> The implications of a UniquePtr<T> parameter aren't immediately clear
> to me, but is there a reason to prefer this over the more-common
> UniquePtr<T>&& ?

Yes, the two usage case have subtle differences. Using UniquePtr<T> in the
function parameter means the caller wants to transfer the object's ownership
to the callee, and callee *must* take the ownership anyway. On the other
hand, if using UniquePtr<T>&&, the callee have the choice *not* to take the
ownership, so the caller have to check whether the UniquePtr is empty
afterwards.

Here, we don't need the semantic of UniquePtr<T>&&. Using UniquePtr<T> is
simpler.

> I also wonder whether it would be worth creating a struct type containing
> an nscoord[8] array to make this clearer and perhaps safer.

This might worth doing. Filed bug 1339732.
Attachment #8837479 - Flags: review+
Attachment #8835297 - Attachment is obsolete: true
Attachment #8835298 - Attachment is obsolete: true
Attachment #8835305 - Attachment is obsolete: true
Re comment 17:

> Unless I missed something, this makes me think ConvertToFloatLogical
> is still better-off constructing the LogicalRect inside the function.

Actually, all the callers of ConvertToFloatLogical() except the one in Part 8
already have LogicalRect, not nsRect. That's why I choose the argument be
LogicalRect.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8bf524e3c8
Part 0 - Add assertion to ShapeUtils functions. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc715f72d61
Part 1 - Extract a function to compute inset rect. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9a5fad30b7
Part 2 - Extract a function to compute inset radii. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0af1a8bdcaf
Part 3 - Extract a function to compute <shape-box> rect. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/366e6522665c
Part 4 - Extract a function to convert a rect to float manager's logical coordinate. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/f45cac0c1b91
Part 5 - Rename ConvertPhysicalToLogical(). r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/0058687f7813
Part 6 - Cache the border radii in BoxShapeInfo. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a514a5851a4
Part 7 - Rename BoxShapeInfo to RoundedBoxShapeInfo. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c77acfaa6e
Part 8 - Implement shape-outside: inset(). r=dbaron
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.
I've documented this, by added an entry and footnote to the browser support table at 

https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside

I've not added a note to the release notes, as it is preffed off at the moment. I've checked that shape-outside is already covered in our Experimental features page:

https://developer.mozilla.org/en-US/Firefox/Experimental_features

Let me know if there's anything else you'd like me to do. Thanks!
You need to log in before you can comment on or make changes to this bug.