1457602 - shape-margin does not work correctly when used with shape-outside image in vertical writing-mode

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase, shape-margin and vertical writing-mode

See testcase. When shape-margin is added to the floats (click the checkbox), it works as expected for the left-hand examples using shape-outside:circle(), but for the right-hand examples, which use shape-outside:image, it fails in vertical writing-mode.

Comment 1

[Brad Werth [:bradwerth]]

Thanks for finding this. I'll figure it out and make sure we have a WPT test that covers this case.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

This patch should address the issue, and includes some funky ASCII art to make the reasoning about chamfer calculations more visual. The patch is based off of some changes for Bug 1457288, so it might not apply and isn't ready for review just yet. I'll post an updated patch and a WPT test that exercises this once Bug 1457288 lands.

Comment 3

[Jonathan Kew [:jfkthame]]

I haven't looked in detail, but from a quick glance at the ASCII-art in the patch, I wonder if it's assuming vertical writing-mode is always vertical-rl. There's also vertical-lr, where the block direction goes left-to-right; does that need to be considered separately?

Comment 4

[Brad Werth [:bradwerth]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> I haven't looked in detail, but from a quick glance at the ASCII-art in the
> patch, I wonder if it's assuming vertical writing-mode is always
> vertical-rl. There's also vertical-lr, where the block direction goes
> left-to-right; does that need to be considered separately?

I'll add tests for these situations, but the way it is SUPPOSED to work is this:
1) The various ::ConvertToFloatLogical methods convert all rects, points into inline = x, block = y coordinates.
2) ImageShapeInfo iterates the bitmaps to measure the inline extents of each block line. That uses an iteration pattern that only has to consider the verticality of the writing-mode, because that's what determines which is block and which is inline.
3) When floats are applied, nsFloatManager uses the writing-mode to determine if it's a ::LineLeft() or ::LineRight() call, and supplies the block start and end of the float. The ShapeInfos just return the start of the extent on that "side" of the relevant interval(s).

So as long as nsFloatManager is consistent in its interpretation of the writing-mode in steps 1 and 3, all the ShapeInfos have to do is build intervals with the inline = x, block = y coordinate space. ImageShapeInfo is more complicated than the other shapes because there's no mathematical transformation to rotate the shape (for example, as PolygonShapeInfo does when it passes all the vertices to ::ConvertToFloatLogical) -- instead we "rotate" the iteration.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

Review commit: https://reviewboard.mozilla.org/r/241202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241202/

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1457602 Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes.

Review commit: https://reviewboard.mozilla.org/r/241204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241204/

Comment 7

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9848eae2e4a15a8b2719227e5d32950482e1d2d

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: extended testcase with more writing-mode and float side combinations

This helps, but it looks like there's still an issue with writing-mode:vertical-lr, even after the patch here. See this updated version of the example.

Note that this isn't limited to shape-margin; I'm seeing a problem even without any margin, just using shape-outside:<image> on a float:left element in vertical-lr writing mode. So you might want to spin off a separate bug to fix that first, if that makes more sense; but either way, I think we need that fixed before we can fully resolve the shape-margin issue.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

https://reviewboard.mozilla.org/r/241202/#review247604

Clearing the r? flag for now, though if you end up dealing with the vertical-lr issue (comment 8) in a separate patch (or bug) this may actually be OK. Please either update the patch (if necessary) or re-request review after looking into the problem there.

Comment 10

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dee595dec1b7a25f2d19c53d11651e64cf1f4d

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1457602 Part 2: Correct shape-outside: image interval creation to correctly handle sideways-lr (with or without positive shape-margin.)

Review commit: https://reviewboard.mozilla.org/r/241786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241786/

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241202/diff/1-2/

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8972658 [details]
Bug 1457602 Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241204/diff/1-2/

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

https://reviewboard.mozilla.org/r/241202/#review247604

The updated patch addresses this issue. Part 3 adds tests of the cases revealed by your second example.

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

This is ready for review, again.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

https://reviewboard.mozilla.org/r/241202/#review248368

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8973316 [details]
Bug 1457602 Part 2: Correct shape-outside: image interval creation to correctly handle sideways-lr (with or without positive shape-margin.)

https://reviewboard.mozilla.org/r/241786/#review248370

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8972658 [details]
Bug 1457602 Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes.

https://reviewboard.mozilla.org/r/241204/#review248372

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 23777b3e361ac5dd9d31727c763ca7ba88e6357d -d 2e9ace719b06: rebasing 462637:23777b3e361a "Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes. r=jfkthame"
merging layout/generic/nsFloatManager.cpp
rebasing 462638:93cda99ff8a0 "Bug 1457602 Part 2: Correct shape-outside: image interval creation to correctly handle sideways-lr (with or without positive shape-margin.) r=jfkthame"
merging layout/generic/nsFloatManager.cpp
rebasing 462639:8c64220e6662 "Bug 1457602 Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes. r=jfkthame" (tip)
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

[Brad Werth [:bradwerth]]

Comment on attachment 8972657 [details]
Bug 1457602 Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241202/diff/2-3/

Comment 21

[Brad Werth [:bradwerth]]

Comment on attachment 8973316 [details]
Bug 1457602 Part 2: Correct shape-outside: image interval creation to correctly handle sideways-lr (with or without positive shape-margin.)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241786/diff/1-2/

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8972658 [details]
Bug 1457602 Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241204/diff/2-3/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c576126a7c7e
Part 1: Correct shape-outside: image shape-margin calculation for vertical writing modes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f706fe09567f
Part 2: Correct shape-outside: image interval creation to correctly handle sideways-lr (with or without positive shape-margin.) r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f1fd4463c2a9
Part 3: Add WPT reftests with shape-outside: image and shape-margin in vertical writing modes. r=jfkthame

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10944 for changes under testing/web-platform/tests

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c576126a7c7e
https://hg.mozilla.org/mozilla-central/rev/f706fe09567f
https://hg.mozilla.org/mozilla-central/rev/f1fd4463c2a9

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged