1094914 - incorrect block start/end margins are applied by nsLineLayout

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase to demonstrate margin bugs in vertical mode

See attached testcase. When the writing mode for the test <div> is vertical-lr, its (physical) left margin of 50px appears as a *top* margin (as well as remaining on the left, as it should); and when the writing mode is vertical-rl, it appears as *bottom* margin.

This is caused by invalid mixing of frameWM and lineWM coordinates in nsLineLayout.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for margins and writing-mode of an inline block.

Here's a reftest version of the testcase here; the rendering should be unaffected by the writing mode of the inner <div>.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Don't confuse frame and line writing-modes in nsLineLayout.

Fix based on converting frame margins to the line's writing mode before combining them with other values in that mode.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Store margins and borders using line's writing mode in perFrameData, to avoid writing-mode conversions.

Having done this, it becomes apparent that we can avoid a bunch of .ConvertTo() calls if we store more things in the line's writing mode within PerFrameData. So I think we should make that switch here.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8518294 [details] [diff] [review]
Store margins and borders using line's writing mode in perFrameData, to avoid writing-mode conversions.

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

Nice!

::: layout/generic/nsLineLayout.cpp
@@ +1110,5 @@
>        // For inline-ish and text-ish things (which don't compute widths
>        // in the reflow state), adjust available inline-size to account for the
>        // start margin. The end margin will be accounted for when we
>        // finish flowing the frame.
> +      aReflowState.AvailableISize() -= pfd->mMargin.IStart(lineWM);//xxx

Please either remove the "//xxx" if it's not needed or add some explanatory text if it is.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #4)
> ::: layout/generic/nsLineLayout.cpp
> @@ +1110,5 @@
> >        // For inline-ish and text-ish things (which don't compute widths
> >        // in the reflow state), adjust available inline-size to account for the
> >        // start margin. The end margin will be accounted for when we
> >        // finish flowing the frame.
> > +      aReflowState.AvailableISize() -= pfd->mMargin.IStart(lineWM);//xxx
> 
> Please either remove the "//xxx" if it's not needed or add some explanatory
> text if it is.

Oops, that wasn't supposed to stick around! I was concerned we should probably be converting pfd->mMargin to the frame's/reflow-state's writing mode there, but let me run some tests and update if necessary.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Store margins and borders using line's writing mode in perFrameData, to avoid writing-mode conversions.

OK, here's what I think we should be doing where that stray xxx was. Though we don't have any current unit tests that can tell the difference, afaict.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8520719 [details] [diff] [review]
part 2 - Store margins and borders using line's writing mode in perFrameData, to avoid writing-mode conversions.

Carrying over r=smontagu here; only change is to fix the //xxx that was pointed out in comment 4.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29519b0eac48
https://hg.mozilla.org/integration/mozilla-inbound/rev/3183bb0cb92c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6555e3ae8c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29519b0eac48
https://hg.mozilla.org/mozilla-central/rev/3183bb0cb92c
https://hg.mozilla.org/mozilla-central/rev/5a6555e3ae8c