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.

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 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!)

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.

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

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?

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?

I was thinking more something like this. WDYT?

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 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.

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 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.

(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!