Closed Bug 1316549 Opened 8 years ago Closed 8 years ago

Fix <shape-box> value with border-radius for non-ltr direction and writing-mode

Categories

(Core :: Layout: Floats, defect, P3)

Product:
Core
Component:
Layout: Floats
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1316549 Part 1 - Fix LogicalSideForLineRelativeDir() for eLineRelativeDirLeft/Right.
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1316549 Part 2 - Fix assertion failure: aRadiusY > 0 in FloatInfo::XInterceptAtY().
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1316549 Part 4 - Use logical names for ComputeEllipseLineInterceptDiff().
58 bytes, text/x-review-board-request
dbaron
: review+
Details 
File per bug 1309467 comment 22. 

nsFloatManager::FloatInfo::LineLeft/LineRight and related helper functions might not be correct for non-ltr direction and writing-mode, and we also need tests to prove it correct.
Priority: -- → P3
Assignee: nobody → tlin
Comment on attachment 8811645 [details]
Bug 1316549 Part 2 - Fix assertion failure: aRadiusY > 0 in FloatInfo::XInterceptAtY().

https://reviewboard.mozilla.org/r/93680/#review94328

r=dbaron, if you checked that the tests fail (due to the assertion) without the patch
Attachment #8811645 - Flags: review?(dbaron) → review+
Comment on attachment 8811646 [details]
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.

https://reviewboard.mozilla.org/r/93682/#review94332

r=dbaron if you've checked both (a) that the tests fail without the patch and (b) that the tests adequately cover what happens if you get the X vs. Y parts of the corners swapped, and the blockStart vs. blockEnd parts of the corners swapped.  (The latter is testable by removing the swapping that you have in the patch; the former is a little trickier.)

::: layout/generic/nsFloatManager.cpp:224
(Diff revision 2)
>        const nscoord bandBlockEnd =
>          aBandInfoType == BandInfoType::BandFromPoint ? blockStart : blockEnd;
>        if (floatStyle == StyleFloat::Left) {
>          // A left float
>          nscoord lineRightEdge =
> -          fi.LineRight(aShapeType, blockStart, bandBlockEnd);
> +          fi.LineRight(aWM, aShapeType, blockStart, bandBlockEnd);

So I'm trying to understand whether we want aWM or mWritingMode here.  (They're allowed to differ in inline-direction, but not block direction.)  And it looks like if they do differ in inline direction, which is used will produce different results.

Per https://drafts.csswg.org/css-writing-modes/#line-mappings , float:left is relative to the line-left of the containing block.  But the containing block and the element establishing the block formatting context (BFC) might be different.  I'm believe that aWM is the writing-mode of the containing block and mWritingMode is the writing-mode of the element establishing the BFC (though this should definitely be better documented; I'd welcome a patch).

So I think this is right, but maybe it should have a comment explaining why it's the right choice.

It's also probably worth having a test where the float's containing block is rtl and the element establishing the BFC is ltr (or vice-versa).

::: layout/generic/nsFloatManager.cpp:621
(Diff revision 2)
> +    mozilla::Side lineLeftSide =
> +      aWM.PhysicalSide(aWM.LogicalSideForLineRelativeDir(eLineRelativeDirLeft));
> +    nscoord blockStartCornerRadiusX =
> +      radii[NS_SIDE_TO_HALF_CORNER(lineLeftSide, true, false)];
> +    nscoord blockStartCornerRadiusY =
> +      radii[NS_SIDE_TO_HALF_CORNER(lineLeftSide, true, true)];
> +    nscoord blockEndCornerRadiusX =
> +      radii[NS_SIDE_TO_HALF_CORNER(lineLeftSide, false, false)];
> +    nscoord blockEndCornerRadiusY =
> +      radii[NS_SIDE_TO_HALF_CORNER(lineLeftSide, false, true)];

You should have a comment (here, and in LineRight, and probably also in ComputeEllipseXInterceptDiff) explaining that X and Y are logical here rather than physical, since X vs. Y is being determined by the third parameter which is whether you want the half of the corner parallel to the side.

It might also be good to come up with a logical version of the X/Y naming.
Attachment #8811646 - Flags: review?(dbaron) → review+
Comment on attachment 8812072 [details]
Bug 1316549 Part 4 - Use logical names for ComputeEllipseLineInterceptDiff().

https://reviewboard.mozilla.org/r/93976/#review94336

::: layout/generic/nsFloatManager.h:352
(Diff revision 1)
>      nscoord BEnd(ShapeType aShapeType) const
>      {
>        return aShapeType == ShapeType::Margin ? BEnd() : ShapeBoxRect().YMost();
>      }
>  
> -    // Compute the minimum x-axis difference between the bounding shape box
> +    // Compute the minimum line-relative axis difference between the

