Closed Bug 1451499 Opened 2 years ago Closed 2 years ago

Implement 'shape-margin' for polygon

Categories

(Core :: Layout: Floats, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 1 obsolete file)

When Bug 1265342 lands, it will add support for all shape-margin shapes except polygon. Polygon implementation is complex enough that it is being split into this separate bug.
Comment on attachment 8969425 [details]
Bug 1451499 Part 2: Stub in support for polygon with shape-margin == 0.

https://reviewboard.mozilla.org/r/238182/#review243916


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsFloatManager.cpp:1245
(Diff revision 1)
> +
> +nsFloatManager::PolygonShapeInfo::PolygonShapeInfo(
> +  nsTArray<nsPoint>&& aVertices,
> +  nscoord aShapeMargin,
> +  int32_t aAppUnitsPerDevPixel,
> +  nsRect aMarginRect)

Warning: The parameter 'amarginrect' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

 0:09.71   nsRect aMarginRect)
 0:09.71   ~~~~~~ ^
 0:09.71   const &
Attachment #8969423 - Attachment is obsolete: true
Attachment #8969424 - Flags: review?(jfkthame)
Attachment #8969425 - Flags: review?(jfkthame)
Attachment #8969426 - Flags: review?(jfkthame)
Attachment #8971450 - Flags: review?(jfkthame)
I haven't studied this in any depth yet, but I'm concerned that the code here seems to be mixing physical and logical coordinates in ways that don't look valid, offhand. Does the shape-margin implementation actually work in vertical writing modes? (It doesn't look like there's much relevant test coverage, sadly.)

Can you either reassure me that this is actually OK (and why), or file a bug about shapes/writing-mode and try creating some testcases to see what really happens?

(I think this is a pre-existing issue in the shapes code, rather than a new problem you're introducing here, but it makes for some fishy-looking code, and I'd be inclined to say we should fix it properly rather than extending a broken implementation further.)
Flags: needinfo?(bwerth)
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> I haven't studied this in any depth yet, but I'm concerned that the code
> here seems to be mixing physical and logical coordinates in ways that don't
> look valid, offhand. Does the shape-margin implementation actually work in
> vertical writing modes? (It doesn't look like there's much relevant test
> coverage, sadly.)
> 
> Can you either reassure me that this is actually OK (and why), or file a bug
> about shapes/writing-mode and try creating some testcases to see what really
> happens?
> 
> (I think this is a pre-existing issue in the shapes code, rather than a new
> problem you're introducing here, but it makes for some fishy-looking code,
> and I'd be inclined to say we should fix it properly rather than extending a
> broken implementation further.)

The best explanation I have is that nsFloatManager converts coordinates to x = inline, y = block consistently (https://searchfox.org/mozilla-central/source/layout/generic/nsFloatManager.h#67) and checks the floats in the LineLeft() and LineRight() methods in the converted "logical" space. This is why all the ShapeInfo classes rely upon that mapping; in this patch it's done before the PolygonShapeInfo constructor is called, in nsFloatManager::ShapeInfo::CreatePolygon in Part 2 of the patch. The vertices of the polygon is convereted, and the margin rect is converted. All the math is done in the converted space.

So although I agree that the WPT tests don't have good coverage of other writing modes for shape-outside -- Ting-Yu Lin added several for shape-outside: image -- the methods used by nsFloatManager have been in place for a long time and the logical coordinate system has been used for all types of floats very successfully.
Flags: needinfo?(bwerth)
Comment on attachment 8969424 [details]
Bug 1451499 Part 1: Uplift some interval methods from ImageShapeInfo to ShapeInfo.

https://reviewboard.mozilla.org/r/238180/#review246590

::: layout/generic/nsFloatManager.cpp:1877
(Diff revision 2)
> -
> -nscoord
>  nsFloatManager::ImageShapeInfo::LineLeft(const nscoord aBStart,
>                                           const nscoord aBEnd) const
>  {
> -  return LineEdge(aBStart, aBEnd, true);
> +  return ShapeInfo::LineEdge(mIntervals, aBStart, aBEnd, true);

I don't think the `ShapeInfo::` prefix should be needed here, as it's the superclass of ImageShapeInfo. Just remove the (no longer used) declaration of LineEdge from the ImageShapeInfo class.

::: layout/generic/nsFloatManager.cpp:1884
(Diff revision 2)
>  
>  nscoord
>  nsFloatManager::ImageShapeInfo::LineRight(const nscoord aBStart,
>                                            const nscoord aBEnd) const
>  {
> -  return LineEdge(aBStart, aBEnd, false);
> +  return ShapeInfo::LineEdge(mIntervals, aBStart, aBEnd, false);

ditto
Attachment #8969424 - Flags: review?(jfkthame) → review+
Comment on attachment 8969425 [details]
Bug 1451499 Part 2: Stub in support for polygon with shape-margin == 0.

https://reviewboard.mozilla.org/r/238182/#review246592

::: layout/generic/nsFloatManager.cpp:1258
(Diff revision 2)
>                                 const nsPoint& aP2);
>  
>    // The vertices of the polygon in the float manager's coordinate space.
>    nsTArray<nsPoint> mVertices;
>  
> +  // An interval is slice of the float area defined by this ImageShapeInfo.

s/ImageShapeInfo/PolygonShapeInfo/

I'm guessing (before reading the following patches) that mIntervals is only going to be used when there's a positive shape-margin in effect (so that you can do the "inflation" of the area in the same way as for an image); otherwise, we just work with the vertices. If that's so, please document it here.

::: layout/generic/nsFloatManager.cpp:1318
(Diff revision 2)
> +{
> +  MOZ_ASSERT(!mEmpty, "Shouldn't be called if the polygon encloses no area.");
> +
> +  // Use intervals if we have them.
> +  if (!mIntervals.IsEmpty()) {
> +    return ShapeInfo::LineEdge(mIntervals, aBStart, aBEnd, true);

Shouldn't need `ShapeInfo::` prefix here.

::: layout/generic/nsFloatManager.cpp:1341
(Diff revision 2)
> +{
> +  MOZ_ASSERT(!mEmpty, "Shouldn't be called if the polygon encloses no area.");
> +
> +  // Use intervals if we have them.
> +  if (!mIntervals.IsEmpty()) {
> +    return ShapeInfo::LineEdge(mIntervals, aBStart, aBEnd, false);

again

::: layout/generic/nsFloatManager.cpp:2400
(Diff revision 2)
> +  nsDeviceContext* dc = aFrame->PresContext()->DeviceContext();
> +  int32_t appUnitsPerDevPixel = dc->AppUnitsPerDevPixel();

No need to explicitly use the nsDeviceContext; there's a PresContext()->AppUnitsPerDevPixel() accessor, which will reduce verbosity a little.

However, what I'd actually suggest is that the caller should pass the appUnits value as a parameter, instead of passing a frame pointer that is only used to retrieve this. (I see that's what the similar ImageShapeInfo constructor does.)
Attachment #8969425 - Flags: review?(jfkthame) → review+
Comment on attachment 8969426 [details]
Bug 1451499 Part 3: Implement polygon for shape-margin > 0.

https://reviewboard.mozilla.org/r/238184/#review246676

::: layout/generic/nsFloatManager.cpp:1303
(Diff revision 5)
> +  // With a positive aShapeMargin, we have to calculate a distance
> +  // field from the opaque pixels, then build intervals based on
> +  // them being within aShapeMargin distance to an opaque pixel.

This code block looks to have a lot in common with the ImageShapeInfo implementation. Creating the distance field is different in each case, but then using it to set up the intervals array seems essentially the same, no? Can we refactor this so as to share code instead of copy/pasting the logic?
Comment on attachment 8971450 [details]
Bug 1451499 Part 4: Change some WPT reftests to expected pass.

https://reviewboard.mozilla.org/r/240196/#review246680

Passing tests = good. \o/
Attachment #8971450 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> Comment on attachment 8969426 [details]
> Bug 1451499 Part 3: Implement polygon for shape-margin > 0.
> 
> https://reviewboard.mozilla.org/r/238184/#review246676
>
> This code block looks to have a lot in common with the ImageShapeInfo
> implementation. Creating the distance field is different in each case, but
> then using it to set up the intervals array seems essentially the same, no?
> Can we refactor this so as to share code instead of copy/pasting the logic?

I agree. There's a lot about the distance field that is the same in all the implementations, but unfortunately the iteration order is quite particular, and that makes the chamfer patterns different. EllipseShapeInfo also has an optimization where it relies on symmetry to only generate a quarter of the distance field, etc. I'm going to do a follow-up bug where I refactor as much as I am able. Some refactoring of calculating the distance field size happens in Bug 1457288.
Comment on attachment 8969425 [details]
Bug 1451499 Part 2: Stub in support for polygon with shape-margin == 0.

https://reviewboard.mozilla.org/r/238182/#review246592

> No need to explicitly use the nsDeviceContext; there's a PresContext()->AppUnitsPerDevPixel() accessor, which will reduce verbosity a little.
> 
> However, what I'd actually suggest is that the caller should pass the appUnits value as a parameter, instead of passing a frame pointer that is only used to retrieve this. (I see that's what the similar ImageShapeInfo constructor does.)

CreatePolygon is not technically the constructor for PolygonShapeInfo. The other Create... methods take an nsIFrame pointer for an argument, which is used in ImageShapeInfo for additional computation. Also, the Create... methods don't always need the appUnitsPerDevPixel value. So instead of changing all the Create... methods to take appUnitsPerDevPixel as a parameter, I'm leaving them as taking the nsIFrame pointer.
Comment on attachment 8969426 [details]
Bug 1451499 Part 3: Implement polygon for shape-margin > 0.

https://reviewboard.mozilla.org/r/238184/#review247196

OK, looks good. I'd still be happy to see further refactoring to share more of the implementation (particularly between image and polygon, I realize ellipse is more different) if possible, but that can come in a followup.

The other thing that I'm wondering about - but it relates to the shapes impl as a whole, rather than this particular bug - is the use of device pixels for a lot of the computations. It seems like this may result in behavior that has a unexpected(?) dependency on the resolution of the device; in the worst case, imagine a page whose layout changes when the window is moved between hi-dpi and lo-dpi monitors. I don't think that would be desirable. But I'd need to think this through more thoroughly; if there's really an issue, it's broader than just this patch, and would need its own bug.
Attachment #8969426 - Flags: review?(jfkthame) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1ff16c65863
Part 1: Uplift some interval methods from ImageShapeInfo to ShapeInfo. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/516cc3ad8538
Part 2: Stub in support for polygon with shape-margin == 0. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/126d2e51c093
Part 3: Implement polygon for shape-margin > 0. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/41bd1a6ea025
Part 4: Change some WPT reftests to expected pass. r=jfkthame
Depends on: 1460041
This shipped in Firefox 61 and the docs are updated: https://developer.mozilla.org/en-US/docs/Web/CSS/shape-margin, so I'm marking it dev-doc-complete. Enabling it by default in 62 is covered in bug 1457297
You need to log in before you can comment on or make changes to this bug.