1082844 - multiple application of container-width to block-dir position of spans within a line with writing-mode:vertical-rl

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase to demonstrate vertical-rl embedded span problems

(This depends on using a build with vertical writing-modes enabled in WritingModes.h and in prefs.)

When writing-mode is vertical-rl, subelements (<span>, <b>, etc.) within a line get rendered too far to the right. It looks like the container-width (used whento converting R-L block coordinates to physical L-R coordinates) from the containing block is being doubly applied when setting the position of the frame in the block direction. An element nested again within such an element is rendered offset by a further container-width amount.

See attached testcase; note the misplacement of the <b> and <i> elements (with pale blue and pale red background) when vertical-rl mode is selected.

Comment 1

[Simon Montagu :smontagu]

The problem here is that whenever we convert between logical and physical coordinates in nsLineLayout, we use the width of the containing block, as the container-width. This is fine for frames that are immediate children of the line, but nested frames' coordinates are relative to their parents, so they need to use the line width instead.

It may be tricky to get the fix exactly right without lots more testcases than we have.

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch

So I think this is a reasonable initial fix, though we may need to refine it later. It fixes the test case attached here and doesn't regress any existing tests, for what that's worth.

What do you think about attaching a reftest? It won't test anything right now until the vertical-text switch is switched on so in a sense it's dead weight, but I think there's something to be said for attaching tests as we go rather than having to add lots at once later. Or we could put them in a subdirectory and include it when we turn the switch on.

Comment 3

[Simon Montagu :smontagu]

> What do you think about attaching a reftest?

Or rather, *checking in* a reftest.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8507553 [details] [diff] [review]
Patch

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

Seems reasonable enough (and works OK in what I've tested so far); let's do it.

::: layout/generic/nsLineLayout.h
@@ +480,5 @@
> +  // The container width to use when converting between logical and
> +  // physical coordinates for frames in this span. For the root span
> +  // this is the width of the block cached in mContainerWidth; for
> +  // child spans it's the width of the root span
> +  nscoord ContainerWidthForSpan(PerSpanData* aPSD) {

Looks like we could constify both the param and the method here:

  nscoord ContainerWidthForSpan(const PerSpanData* aPSD) const

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #3)
> > What do you think about attaching a reftest?
> 
> Or rather, *checking in* a reftest.

Yes, it would be good to start landing (disabled) tests, if only to help us keep track of testcases. I'd suggest we use a separate reftests/writing-mode directory that we can leave out of the overall manifest until the switch is enabled.

Comment 6

[Simon Montagu :smontagu]

Attachment: reftest

Comment 7

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f38da83c1d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b28249abe6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f38da83c1d9c
https://hg.mozilla.org/mozilla-central/rev/37b28249abe6