Just call this "inline-axis" rather than "line-relative axis"

::: layout/generic/nsFloatManager.h:354
(Diff revision 1)
>        return aShapeType == ShapeType::Margin ? BEnd() : ShapeBoxRect().YMost();
>      }
>  
> -    // Compute the minimum x-axis difference between the bounding shape box
> -    // and its rounded corner within the given band (y-axis region). This is
> -    // used as a helper function to compute the LineRight() and LineLeft().
> +    // Compute the minimum line-relative axis difference between the
> +    // bounding shape box and its rounded corner within the given band
> +    // (block-relative axis region). This is used as a helper function to

block-axis rather than block-relative axis

::: layout/generic/nsFloatManager.h:355
(Diff revision 1)
>      }
>  
> -    // Compute the minimum x-axis difference between the bounding shape box
> -    // and its rounded corner within the given band (y-axis region). This is
> -    // used as a helper function to compute the LineRight() and LineLeft().
> -    // See the picture in the implementation for an example.
> +    // Compute the minimum line-relative axis difference between the
> +    // bounding shape box and its rounded corner within the given band
> +    // (block-relative axis region). This is used as a helper function to
> +    // compute the LineRight() and LineLeft(). See the graph in the

Keep "picture" rather than "graph".

::: layout/generic/nsFloatManager.h:358
(Diff revision 1)
> -    // and its rounded corner within the given band (y-axis region). This is
> -    // used as a helper function to compute the LineRight() and LineLeft().
> -    // See the picture in the implementation for an example.
> +    // bounding shape box and its rounded corner within the given band
> +    // (block-relative axis region). This is used as a helper function to
> +    // compute the LineRight() and LineLeft(). See the graph in the
> +    // implementation for an example.
>      //
> -    // Returns the x-axis diff, or 0 if there's no rounded corner within
> +    // Returns radius-x diff on the line-relative axis, or 0 if there's no

inline-axis rather than line-relative axis, and maybe have a logical term for radius-x.

