Closed Bug 902799 Opened 11 years ago Closed 10 years ago

support vertical text in canvas text drawing methods

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jtd, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

Canvas methods such as FillText, etc. need to support some form of vertical text rendering.  Initially I think this could be added simply by respecting the writing-mode value for the canvas element but eventually I think there probably needs to be a separate method or context parameter for this.

This is actually interesting because this can be used to experiment with vertical text run construction (bug 789104, bug 902762) without the need to wait for the more complex issues associated with handling vertical text runs within text frames.
Is there any proposal about this?
Assignee: jdaggett → jfkthame
Status: NEW → ASSIGNED
Updated to fix a crash when the Canvas element is not in a document.
Attachment #8487084 - Flags: review?(jdaggett)
Attachment #8486621 - Attachment is obsolete: true
Attachment #8486621 - Flags: review?(jdaggett)
Blocks: 1065348
Updated to include support for drawing right-to-left textruns to canvas2d.
Attachment #8489618 - Flags: review?(jdaggett)
Attachment #8487084 - Attachment is obsolete: true
Attachment #8487084 - Flags: review?(jdaggett)
Attachment #8489618 - Attachment is obsolete: true
Attachment #8489618 - Flags: review?(jdaggett)
Rebased this patch following bug 927892. (Also depends on review-pending patches in bug 902762.)
Attachment #8495164 - Flags: review?(bas)
Attachment #8490352 - Attachment is obsolete: true
Attachment #8490352 - Flags: review?(smontagu)
Comment on attachment 8495164 [details] [diff] [review]
support textruns with vertical writing modes when drawing Canvas2D text.

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

I wonder if there's a way in which we could make this code a little cleaner, perhaps taking the conditions out of some of the loops somehow.
OK, here's a version that's a bit cleaner and easier to read, I think; it sets up 'inline' and 'block'-named aliases (references) for the x and y coordinates, allowing us to avoid a bunch of the nested if-else blocks. The overall effect is the same, but the code looks less cluttered. What I'd really like to do (as suggested by the comment in the code here) is to replace the guts of CanvasBidiProcessor::DrawText with code from gfxTextRun and gfxFont, which has to do all the same work, but last time I looked that was a bit awkward because the two areas of the codebase were at different stages of Moz2d-ification. So I think it's easiest to defer that refactoring to some future bug, and just handle the drawing of different direction runs in canvas manually for now.
Attachment #8496825 - Flags: review?(bas)
Attachment #8495164 - Attachment is obsolete: true
Attachment #8495164 - Flags: review?(bas)
Bas, review ping?
Flags: needinfo?(bas)
Comment on attachment 8496825 [details] [diff] [review]
support textruns with vertical writing modes when drawing Canvas2D text.

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

Sorry about missing this r?. This is a lot better indeed and a lot easier to read. I've got one comment which is a little bit of a nit, but I'm going to r- over it just to make sure we get this all right.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3213,5 @@
> +          gfxTextRunFactory::TEXT_ORIENT_VERTICAL_SIDEWAYS_RIGHT) {
> +        sideways = true;
> +        mCtx->Save();
> +        ErrorResult error;
> +        mCtx->Translate(baselineOrigin.x, baselineOrigin.y, error);

This will result in a lot of wasteful sets and gets. Which are reasonably expensive. We should replace it with something like mCtx->SetMatrix(mCtx->GetMatrix().Copy().Translate().Rotate().Translate()) etc. I'd suggest we manipulate the Moz2D target directly and use AutoRestoreTransform (i.e. have an empty AutoRestoreTransform outside of the scope of this block, and Init it inside this block, see gfx/2d/Helpers.h). That'll prevent the wasteful full context save/restore pair in this situation as well.
Something like this? Note that I had to add the no-arg constructor to AutoRestoreTransform in Helpers.h, otherwise the compiler won't let me declare an empty instance.
Attachment #8500402 - Flags: review?(bas)
Attachment #8496825 - Attachment is obsolete: true
Attachment #8496825 - Flags: review?(bas)
Comment on attachment 8500402 [details] [diff] [review]
Support textruns with vertical writing modes when drawing Canvas2D text.

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

Looks good to me!
Attachment #8500402 - Flags: review?(bas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3250bad7f3c
Flags: needinfo?(bas)
Target Milestone: --- → mozilla35
Argh, warnings-as-errors strikes again. Relanded with fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/88bb2a142e10
https://hg.mozilla.org/mozilla-central/rev/88bb2a142e10
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1079409
No longer depends on: 1079409
We should either back this out or fix the crashers asap.
(I asked Tomcat to back out, but perhaps there is a crash fix already coming.)
(In reply to Olli Pettay [:smaug] from comment #17)
> We should either back this out or fix the crashers asap.
> (I asked Tomcat to back out, but perhaps there is a crash fix already
> coming.)

backed out per irc request (as smaug mentioned) in remote:   https://hg.mozilla.org/mozilla-central/rev/dd7637cc42d5 and will retrigger nightly builds
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
Comment on attachment 8500402 [details] [diff] [review]
Support textruns with vertical writing modes when drawing Canvas2D text.

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3507,5 @@
>    // offset pt.y based on text baseline
>    processor.mFontgrp->UpdateUserFonts(); // ensure user font generation is current
>    const gfxFont::Metrics& fontMetrics =
> +    processor.mFontgrp->GetFirstValidFont()->GetMetrics(
> +      processor.mTextRun->IsVertical() ? gfxFont::eVertical :

It turns out this isn't safe, as processor.mTextRun may not have been created yet; hence the crashes people have reported.

Instead, we can check the TEXT_ORIENTATION_MASK field of processor.mTextRunFlags to decide what to pass here.
Re-landed with fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d454aaf1dec
Target Milestone: --- → mozilla35
Depends on: 1079746
https://hg.mozilla.org/mozilla-central/rev/1d454aaf1dec
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.