Closed Bug 1084370 Opened 7 years ago Closed 7 years ago

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

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

(3 files, 2 obsolete files)

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.
Blocks: writing-mode
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.
Attachment #8506896 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8506896 - Attachment description: Make text-align:super and :sub respect the IsLineInverted() attribute of WritingMode. → Make vertical-align:super and :sub respect the IsLineInverted() attribute of WritingMode.
Blocks: 1079125
No longer blocks: writing-mode
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.
Attachment #8506896 - Flags: review?(smontagu)
Depends on: 1093553
AFAICT, this fixes all the various vertical-align options, not just sub- and superscript.
Attachment #8516630 - Flags: review?(smontagu)
Attachment #8506896 - Attachment is obsolete: true
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?
Attachment #8521593 - Flags: review?(smontagu)
Attachment #8516630 - Attachment is obsolete: true
Attachment #8516630 - Flags: review?(smontagu)
I was thinking more something like this. WDYT?
Attachment #8522069 - Flags: feedback?(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 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.
Attachment #8521593 - Flags: review?(smontagu) → review+
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
Target Milestone: --- → mozilla36
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.
Attachment #8522069 - Flags: feedback?(jfkthame) → feedback+
(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!
https://hg.mozilla.org/mozilla-central/rev/f77efca402c2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.