::: layout/generic/nsFloatManager.cpp:712
(Diff revision 1)
>  
>    return LineRight();
>  }
>  
>  /* static */ nscoord
> -nsFloatManager::FloatInfo::ComputeEllipseXInterceptDiff(
> +nsFloatManager::FloatInfo::ComputeEllipseRadiusXInterceptDiff(

Again, either have a comment about the X/Y being logical, or find better names.

Also, I'm not sure what adding the word "Radius" does.  Maybe take it back out?

::: layout/generic/nsFloatManager.cpp:718
(Diff revision 1)
> -  const nscoord aShapeBoxY, const nscoord aShapeBoxYMost,
> -  const nscoord aTopCornerRadiusX, const nscoord aTopCornerRadiusY,
> -  const nscoord aBottomCornerRadiusX, const nscoord aBottomCornerRadiusY,
> -  const nscoord aBandY, const nscoord aBandYMost)
> +  const nscoord aShapeBoxBStart, const nscoord aShapeBoxBEnd,
> +  const nscoord aBStartCornerRadiusX, const nscoord aBStartCornerRadiusY,
> +  const nscoord aBEndCornerRadiusX, const nscoord aBEndCornerRadiusY,
> +  const nscoord aBandBStart, const nscoord aBandBEnd)
>  {
>    // An Example for the band intersects with the top right corner of an ellipse.

Mention the example is with writing-mode: horizontal-tb?
Attachment #8812072 - Flags: review?(dbaron) → review+
Comment on attachment 8811646 [details]
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.

https://reviewboard.mozilla.org/r/93682/#review94340

::: layout/generic/nsFloatManager.cpp:224
(Diff revision 2)
>        const nscoord bandBlockEnd =
>          aBandInfoType == BandInfoType::BandFromPoint ? blockStart : blockEnd;
>        if (floatStyle == StyleFloat::Left) {
>          // A left float
>          nscoord lineRightEdge =
> -          fi.LineRight(aShapeType, blockStart, bandBlockEnd);
> +          fi.LineRight(aWM, aShapeType, blockStart, bandBlockEnd);

Er, actually, now I think I'm wrong about line-left and line-right being able to differ between two writing modes with the same block direction.  But it would be good if you can confirm you agree, and maybe still improve the comment.
Comment on attachment 8811645 [details]
Bug 1316549 Part 2 - Fix assertion failure: aRadiusY > 0 in FloatInfo::XInterceptAtY().

https://reviewboard.mozilla.org/r/93680/#review94328

> r=dbaron, if you checked that the tests fail (due to the assertion) without the patch

Yes. I've checked the tests fail without the modification to nsFloatManager.cpp.
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #11)
> Er, actually, now I think I'm wrong about line-left and line-right being
> able to differ between two writing modes with the same block direction.

This mismatch occurs for sideways-lr vs vertical-lr, doesn't it? These both have block direction left-to-right, but for sideways-lr, line-left will be bottom, whereas for vertical-lr it will be top.
Comment on attachment 8811644 [details]
Bug 1316549 Part 1 - Fix LogicalSideForLineRelativeDir() for eLineRelativeDirLeft/Right.

https://reviewboard.mozilla.org/r/93678/#review94492

Yes, this looks correct. (Be sure to include sideways-lr examples in your testing, as that tends to be the "odd" case that exposes issues like this.)
Attachment #8811644 - Flags: review?(jfkthame) → review+
Comment on attachment 8811646 [details]
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.

https://reviewboard.mozilla.org/r/93682/#review94332

(a) Checked. 7 test fails without the patch. (b) I don't have test to test X vs Y coners swapped, but I'll add something; blockStart vs. blockEnd is covered by the "writing-mode: vertical-lr"
Comment on attachment 8811646 [details]
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.

https://reviewboard.mozilla.org/r/93682/#review94332

> You should have a comment (here, and in LineRight, and probably also in ComputeEllipseXInterceptDiff) explaining that X and Y are logical here rather than physical, since X vs. Y is being determined by the third parameter which is whether you want the half of the corner parallel to the side.
> 
> It might also be good to come up with a logical version of the X/Y naming.

Yes. The RadiusX/Y are logical here, and I feel it is easily to get confuse with the physical position x/y. How about we use RadiusL and RadiusB for "radius on the line-axis and block-axis"?
NI for comment 16.
Flags: needinfo?(dbaron)
Comment on attachment 8811644 [details]
Bug 1316549 Part 1 - Fix LogicalSideForLineRelativeDir() for eLineRelativeDirLeft/Right.

https://reviewboard.mozilla.org/r/93678/#review94492

> Yes, this looks correct. (Be sure to include sideways-lr examples in your testing, as that tends to be the "odd" case that exposes issues like this.)

Thanks. I've added sideways-lr tests in shape-outside-border-box-border-radius-011/012.html in Part 3.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #16)
> Yes. The RadiusX/Y are logical here, and I feel it is easily to get confuse
> with the physical position x/y. How about we use RadiusL and RadiusB for
> "radius on the line-axis and block-axis"?

I think RadiusI and RadiusB would be better, since we use I/B elsewhere.  But otherwise, I suppose that sounds good.
Flags: needinfo?(dbaron)
Comment on attachment 8811646 [details]
Bug 1316549 Part 3 - Fix <shape-box> with border-radius in writing-mode.

https://reviewboard.mozilla.org/r/93682/#review94340

> Er, actually, now I think I'm wrong about line-left and line-right being able to differ between two writing modes with the same block direction.  But it would be good if you can confirm you agree, and maybe still improve the comment.

I agree. The two writing modes, which are aWM and mWritingMode here, must have the same block direction and the same "writing-mode" value, since 'writing-mode' creates a block formating context. According to the table [1], the css "direction" property cannot affect the physical side of line-left/line-right. We don't need to worry about inline direction, since we are dealing with floats.

BTW, mWritingMode is debug-only. If there's anything we care about are different from aWM, we'll need to store mWritingMode all the time.

[1] https://drafts.csswg.org/css-writing-modes/#logical-to-physical
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #19)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #16)
> > Yes. The RadiusX/Y are logical here, and I feel it is easily to get confuse
> > with the physical position x/y. How about we use RadiusL and RadiusB for
> > "radius on the line-axis and block-axis"?
> 
> I think RadiusI and RadiusB would be better, since we use I/B elsewhere. 
> But otherwise, I suppose that sounds good.

Yes. We use I/B elsewhere. However, line-axis is more prominent and use extensively in nsFloatManager. It's also related to float left/right. Perhaps line would be better in float layout specifically.

For Part 4, ComputeEllipseXInterceptDiff() could be renamed ComputeEllipseLineInterceptDiff().
I modify the test cases in Part 2 so that the border-radii have different semi-major axis, so they will catch failures if the code gets the two semi-major axis of the corners swapped. Also added tests where the float's containing block is rtl and the element establishing the BFC is ltr (and vice-versa).

The variables and function arguments in Part 3 and 4 have logical names per comment 21.
Blocks: 1319672
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7286c44643a
Part 1 - Fix LogicalSideForLineRelativeDir() for eLineRelativeDirLeft/Right. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/aaf70a7cd344
Part 2 - Fix assertion failure: aRadiusY > 0 in FloatInfo::XInterceptAtY(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/0d04f4ee523b
Part 3 - Fix <shape-box> with border-radius in writing-mode. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/e9872fff04e2
Part 4 - Use logical names for ComputeEllipseLineInterceptDiff(). r=dbaron
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: