1088025 - incorrect container-width used when reflowing block elements within a <div> with writing-mode:vertical-rl

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: Testcase for paragraphs within a block with writing-mode:vertical-rl

See attached testcase. Displays fine in horizontal and vertical-lr mode, but when writing-mode is set to vertical-rl, the content of the second test <div>, which contains <p> elements rather than directly containing spans of text, disappears off-screen.

This seems to be related to not having the correct state.mContainerWidth on the reflow state used when we call ReflowDirtyLines() in nsBlockFrame::Reflow.

Comment 1

[Jonathan Kew [:jfkthame]]

BTW, adding background and borders to the style for <p> in the testcase reveals that the paragraphs as a whole are positioned correctly within the <div> (the background and borders appear where expected); it's just the line boxes within the paragraph that were placed using an unconstrained container-width, and hence disappear.

Comment 3

[Simon Montagu :smontagu]

The part of this not fixed by the patch in bug 1088151 is very similar to bug 1082844, in that when the text is wrapped in a <p> the LineLayout should be using the width of the <p> rather than the width of its containing <div> as mContainerWidth. I can't put my finger on where to fix it, though.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #3)
> The part of this not fixed by the patch in bug 1088151 is very similar to
> bug 1082844, in that when the text is wrapped in a <p> the LineLayout should
> be using the width of the <p> rather than the width of its containing <div>
> as mContainerWidth. I can't put my finger on where to fix it, though.

Yes, entirely agree with this ... unfortunately including the fact that I can't put my finger on where to fix it yet.

Comment 5

[Jonathan Kew [:jfkthame]]

OK, I think I've made some progress here. The root of the problem is that we're not passing the correct container-width to nsLineLayout::BeginLineReflow, and therefore when the frames on each line are placed, the logical-coordinate positions that we compute for them don't get correctly converted to physical positions.

If we passed the correct container-width (which would be the block-size of the paragraph) to BeginLineReflow, all would be well. Unfortunately, we can't do that, as before we've reflowed the lines that are going to make up the paragraph, we don't yet know what its final block-size is going to be.

(The same "bug" exists for other writing modes, in the sense that we don't pass a correct container-width here, but it doesn't matter because the container-width doesn't end up getting used to position the lines. But for vertical-rl mode, it's a key input to the logical-to-physical conversion.)

What I've been experimenting with is a patch to "fix up" the positions of the lines in a vertical-rl block once the final size of the block is known. We also need to adjust the positions of any associated bullets or floats, as those will also have been placed with incorrect physical coordinates during reflow. (There may well be other things that need similar adjustment, but these are the ones I've run across so far.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Ensure nsBlockReflowState gets an appropriately constrained mContainerWidth.

First, we need to ensure we're not passing NS_UNCONSTRAINEDSIZE as the container width in orthogonal-flow cases.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Fix up block-dir position of lines in a vertical-rl block once we know the final block size (container width) needed to map to physical coordinates.

And then here's the patch (or should I say 'hack') to adjust line positions in vertical-rl paragraphs after we know the size of the containing paragraph.

Comment 8

[Jonathan Kew [:jfkthame]]

FTR, this also depends on the SlideLine issues (bug 1089388, bug 1089581) being fixed before it will work correctly.

Comment 9

[Jonathan Kew [:jfkthame]]

:dbaron, will you have a chance to look at this sometime soon, or should I redirect it to Simon?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8512151 [details] [diff] [review]
part 1 - Ensure nsBlockReflowState gets an appropriately constrained mContainerWidth.

Sorry for the delay.

I don't see why you want mContainerWidth to work this way.  Shouldn't it become logical instead?

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #10)
> Comment on attachment 8512151 [details] [diff] [review]
> part 1 - Ensure nsBlockReflowState gets an appropriately constrained
> mContainerWidth.
> 
> Sorry for the delay.
> 
> I don't see why you want mContainerWidth to work this way.  Shouldn't it
> become logical instead?

No, "container width" is always a physical width. Its role is in converting horizontal coordinates (whether inline values for horizontal-tb, depending on bidi direction, or block-direction values for vertical-lr vs vertical-rl) between left-to-right (with the origin at the left of the container), and right-to-left (with the origin at the right of the containing box).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8512151 [details] [diff] [review]
part 1 - Ensure nsBlockReflowState gets an appropriately constrained mContainerWidth.

Er, I guess I see why it's physical, and that it's an entirely new thing introduced just for logicalization.

I still don't understand why this code in particular makes sense.  It seems like it makes sense only if it doesn't actually matter what the container width is as long as it's consistent -- in which case, why not just act like it's 0 all the time and stop wasting energy passing it around?  But it seems to me like it does matter.  So why do you believe this yields correct conversions?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8512153 [details] [diff] [review]
part 2 - Fix up block-dir position of lines in a vertical-rl block once we know the final block size (container width) needed to map to physical coordinates.

>+  // If writing-mode is vertical-rl, we need to update the bounds of any
>+  // lines that were placed relative to our container width during reflow,
>+  // to be based on the actual width we ended up computing.

Please mention specifically that vertical-rl is the only writing mode in which the block logical direction progresses in a negative physical direction.

Also, could you explain what the frames are being placed relative to during reflow?  I guess it's "whatever we picked as a container width"?  But if that's the case, why not just pick 0 rather than crawl up the reflow state chain, potentially all the way to the top?

>+  if (wm.IsVertical() && !wm.IsVerticalLR()) {

How about wm.GetBlockDir() == eBlockRL?  I think that's a little clearer.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See comment 12 and comment 13 (note that comment 12 was 4 seconds after comment 11)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, not that after seeing part 2, I now understand why you can get away with picking a bogus value.  The purpose of part 2 is to correct for having used a bogus value.  But why not pick an easier-to-compute bogus value if you're going to do so?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8512151 [details] [diff] [review]
part 1 - Ensure nsBlockReflowState gets an appropriately constrained mContainerWidth.

Also, it would probably be helpful to have a comment above {nsBlockReflowState,nsLineLayout,nsLineBox}::mContainerWidth that says something like:
  // Physical width.  Use only for physical <-> logical coordinate conversion.
so that people aren't tempted to use it for other things.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #15)
> Also, not that after seeing part 2, I now understand why you can get away
> with picking a bogus value.  The purpose of part 2 is to correct for having
> used a bogus value.  But why not pick an easier-to-compute bogus value if
> you're going to do so?

I think you're right, we can simply use zero in this case, and let Part 2 move the lines to their proper places. I'll test this a bit more and aim to post an updated patch tomorrow.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Ensure nsBlockReflowState has a constrained mContainerWidth before we reflow lines into the container.

Simplified this to use zero instead of looking up the chain.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Fix up block-dir position of lines in a vertical-rl block once we know the final block size (container width) needed to map to physical coordinates.

Updated per comment 13, carrying forward r=dbaron.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for positioning of lines within paragraphs in a vertical-rl block.

Just noticed that I failed to post the corresponding reftest patch for this bug. Here is it.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521286 [details] [diff] [review]
part 1 - Ensure nsBlockReflowState has a constrained mContainerWidth before we reflow lines into the container.

Could you be more specific in the comment and change "after reflow" to "at the end of nsBlockFrame::Reflow".

r=dbaron with that or similar

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521310 [details] [diff] [review]
Reftest for positioning of lines within paragraphs in a vertical-rl block.

I assume this failed without the patch and passes with it?

r=dbaron if so



Maybe it's also worth adding a commented-out include of writing-modes/reftest.list in the main reftest.list so that we're less likely to forget about turning it on?

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #22)
> Comment on attachment 8521310 [details] [diff] [review]
> Reftest for positioning of lines within paragraphs in a vertical-rl block.
> 
> I assume this failed without the patch and passes with it?

Indeed. Without the patch, the text is completely missing from the reftest image (it'd be positioned way off-screen).

> Maybe it's also worth adding a commented-out include of
> writing-modes/reftest.list in the main reftest.list so that we're less
> likely to forget about turning it on?

Could do, though I already have the addition in my local patch queue that enables vertical-mode support, so I think we're unlikely to forget it.

Comment 24

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/355036dd6de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/633acdbe6568
https://hg.mozilla.org/integration/mozilla-inbound/rev/902d968a06b1

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/355036dd6de6
https://hg.mozilla.org/mozilla-central/rev/633acdbe6568
https://hg.mozilla.org/mozilla-central/rev/902d968a06b1