Closed Bug 1094914 Opened 7 years ago Closed 7 years ago

incorrect block start/end margins are applied by nsLineLayout

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 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.
Here's a reftest version of the testcase here; the rendering should be unaffected by the writing mode of the inner <div>.
Attachment #8518288 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Fix based on converting frame margins to the line's writing mode before combining them with other values in that mode.
Attachment #8518290 - Flags: review?(smontagu)
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.
Attachment #8518294 - Flags: review?(smontagu)
Attachment #8518288 - Flags: review?(smontagu) → review+
Attachment #8518290 - Flags: review?(smontagu) → review+
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.
Attachment #8518294 - Flags: review?(smontagu) → review+
(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.
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.
Attachment #8518294 - Attachment is obsolete: true
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.
Attachment #8520719 - Flags: review+
You need to log in before you can comment on or make changes to this bug.