1084370 - <sup> and <sub> elements are offset the wrong way in vertical-lr writing mode

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: Testcase for <super> and <sub> in vertical text

See attached testcase. When writing-mode is vertical-lr, the "over/under" sides of the line are opposite to the "block-start/-end" sides, instead of matching them as is normal for horizontal and vertical-rl text. (The same would be true of vertical-rl with text-orientation:sideways-left, but we aren't implementing that yet.)

WritingMode exposes this as the IsLineInverted() method. When implementing NS_STYLE_VERTICAL_ALIGN_{SUPER,SUB}, we need to take account of this so as to apply the super/subscript shift in the appropriate direction in logical block coordinates.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Make vertical-align:super and :sub respect the IsLineInverted() attribute of WritingMode.

I expect there'll be more places we need to take account of this, but this patch at least fixes the rendering of simple super/subscript examples.

Comment 2

[Simon Montagu :smontagu]

Comment on attachment 8506896 [details] [diff] [review]
Make vertical-align:super and :sub respect the IsLineInverted() attribute of WritingMode.

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

::: layout/generic/nsLineLayout.cpp
@@ +1848,5 @@
>            // offset to the baseline BCoord.
>            nscoord parentSubscript = fm->SubscriptOffset();
> +          nscoord revisedBaselineBCoord =
> +            baselineBCoord + (lineWM.IsLineInverted() ? -parentSubscript
> +                                                      : parentSubscript);

Why not just use
 baselineBCoord + parentSubscript * lineWM.FlowRelativeToLineRelativeFactor();
? (though that name seems clumsier than necessary!)

Comment 3

[Jonathan Kew [:jfkthame]]

Yes, that'd work fine too -- I'd forgotten about that method when I wrote this.

Actually, I've been looking at more of the baseline and vertical-align issues, and might try to extend the scope of this bug a bit. Let's clear the r? here for now and I'll see if I can pull together a more complete patch.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Fix handling of vertical-align in lines with vertical writing mode.

AFAICT, this fixes all the various vertical-align options, not just sub- and superscript.

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 8516630 [details] [diff] [review]
Fix handling of vertical-align in lines with vertical writing mode.

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

::: layout/generic/nsLineLayout.cpp
@@ +1875,5 @@
> +          // alignment except for the addition of the super/subscript
> +          // offset to the baseline BCoord.
> +          revisedBaselineBCoord += lineWM.FlowRelativeToLineRelativeFactor() *
> +            (verticalAlignEnum == NS_STYLE_VERTICAL_ALIGN_SUB
> +              ? fm->SubscriptOffset() : -fm->SuperscriptOffset());

I find this confusing. Can you take the special casing of _SUB and _SUPER outside the switch somehow?

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Fix handling of vertical-align in lines with vertical writing mode.

Well, I was trying to share code as much as possible between the cases, but yeah, it's a bit opaque. How about this version? ISTM this may be one of those rare cases where a |goto| statement is actually appropriate... WDYT?

Comment 7

[Simon Montagu :smontagu]

Attachment: Alternative version of the patch

I was thinking more something like this. WDYT?

Comment 8

[Jonathan Kew [:jfkthame]]

That would also work; it's likely a little more expensive, in that it does an extra conditional separately from the switch() statement, but I don't suppose it matters much in the overall scheme of things. If you have a strong preference for that version, I can live with it.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 8521593 [details] [diff] [review]
Fix handling of vertical-align in lines with vertical writing mode.

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

r=me on the rest of the patch. For me the increase in human-readability trumps the performance impact, which I think should be imperceptible here, but I'll let you choose which version to check in.

Comment 10

[Jonathan Kew [:jfkthame]]

OK, I landed this with your preferred approach (slightly modified -- the sub/sup check needs to be outside the "if (lineWM.IsVertical()) ..." block, otherwise it'd break them for horizontal mode).

https://hg.mozilla.org/integration/mozilla-inbound/rev/f77efca402c2

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8522069 [details] [diff] [review]
Alternative version of the patch

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

::: layout/generic/nsLineLayout.cpp
@@ +1861,5 @@
> +          }
> +        }
> +
> +        if (verticalAlignEnum == NS_STYLE_VERTICAL_ALIGN_SUB ||
> +            verticalAlignEnum == NS_STYLE_VERTICAL_ALIGN_SUPER) {

Just FTR, I've basically used this version, but pulled this chunk outside the if (lineWM.IsVertical()) conditional.

Comment 12

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> OK, I landed this with your preferred approach (slightly modified -- the
> sub/sup check needs to be outside the "if (lineWM.IsVertical()) ..." block,
> otherwise it'd break them for horizontal mode).

Oops, I thought it *was* outside that block, and as you can tell I didn't test it very hard ;-)

Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f77efca402c2