1057331 - for text-orientation:mixed, split text runs based on Vertical_Orientation property of the characters

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: add orientation flags to gfxShapedText/gfxTextRun and to glyph runs within the text run, and split glyph runs on orientation changes

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: add orientation flags to gfxShapedText/gfxTextRun and to glyph runs within the text run, and split glyph runs on orientation changes

Rebased on top of the gfxFont split in bug 1066043. This provides the UTR50 support needed for text-orientation:mixed.

Comment 3

[John Daggett (:jtd)]

Comment on attachment 8489235 [details] [diff] [review]
add orientation flags to gfxShapedText/gfxTextRun and to glyph runs within the text run, and split glyph runs on orientation changes

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

r+ with minor comment adjustments.

::: gfx/thebes/gfxFont.h
@@ +521,5 @@
> +        TEXT_ORIENT_VERTICAL_UPRIGHT  = 0x1000,
> +        TEXT_ORIENT_VERTICAL_SIDEWAYS_RIGHT = 0x2000,
> +        TEXT_ORIENT_VERTICAL_SIDEWAYS_LEFT  = 0x4000,
> +        TEXT_ORIENT_VERTICAL_MIXED    = 0x8000,
> +

I think this needs better explanation. When and how are these intended to be used? The code seems to use mixed initially, then break that down into one of the other vertical values.

::: gfx/thebes/gfxTextRun.h
@@ +410,5 @@
>      struct GlyphRun {
>          nsRefPtr<gfxFont> mFont;   // never null
>          uint32_t          mCharacterOffset; // into original UTF16 string
>          uint8_t           mMatchType;
> +        uint16_t          mOrient; // gfxTextRunFactory::TEXT_ORIENT_* value

The abbreviation of 'orientation' to 'orient' is sort of unfortunate. But this seems to be consistent with other changes you and Simon are making so I guess it's too much effort to fix.

::: layout/mathml/nsMathMLChar.cpp
@@ +553,5 @@
>    };
>    gfxTextRun* textRun = gfxTextRun::Create(&params, 1, aFontGroup, 0);
>    textRun->AddGlyphRun(aFontGroup->GetFontAt(0), gfxTextRange::kFontGroup, 0,
> +                       false, gfxTextRunFactory::TEXT_ORIENT_HORIZONTAL);
> +                              // FIXME: what about math in vertical modes?

FIXME?!? You can't be serious. A simple comment noting that math runs are assumed to be horizontal will suffice.

Comment 4

[John Daggett (:jtd)]

Hmmm, as I start to look at other patches, it's confusing to see "orientation" used in some places and "orient" used in others. I'm starting to feel more strongly that we should be consistent and avoid 'orient', as that seems to be associated with the old 'box-orient' property. At least for text orientation, let's use 'orientation' consistently and avoid 'orient' as an abbreviation.

Comment 5

[Jonathan Kew [:jfkthame]]

I'll make this mOrientation. Just was too lazy to type it all... :)

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #3)
> I think this needs better explanation. When and how are these intended to be
> used? The code seems to use mixed initially, then break that down into one
> of the other vertical values.

Right; MIXED is the default for vertical textRuns, but is resolved to UPRIGHT or SIDEWAYS_RIGHT according to UTR50 properties, and at the glyphRun level MIXED can no longer occur.

(The other vertical settings can also occur at the textRun level, if text-orientation is explicitly set to an 'upright' or 'sideways-*' value rather than the default of 'mixed'. In this case, it's simply copied to the glyphRun(s), as no UTR50 resolution is needed.)

I'll add a comment on this.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4703e422ab

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a4703e422ab

Comment 9

[Jacek Caban]

I landed a trivial fixup for mingw, where wchar_t != char16_t:

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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