Closed
Bug 1311244
Opened 8 years ago
Closed 8 years ago
Implement the rendering of basic shape circle() for CSS shape-outside
Categories
(Core :: Layout: Floats, defect)
Core
Layout: Floats
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(9 files)
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chenpighead
:
review+
|
Details |
Once bug Bug 1309467 has landed, we could use the utility added to implement basic shape circle() and ellipse(). I imaging we could reuse the methods for clip-path [1] which converts CSS basic shape value into the metrics of ellipse that layout could consume.
[1] http://searchfox.org/mozilla-central/source/layout/svg/nsCSSClipPathInstance.cpp
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
The patch set implementing shape-outside: circle() is big enough. I'll do ellipse() in a separate bug.
Status: NEW → ASSIGNED
Summary: Implement the rendering of basic shape circle() and ellipse() for CSS shape-outside → Implement the rendering of basic shape circle() for CSS shape-outside
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8822653 [details]
Bug 1311244 Part 1 - Use nsPoint type for center in nsCSSClipPathInstance.
https://reviewboard.mozilla.org/r/101144/#review102062
Attachment #8822653 -
Flags: review?(dbaron) → review+
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8822654 [details]
Bug 1311244 Part 2 - Create ShapeUtils, and move EnumerationToLength into it.
https://reviewboard.mozilla.org/r/101146/#review102064
::: layout/generic/ShapeUtils.h:21
(Diff revision 1)
> + // 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.
> + // https://drafts.csswg.org/css-shapes/#typedef-shape-radius.
Maybe note that if it's for a circle, the caller has to call for both dimensions and combine the results appropriately?
::: layout/generic/ShapeUtils.h:25
(Diff revision 1)
> +{
> + // 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.
> + // https://drafts.csswg.org/css-shapes/#typedef-shape-radius.
> + //
> + // @return The length of the radius in app unit.
s/app unit/app units/
::: layout/generic/moz.build:115
(Diff revision 1)
>
> EXPORTS.mozilla += [
> 'CSSAlignUtils.h',
> 'ReflowInput.h',
> 'ReflowOutput.h',
> + 'ShapeUtils.h',
I would suggest putting the new file in layout/base, not layout/generic.
Attachment #8822654 -
Flags: review?(dbaron) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8822655 [details]
Bug 1311244 Part 3 - Extract the computation of center as ComputeCircleOrEllipseCenter().
https://reviewboard.mozilla.org/r/101148/#review102066
::: layout/generic/ShapeUtils.h:40
(Diff revision 1)
> + // Compute the center of a circle or an ellipse.
> + //
> + // @param aRefBox The reference box of the basic shape.
> + // @return The point of the center.
> + static nsPoint ComputeCircleOrEllipseCenter(
> + const StyleBasicShape& aBasicShape,
Please take a pointer rather than a reference, since that's what the callers are likely to have.
::: layout/generic/ShapeUtils.cpp:41
(Diff revision 1)
> +nsPoint
> +ShapeUtils::ComputeCircleOrEllipseCenter(const StyleBasicShape& aBasicShape,
> + const nsRect& aRefBox)
> +{
> + nsPoint topLeft, anchor;
> + nsSize size(aRefBox.width, aRefBox.height);
This should just be:
nsSize size(aRefBox.Size());
Attachment #8822655 -
Flags: review?(dbaron) → review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8822656 [details]
Bug 1311244 Part 4 - Extract the computation of circle radius as ComputeCircleRadius().
https://reviewboard.mozilla.org/r/101150/#review102068
::: layout/generic/ShapeUtils.h:48
(Diff revision 1)
> + // Compute the radius for a circle.
> + // @param aCenter the center of the circle.
> + // @param aRefBox the reference box of the circle.
> + // @return The length of the radius in app unit.
> + static nscoord ComputeCircleRadius(
> + const mozilla::StyleBasicShape& aBasicShape,
Again, a pointer rather than a reference here.
::: layout/generic/ShapeUtils.h:49
(Diff revision 1)
> + const nsPoint& aCenter,
> + const nsRect& aRefBox);
These two should be on the same line.
::: layout/generic/ShapeUtils.cpp:61
(Diff revision 1)
> + nscoord horizontal =
> + ShapeUtils::ComputeShapeRadius(styleShapeRadius, aCenter.x, aRefBox.x,
> + aRefBox.x + aRefBox.width);
> + nscoord vertical =
> + ShapeUtils::ComputeShapeRadius(styleShapeRadius, aCenter.y, aRefBox.y,
> + aRefBox.y + aRefBox.height);
Drop the "ShapeUtils::" here.
And, probably use aRefBox.XMost() / aRefBox.YMost() for the final parameter as well, rather than manually adding.
::: layout/generic/ShapeUtils.cpp:67
(Diff revision 1)
> + if (styleShapeRadius == StyleShapeRadius::FarthestSide) {
> + r = horizontal > vertical ? horizontal : vertical;
> + } else {
> + r = horizontal < vertical ? horizontal : vertical;
> + }
These should probably use std::max and std::min
Attachment #8822656 -
Flags: review?(dbaron) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8822657 [details]
Bug 1311244 Part 5 - Convert FloatInfo's copy constructor into a move constructor.
https://reviewboard.mozilla.org/r/101152/#review102070
The out-of-memory check was no longer meaningful since nsTArray has been changed to infallible allocation.
::: layout/generic/nsFloatManager.h:389
(Diff revision 1)
> const nscoord aBandBStart, const nscoord aBandBEnd);
>
> static nscoord XInterceptAtY(const nscoord aY, const nscoord aRadiusX,
> const nscoord aRadiusY);
>
> #ifdef NS_BUILD_REFCNT_LOGGING
Do these #ifdefs need to be removed if you're depending on this being a move constructor? Or are correct move constructors automatically generated? (And does having a UniquePtr member prevent a copy constructor from being automatically generated and compiling?)
Attachment #8822657 -
Flags: review?(dbaron) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8822658 [details]
Bug 1311244 Part 6 - Add ShapeInfo and move <shape-box> impl to BoxShapeInfo.
https://reviewboard.mozilla.org/r/101154/#review102078
There's a typo in the commit message: ShpapeInfo should be ShapeInfo.
::: layout/generic/nsFloatManager.h:437
(Diff revision 1)
> // either of the block which created the frame manager or the block
> // that is calling the frame manager. The inline coordinates are in
> // the line-relative axis of the frame manager and its block
> // coordinates are in the frame manager's real block direction.
> nsRect mRect;
> - // This is the reference box of css shape-outside if specified, which
> + // Pointer to a concrete subclass of ShapeInfo.
Presumably "or null, which means that there is no shape-outside"?
::: layout/generic/nsFloatManager.cpp:663
(Diff revision 1)
> - mShapeBoxRect.emplace(rect.LineLeft(aWM, aContainerSize) + aLineLeft,
> + if (shapeOutside.GetType() == StyleShapeSourceType::Box) {
> + nsRect shapeBoxRect(rect.LineLeft(aWM, aContainerSize) + aLineLeft,
> - rect.BStart(aWM) + aBlockStart,
> + rect.BStart(aWM) + aBlockStart,
> - rect.ISize(aWM), rect.BSize(aWM));
> + rect.ISize(aWM), rect.BSize(aWM));
> + mShapeInfo = MakeUnique<BoxShapeInfo>(shapeBoxRect, mFrame);
> }
Maybe add an else with an assertion here to ensure there aren't unhandled cases?
Attachment #8822658 -
Flags: review?(dbaron) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review102084
Leaving this as r? for now since I had a bunch of questions about the emptiness tests and the state of the spec and other implementations. But otherwise I think it looks good (with the comments addressed).
Also, please run layout/reftests/w3c-css/submitted/check-for-references.sh to check the reference links in the tests.
(I didn't review the tests very carefully.)
::: layout/generic/nsFloatManager.h:420
(Diff revision 1)
> + nscoord LineRight(mozilla::WritingMode aWM,
> + const nscoord aBStart,
> + const nscoord aBEnd) const override;
> + nscoord BStart() const override { return mCenter.y - mRadius; }
> + nscoord BEnd() const override { return mCenter.y + mRadius; }
> + bool IsEmpty() const override { return mRadius == 0; };
Does this match the spec? In particular, if a shape is a circle with a radius of 0, should text flow around that point, or should it flow around nothing?
If this isn't clear from the spec, please file a spec issue.
::: layout/generic/nsFloatManager.h:423
(Diff revision 1)
> + nscoord BStart() const override { return mCenter.y - mRadius; }
> + nscoord BEnd() const override { return mCenter.y + mRadius; }
> + bool IsEmpty() const override { return mRadius == 0; };
> +
> + private:
> + // The position of the center of the circle. The coordinate is the same
probably s/coordinate/coordinate system/ or s/coordinate/coordinate space/.
::: layout/generic/nsFloatManager.h:426
(Diff revision 1)
> +
> + private:
> + // The position of the center of the circle. The coordinate is the same
> + // as FloatInfo::mRect.
> + nsPoint mCenter;
> + // The radius of the circle in app unit.
s/app unit/app units/
::: layout/generic/nsFloatManager.cpp:191
(Diff revision 1)
> - if (fi.IsEmpty()) {
> + if (fi.IsEmpty(aShapeType)) {
> // For compatibility, ignore floats with empty rects, even though it
> // disagrees with the spec. (We might want to fix this in the
> // future, though.)
> continue;
> }
Given the comment here about disagreeing with the spec, I'm not sure we want to do *more* ignoring than we used to. What do other implementations do? And does this still not match the spec? (If it agrees with all implementations and still doesn't match the spec, we should get the spec corrected and figure out whether we should be adjusting this for shapes.)
::: layout/generic/nsFloatManager.cpp:627
(Diff revision 1)
> + nscoord aBlockStart,
> + const LogicalRect& aShapeBoxRect,
> + WritingMode aWM,
> + const nsSize& aContainerSize)
> +{
> + // Use physical coordinate to compute center of circle() since the
s/coordinate/coordinates/
s/compute center of circle()/compute the center of the circle()/
::: layout/generic/nsFloatManager.cpp:638
(Diff revision 1)
> + // Convert the coordinate back to the same as FloatInfo::mRect.
> + if (aWM.IsVertical()) {
> + mCenter.x = aWM.IsSideways() ? aContainerSize.height - physicalCenter.y
> + : physicalCenter.y;
> + mCenter.y = aWM.IsVerticalLR() ? physicalCenter.x
> + : aContainerSize.width - physicalCenter.x;
> + } else {
> + mCenter = physicalCenter;
> + }
> + mCenter.MoveBy(aLineLeft, aBlockStart);
This makes it seem like:
- mCenter should be mozilla::LogicalPoint
- this should be using the standard logical coordinate conversion methods rather than rolling your own
- the uses below of mCenter.x and .y should use its I() and B() methods
::: layout/generic/nsFloatManager.cpp:713
(Diff revision 1)
> - MOZ_ASSERT_UNREACHABLE("Why don't we have a shape-box?");
> + MOZ_ASSERT(shapeOutside.GetType() != StyleShapeSourceType::Box,
> + "Box source type must have <shape-box> specified!");
Shouldn't this entire switch just move into the if (... == StyleShapeSourceType::Box) below? Or are there other cases it will be needed for?
::: layout/generic/nsFloatManager.cpp:724
(Diff revision 1)
> nsRect shapeBoxRect(rect.LineLeft(aWM, aContainerSize) + aLineLeft,
> rect.BStart(aWM) + aBlockStart,
> rect.ISize(aWM), rect.BSize(aWM));
> mShapeInfo = MakeUnique<BoxShapeInfo>(shapeBoxRect, mFrame);
> + } else if (shapeOutside.GetType() == StyleShapeSourceType::Shape) {
> + const StyleBasicShape& basicShape = *shapeOutside.GetBasicShape();
no need to dereference if you make the type the constructor takes consistent with what callers have
::: layout/generic/nsFloatManager.cpp:764
(Diff revision 1)
>
> MOZ_ASSERT(aShapeType == ShapeType::ShapeOutside);
> if (!mShapeInfo) {
> return LineLeft();
> }
> - return mShapeInfo->LineLeft(aWM, aBStart, aBEnd);
> + // Clip the flow area to the margin-box.
Perhaps cite:
because https://drafts.csswg.org/css-shapes-1/#relation-to-box-model-and-float-behavior says "When a shape is used to define a float area, the shape is clipped to the float’s margin box."
(and then "(see above)" for the other 3 occurrences)
It would also be good to have a test for this for 'shape-outside: border-box' combined with negative margins.
::: layout/reftests/w3c-css/submitted/shapes1/shape-outside-circle-001.html:49
(Diff revision 1)
> + <div class="shape"></div>
> + <div class="shape"></div>
> + <div class="box" style="height: 36px;"></div>
> + <div class="box" style="height: 12px;"></div> <!-- Box at corner -->
> + <div class="box" style="height: 12px;"></div> <!-- Box at corner -->
> + <div class="long box" style="height: 60px;"></div> <!-- Saturate the space between two floats -->
I don't think "Saturate" makes sense in this context. Did you mean "Fill"?
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #15)
> Leaving this as r? for now since I had a bunch of questions about the
> emptiness tests and the state of the spec and other implementations. But
> otherwise I think it looks good (with the comments addressed).
Please needinfo? me when you want me to look at this again.
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8822657 [details]
Bug 1311244 Part 5 - Convert FloatInfo's copy constructor into a move constructor.
https://reviewboard.mozilla.org/r/101152/#review102070
> Do these #ifdefs need to be removed if you're depending on this being a move constructor? Or are correct move constructors automatically generated? (And does having a UniquePtr member prevent a copy constructor from being automatically generated and compiling?)
> Do these #ifdefs need to be removed if you're depending on this being a move constructor? Or are correct move constructors automatically generated?
I think it's fine to leave #ifdefs untouched. When NS_BUILD_REFCNT_LOGGING is not defined, the implicit move constructor will be generated per [1] since only the FloatInfo's constructor is defined.
> And does having a UniquePtr member prevent a copy constructor from being automatically generated and compiling?
Yes, a UniquePtr member prevent the copy constructor from being compiling. UniquePtr does not allow copy operations [2] since it uniquely owns the resource. Thus the class having a UniquePtr member must support move operations.
[1] http://en.cppreference.com/w/cpp/language/move_constructor
[2] http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/mfbt/UniquePtr.h#352-353
Assignee | ||
Comment 18•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8822658 [details]
Bug 1311244 Part 6 - Add ShapeInfo and move <shape-box> impl to BoxShapeInfo.
https://reviewboard.mozilla.org/r/101154/#review102078
> Maybe add an else with an assertion here to ensure there aren't unhandled cases?
Since Part 7 will add a new if-else for ShapeType, I'll list all StyleShapeSourceType as well as an else with an assertion in Part 7.
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review102084
> Also, please run layout/reftests/w3c-css/submitted/check-for-references.sh to check the reference links in the tests.
Thanks for the reminder. I've run check-for-references.sh. No error.
> Does this match the spec? In particular, if a shape is a circle with a radius of 0, should text flow around that point, or should it flow around nothing?
>
> If this isn't clear from the spec, please file a spec issue.
The spec defines "empty float area" in https://drafts.csswg.org/css-shapes/#empty-float-area, but it doesn't explicit says "a circle with a radius of 0 defines an empty area". It did have an example for inset() [2] that defines an empty area though. I've filed https://github.com/w3c/csswg-drafts/issues/850
Both Chrome and Safari behave the same on this case. The text flow around nothing. (I have a test `shape-outside-circle-008.html` for circle(0%).)
> probably s/coordinate/coordinate system/ or s/coordinate/coordinate space/.
The document of GetTranslation() and Translate() use "coordinate space", so I'll use that for consistency.
> Given the comment here about disagreeing with the spec, I'm not sure we want to do *more* ignoring than we used to. What do other implementations do? And does this still not match the spec? (If it agrees with all implementations and still doesn't match the spec, we should get the spec corrected and figure out whether we should be adjusting this for shapes.)
See my explantion above for other implementations do. It's clear that an empty float should be ignored per CSS shapes spec. However, "the spec" this comment refers to should be the CSS2 spec, but I cannot find anything regarding empty float area in CSS2 spec. Perhaps I miss something.
> This makes it seem like:
> - mCenter should be mozilla::LogicalPoint
> - this should be using the standard logical coordinate conversion methods rather than rolling your own
> - the uses below of mCenter.x and .y should use its I() and B() methods
To convert mCenter to the coordinate space used in nsFloatManager, mCenter.x needs to be in the line-axis rather than in the normal inline-axis. So we cannot use LogicalPoint's I() here for mCenter.x. (That's why mRect.x is defined by LineLeft() [1].
Instead of rolling my own, perhaps it's better to define an L() method for LogicalPoint.
[1] http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/layout/generic/nsFloatManager.cpp#542
> Shouldn't this entire switch just move into the if (... == StyleShapeSourceType::Box) below? Or are there other cases it will be needed for?
Yes. Basic shapes like circle or ellipse also need reference box |rect| to compute their center, so it's better not to move the switch statement.
> Perhaps cite:
>
> because https://drafts.csswg.org/css-shapes-1/#relation-to-box-model-and-float-behavior says "When a shape is used to define a float area, the shape is clipped to the float’s margin box."
>
> (and then "(see above)" for the other 3 occurrences)
>
>
> It would also be good to have a test for this for 'shape-outside: border-box' combined with negative margins.
Filed a followup bug 1328236 for adding a test.
> I don't think "Saturate" makes sense in this context. Did you mean "Fill"?
Yes. I mean "fill the empty space". I've replaced "Saturate" by "Fill" in the tests.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•8 years ago
|
||
David, I've addressed all the review comments. Please review part 7 again and the new part 0 for fixing a build error.
Assignee | ||
Updated•8 years ago
|
status-firefox52:
affected → ---
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8823247 [details]
Bug 1311244 Part 0 - Preemptively fix unified build bustage in nsLayoutUtils.
https://reviewboard.mozilla.org/r/101808/#review103120
Attachment #8823247 -
Flags: review?(dbaron) → review+
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review103126
::: layout/generic/WritingModes.h:791
(Diff revision 2)
> }
> + nscoord L(WritingMode aWritingMode,
> + const nsSize& aContainerSize) const // line-axis
> + {
> + CHECK_WRITING_MODE(aWritingMode);
> + if (aWritingMode.IsBidiLTR()) {
Why is this IsBidiLTR() rather than IsInlineReversed()?
(They should be != when 'writing-mode' is 'sideways-lr'. Which is true and which is false will depend on 'direction'. See WritingMode::InitFromStyleVisibility.)
You should probably actually test what happens rather than relying on logic for which is correct. :-)
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review103128
I shouldn't have let this stay in MozReview, since doing re-reviews in MozReview is horrible, but anyway:
(See also the previous comment.)
::: layout/generic/WritingModes.h:787
(Diff revision 2)
> nscoord B(WritingMode aWritingMode) const // block-axis
> {
> CHECK_WRITING_MODE(aWritingMode);
> return mPoint.y;
> }
> + nscoord L(WritingMode aWritingMode,
L() seems like a rather cryptic name here. Maybe this should be line-relative?
::: layout/generic/nsFloatManager.cpp:643
(Diff revision 2)
> + // Convert the coordinate space back to the same as FloatInfo::mRect.
> + // mCenter.x is in the line-axis of the frame manager and mCenter.y are in
> + // the frame manager's real block-axis.
> + LogicalPoint logicalCenter(aWM, physicalCenter, aContainerSize);
> + mCenter = nsPoint(logicalCenter.L(aWM, aContainerSize) + aLineLeft,
> + logicalCenter.B(aWM) + aBlockStart);
So if you needed to introduce L(), why isn't use of B() problematic for 'writing-mode: vertical-rl', where the block progression is to the negative direction?
(Maybe you should instead have ignored by advice on this issue from the last comment?)
Again, probably worth actually testing what happens here.
Attachment #8822659 -
Flags: review?(dbaron) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review103126
> Why is this IsBidiLTR() rather than IsInlineReversed()?
>
> (They should be != when 'writing-mode' is 'sideways-lr'. Which is true and which is false will depend on 'direction'. See WritingMode::InitFromStyleVisibility.)
>
> You should probably actually test what happens rather than relying on logic for which is correct. :-)
LineRelative() is implemented by converting from I() rather than from the physical axis. The physical-axis to logical-axis conversion is done in LogicalPoint's constructor (if constructing from physical point). Therefore, in LineRelative() all we need to do is convert all the case where inline-axis and line-axis progression disagrees, which happens to be `!IsBidiLTR()` according to https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical
For example, when 'writing-mode' is 'sideways-lr' and 'direction' is 'ltr', we don't need to do anything because both inline and line direction are from 'bottom' to 'top'.
I have shape-outside-circle-021.html and shape-outside-circle-022.html which test 'sideways-lr' with 'direction: ltr' and 'direction: rtl'. Changing the line to `!IsInlineReversed()` will fail the two tests. :)
Assignee | ||
Comment 41•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8822659 [details]
Bug 1311244 Part 7 - Implement shape-outside: circle().
https://reviewboard.mozilla.org/r/101156/#review103128
> L() seems like a rather cryptic name here. Maybe this should be line-relative?
Sounds good. Change the method name to LineRelative().
> So if you needed to introduce L(), why isn't use of B() problematic for 'writing-mode: vertical-rl', where the block progression is to the negative direction?
>
> (Maybe you should instead have ignored by advice on this issue from the last comment?)
>
> Again, probably worth actually testing what happens here.
I think it's because the coordinate space of nsFloatManager uses real block-axis, so block progression of the physical-axis doesn't matter.
From shape-outside-circle-017.html to shape-outside-circle-024.html (8 of them) test all the combination of 'writing-mode' and 'direction'. They should have a decent test coverage.
Comment 42•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ee4ea83a6b4
Part 0 - Preemptively fix unified build bustage in nsLayoutUtils. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/f36cd1532fdb
Part 1 - Use nsPoint type for center in nsCSSClipPathInstance. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/20148e33d523
Part 2 - Create ShapeUtils, and move EnumerationToLength into it. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/2c33905ccb04
Part 3 - Extract the computation of center as ComputeCircleOrEllipseCenter(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/9d257713833a
Part 4 - Extract the computation of circle radius as ComputeCircleRadius(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/96988ec5b81c
Part 5 - Convert FloatInfo's copy constructor into a move constructor. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/ff9c71e1dbc8
Part 6 - Add ShapeInfo and move <shape-box> impl to BoxShapeInfo. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/d5411799a28f
Part 7 - Implement shape-outside: circle(). r=dbaron
Comment 43•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/68c4261192ac for giving Win7 debug R1 a 20% chance of hitting either https://treeherder.mozilla.org/logviewer.html#?job_id=67099842&repo=autoland or https://treeherder.mozilla.org/logviewer.html#?job_id=67149762&repo=autoland or https://treeherder.mozilla.org/logviewer.html#?job_id=67149764&repo=autoland
Those are almost certainly all the same thing, and the same thing as bug 1300355, that on Win7 we are right on the edge of gfx giving up the ghost and failing to keep drawing during Win7 reftests. No idea whether you actually made gfx's life more difficult, or just shuffled around the contents of the chunks of reftests so that R1 wound up with more work to do, or just happened to be the one to add enough load to tip us over the edge into failing at a too-obvious and too-frequent rate.
Assignee | ||
Comment 44•8 years ago
|
||
I've tried to skip the entire shape1 reftests, but Win7 debug R1 jobs still fail a lot.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd69fa5de7d34da04034538f1c4fc173beb82402
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•8 years ago
|
||
I rebase and modify Part 6 a little bit. Instead of moving ComputeEllipseLineInterceptDiff() and XInterceptAtY() to nsFloatManager, I move them to ShapeInfo because they're needed only by the shapes. nsFloatManager doesn't need to know them.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•8 years ago
|
||
I'll try disable shapes reftest on Win7 debug like bug 1316482 comment 136 and land it. It still has chance to cause known intermittent though. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada029656798e8cb3e9effe833be6c3c5a6c86a6
If it is not work, we might consider disable the variables reftests since most of the intermittents come from that folder. The try results is here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbff8680d8e03d3b896417e0b9b285846c8687f
Comment 64•8 years ago
|
||
mozreview-review |
Comment on attachment 8826123 [details]
Bug 1311244 Part 8 - Disable CSS shapes reftests on Windows debug builds.
https://reviewboard.mozilla.org/r/104138/#review104872
Looks fair enough to me. With disabling the folder on Win7 debug, we can still test this feature on other platforms.
Since the intermittent (tracked by Bug 1300355) is not the fault of anything in this bug, I agree that we should not block the progress here.
Attachment #8826123 -
Flags: review?(jeremychen) → review+
Comment 65•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09115127684a
Part 0 - Preemptively fix unified build bustage in nsLayoutUtils. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/ad8e7493c933
Part 1 - Use nsPoint type for center in nsCSSClipPathInstance. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/d523b9c7c0bb
Part 2 - Create ShapeUtils, and move EnumerationToLength into it. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/61d064c78aa2
Part 3 - Extract the computation of center as ComputeCircleOrEllipseCenter(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/b1d7a47b0c70
Part 4 - Extract the computation of circle radius as ComputeCircleRadius(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/60d8d64ca347
Part 5 - Convert FloatInfo's copy constructor into a move constructor. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/2893ecc79fef
Part 6 - Add ShapeInfo and move <shape-box> impl to BoxShapeInfo. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/466053d9302b
Part 7 - Implement shape-outside: circle(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/b3d203178a5e
Part 8 - Disable CSS shapes reftests on Windows debug builds. r=jeremychen
Comment 66•8 years ago
|
||
sorry had to backout for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68362219&repo=autoland
Flags: needinfo?(tlin)
Comment 67•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b178635281a
Backed out changeset b3d203178a5e
https://hg.mozilla.org/integration/autoland/rev/ab3ef58d9f81
Backed out changeset 466053d9302b
https://hg.mozilla.org/integration/autoland/rev/cf73a75e0143
Backed out changeset 2893ecc79fef
https://hg.mozilla.org/integration/autoland/rev/57282075e53f
Backed out changeset 60d8d64ca347
https://hg.mozilla.org/integration/autoland/rev/e87193dc451d
Backed out changeset b1d7a47b0c70
https://hg.mozilla.org/integration/autoland/rev/a89f21c70638
Backed out changeset 61d064c78aa2
https://hg.mozilla.org/integration/autoland/rev/fa7b1eb59660
Backed out changeset d523b9c7c0bb
https://hg.mozilla.org/integration/autoland/rev/a9eb464edda5
Backed out changeset ad8e7493c933
https://hg.mozilla.org/integration/autoland/rev/e399e5c14f32
Backed out changeset 09115127684a for reftest failures
Assignee | ||
Comment 68•8 years ago
|
||
Re comment 66:
The test failure in Android 4.3 API15+ debug R15 doesn't seem related to this bug. Perhaps my patches changed the performance characteristic somehow. Anyway, I'll fix the intermittent bug 1330630.
Depends on: 1330630
Flags: needinfo?(tlin)
Assignee | ||
Comment 69•8 years ago
|
||
With the patch in bug 1330630, try results look good. Bug 1330630 had been landed to autoland in bug 1330630 comment 4, so I'm going to land this again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3c8f46302f9a2e24560d26d93a0680962ebe49
Comment 70•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50a921a0b78d
Part 0 - Preemptively fix unified build bustage in nsLayoutUtils. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8b36e6de0c4a
Part 1 - Use nsPoint type for center in nsCSSClipPathInstance. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/ccdb3279cca9
Part 2 - Create ShapeUtils, and move EnumerationToLength into it. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/d1b0fefb0e4a
Part 3 - Extract the computation of center as ComputeCircleOrEllipseCenter(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/dd7f9da94447
Part 4 - Extract the computation of circle radius as ComputeCircleRadius(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/eb7632901874
Part 5 - Convert FloatInfo's copy constructor into a move constructor. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/6d0f6c3c988b
Part 6 - Add ShapeInfo and move <shape-box> impl to BoxShapeInfo. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/7de6d937014c
Part 7 - Implement shape-outside: circle(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/855c910a2891
Part 8 - Disable CSS shapes reftests on Windows debug builds. r=jeremychen
Comment 71•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50a921a0b78d
https://hg.mozilla.org/mozilla-central/rev/8b36e6de0c4a
https://hg.mozilla.org/mozilla-central/rev/ccdb3279cca9
https://hg.mozilla.org/mozilla-central/rev/d1b0fefb0e4a
https://hg.mozilla.org/mozilla-central/rev/dd7f9da94447
https://hg.mozilla.org/mozilla-central/rev/eb7632901874
https://hg.mozilla.org/mozilla-central/rev/6d0f6c3c988b
https://hg.mozilla.org/mozilla-central/rev/7de6d937014c
https://hg.mozilla.org/mozilla-central/rev/855c910a2891
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 72•8 years ago
|
||
Added a note about this to https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside#Browser_compatibility.
Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Comment 73•8 years ago
|
||
for information only (no action needed), this set of patches increased the number of constructors (+1):
== Change summary for alert #4798 (as of January 13 2017 05:54 UTC) ==
Regressions:
1% compiler_metrics num_constructors linux32 debug 180 -> 181
1% compiler_metrics num_constructors linux64 debug 180 -> 181
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4798
Comment 74•8 years ago
|
||
I've also added a note to https://developer.mozilla.org/en-US/Firefox/Experimental_features#shape-outside, now.
Sebastian
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.
Comment 76•8 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•