Closed Bug 1096224 Opened 10 years ago Closed 9 years ago

incorrect block-direction end margin applied after an orthogonal block

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

See testcase. Changing writing mode of the inner (yellow) <div>s should only affect the display of their content; the overall layout should not change.

However, what actually happens is that when the writing mode of the inner <div>s is vertical, one of the *side* margins (right in the case of vertical-lr, left in the case of vertical-rl) appears *below* the inner block.

(This is kinda like a display:block version of bug 1094914, which was about similar margin confusion on display:inline elements.)
One thing that I think is a problem here is the line at

http://hg.mozilla.org/mozilla-central/annotate/d380166816dd/layout/generic/nsBlockReflowContext.cpp#l355

which combines a margin value from aReflowState (in the placed block's writing mode) with one from aMetrics (in the parent's writing mode).

However, adding what looks like the appropriate conversion here, with something like

-    aBEndMarginResult.Include(aReflowState.ComputedLogicalMargin().BEnd(wm));
+    aBEndMarginResult.Include(aReflowState.ComputedLogicalMargin().
+      ConvertTo(parentWM, wm).BEnd(parentWM));

is not sufficient (the resulting BEnd margin is close to "unconstrained").
Attachment #8519843 - Attachment description: wm23.html → testcase for margins on an orthogonal block
The main problem here, I think, is that CalculateBlockSideMargins should be computing side margins for the containing block's writing mode.
Attachment #8520059 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
We also need to fix the mixture of writing modes in PlaceBlock, as suggested in comment 1 above.
Attachment #8520060 - Flags: review?(smontagu)
And here's a simple reftest based on the testcase above.
Attachment #8520064 - Flags: review?(smontagu)
Comment on attachment 8520059 [details] [diff] [review]
part 1 - CalculateBlockSideMargins should be working in the containing block's writing mode.

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

::: layout/generic/nsHTMLReflowState.cpp
@@ +2346,5 @@
> +    ComputedLogicalMargin().ConvertTo(cbWM, mWritingMode);
> +  LogicalMargin borderPadding =
> +    ComputedLogicalBorderPadding().ConvertTo(cbWM, mWritingMode);
> +  nscoord sum = margin.IStartEnd(cbWM) +
> +    borderPadding.IStartEnd(cbWM) + aComputedISize;

Is this right? It looks as if the caller is passing in aComputedISize and aAvailISize from ComputedISize() and AvailISize(), which will be in its own writing mode.
Attachment #8520060 - Flags: review?(smontagu) → review+
I think you're right, that doesn't make sense.

Actually, there's no need for the caller to pass these values at all; as CalculateBlockSideMargins is a method of the same nsHTMLReflowState that's calling it, it can decide internally (based on the writing mode of the containing block) what to use.
Attachment #8520059 - Attachment is obsolete: true
Attachment #8520059 - Flags: review?(smontagu)
Comment on attachment 8528301 [details] [diff] [review]
part 1 - CalculateBlockSideMargins should be working in the containing block's writing mode.

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

::: layout/generic/nsHTMLReflowState.cpp
@@ +2356,5 @@
> +    availISizeCBWM = AvailableBSize();
> +  } else {
> +    computedISizeCBWM = ComputedISize();
> +    availISizeCBWM = AvailableISize();
> +  }

I'm in two minds whether it would be preferable to just do
  nscoord computedISizeCBWM = ComputedSize(cbWM).ISize(cbWM);
  nscoord availISizeCBWM = AvailableSize(cbWM).ISize(cbWM);

On the one hand it makes the code here cleaner. On the other hand it's a bit less efficient (but not in the commonest case IIANM).

@@ +2369,5 @@
> +    ComputedLogicalMargin().ConvertTo(cbWM, mWritingMode);
> +  LogicalMargin borderPadding =
> +    ComputedLogicalBorderPadding().ConvertTo(cbWM, mWritingMode);
> +  nscoord sum = margin.IStartEnd(cbWM) +
> +    borderPadding.IStartEnd(cbWM) + computedISizeCBWM;

Here too a possible alternative with the same caveats is
  nscoord sum = ComputedSizeWithMarginBorderPadding(cbWM).ISize(cbWM);

r=me either way.
Attachment #8528301 - Flags: review?(smontagu) → review+
Attachment #8520064 - Flags: review?(smontagu) → review+
You need to log in before you can comment on or make changes to this bug.