incorrect block-direction end margin applied after an orthogonal block

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.)
(Assignee)

Comment 1

5 years ago
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").
(Assignee)

Updated

5 years ago
Attachment #8519843 - Attachment description: wm23.html → testcase for margins on an orthogonal block
(Assignee)

Comment 2

5 years ago
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)

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
We also need to fix the mixture of writing modes in PlaceBlock, as suggested in comment 1 above.
Attachment #8520060 - Flags: review?(smontagu)
(Assignee)

Comment 4

5 years ago
And here's a simple reftest based on the testcase above.
Attachment #8520064 - Flags: review?(smontagu)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
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.