Closed Bug 1457602 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jfkthame, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
Flags: needinfo?(bwerth)
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Thanks for finding this. I'll figure it out and make sure we have a WPT test that covers this case.
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.
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?
(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.
Attachment #8972193 - Attachment is obsolete: true
Attachment #8972657 - Flags: review?(jfkthame)
Attachment #8972658 - Flags: review?(jfkthame)
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 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.
Attachment #8972657 - Flags: review?(jfkthame)
Attachment #8972657 - Flags: review?(jfkthame)
Attachment #8973316 - Flags: review?(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

The updated patch addresses this issue. Part 3 adds tests of the cases revealed by your second example.
Priority: -- → P3
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.
Attachment #8972657 - Flags: review?(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
Attachment #8972657 - Flags: review?(jfkthame) → review+
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
Attachment #8973316 - Flags: review?(jfkthame) → review+
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
Attachment #8972658 - Flags: review?(jfkthame) → review+
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)
Pushed by bwerth@mozilla.com:
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
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10944 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.