1083892 - In a block with vertical writing-mode, only the first of a series of child blocks (e.g. paragraphs) appears correctly

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

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

See testcase. When vertical-lr writing mode is selected, only the first of the three paragraphs within the test <div> is visible.

Note that an unexpected vertical scrollbar appears on the top-level viewport, suggesting that layout may have placed the following paragraphs far below the expected position (thanks to an unconstrained height somewhere?), although scrolling down does not reveal them.

Comment 1

[Jonathan Kew [:jfkthame]]

To fix this, we need to make nsHTMLReflowState::CalculateBlockSideMargins work with logical coordinates. I have a WIP patch that seems to work pretty well; waiting for tryserver results (https://tbpl.mozilla.org/?tree=Try&rev=bb776069eab6) before posting for review.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Make CalculateBlockSideMargins work with logical coordinates.

Updated to fix a few test failures from the first attempt; now passes on try. https://tbpl.mozilla.org/?tree=Try&rev=c52ae6754d05

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Reftest.

And a reftest to go with it, based on the original failing testcase here. (Passes harmlessly when vertical mode is not enabled.)

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8510144 [details] [diff] [review]
Make CalculateBlockSideMargins work with logical coordinates.

Review of attachment 8510144 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2343,5 @@
>    if (availMarginSpace < 0) {
>      if (mCBReflowState &&
> +        mCBReflowState->GetWritingMode().IsBidiLTR() !=
> +          mWritingMode.IsBidiLTR()) {
> +      margin.IStart(mWritingMode) += availMarginSpace;

I'm not clear why you have this condition instead of just adding the space to margin.IEnd(mWritingMode) in all cases.

@@ +2406,5 @@
>      // it to 'auto', depending on 'direction'.
>      else if (mCBReflowState &&
> +             mCBReflowState->GetWritingMode().IsBidiLTR() !=
> +               mWritingMode.IsBidiLTR()) {
> +      isAutoStartMargin = true;

Same question here

Comment 5

[Jonathan Kew [:jfkthame]]

Which side's margin we need to adjust is dependent on the parent's directionality; if that doesn't match this state's mWritingMode, then we need to adjust the opposite margin than we'd do if they were the same direction.

Without this, we fail a number of reftests, e.g. a lot of the examples in box-properties/CSS21-t100303.xhtml.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8510144 [details] [diff] [review]
Make CalculateBlockSideMargins work with logical coordinates.

Review of attachment 8510144 [details] [diff] [review]:
-----------------------------------------------------------------

I see.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80ba33c7c549
https://hg.mozilla.org/integration/mozilla-inbound/rev/155f1e8a1578

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80ba33c7c549
https://hg.mozilla.org/mozilla-central/rev/155f1e8a1578