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?

Updated to fix a crash when the Canvas element is not in a document.

Updated to include support for drawing right-to-left textruns to canvas2d.

Rebased this patch following bug 927892. (Also depends on review-pending patches in bug 902762.)

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.

Bas, review ping?

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.

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!

Reverted for: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2793540&repo=mozilla-inbound https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2793544&repo=mozilla-inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2be160148953

Argh, warnings-as-errors strikes again. Relanded with fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/88bb2a142e10

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

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