402276 - [RTL] Canvas text routines draw right-to-left text backwards

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect, P5)

Description

[Tomer Cohen :tomer]

Attachment: Places Organizer

When launching the Places Organizer in Minefield, we get the text for "Preview Image" (alt?) reversed. This is similar to old bug cause the alternative text to render reversed in Firefox 2 under Linux, but that bug have been resolved for Cairo. 

The text appear in the screenshot as "המידקמ הגוצת", and should be "תצוגה מקדימה".

Comment 1

[Axel Hecht [:Pike]]

Moving over a flock of granparadiso RTL bugs to Firefox for triage.

Comment 2

[Simon Montagu :smontagu]

Attachment: testcase

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Super low priority, and it probably won't get fixed for Fx3.  It would be better if the places UI were reworked so that this wasn't an issue -- like using a real XUL Label that's swapped into place instead of a canvas.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

gfxTextRun does not handle bidi itself. You have to give it a direction. But it can only handle a single direction and in general you can have mixed-direction text, so you can't solve this in gfxTextRun.

There are nsBidiPresUtils methods that XUL labels use to do bidi resolution and rendering, you could build on that, but really we need a better utility class. Say something that takes a string and returns a collection of gfxTextRuns or some new class encapsulating such a collection. SVG needs this too.

For this bug I would suggest not rendering "Preview Image" in the canvas itself, but as a XUL label.

Comment 5

[Simon Montagu :smontagu]

(In reply to comment #4)
> There are nsBidiPresUtils methods that XUL labels use to do bidi resolution and
> rendering, you could build on that, but really we need a better utility class.
> Say something that takes a string and returns a collection of gfxTextRuns or
> some new class encapsulating such a collection.

Let's target that for after Fx3. We would probably also want to return some kind of logical<->visual map.

> SVG needs this too.

Would taking a string be enough for SVG? I suspect we would need to handle all the complications of overlapping elements and directional runs that make ResolveBidi such a mess.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think for SVG 1.1 we might be able to get away with building a string from the SVG elements, along with a map between string characters and elements, then convert the string to a set of textruns plus a map between string characters and textrun characters. Of course that's heading towards a general inline layout engine and for SVG 1.2 we'd certainly need one.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note for bugs that are being marked as blocking: this will not be fixed for Firefox 3/Gecko 1.9.

Comment 8

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 9

[Eric Butler [:ebutler]]

There is a new text API for canvas (conforming to the HTML 5 draft) which renders RTL text correctly. This should be used in favor of the old moz text functions, so marking WONTFIX.

Comment 10

[Simon Montagu :smontagu]

Attachment: Previous testcase rewritten with new API

RTL text doesn't work correctly with the new API either.

Comment 11

[Simon Montagu :smontagu]

I think it makes more sense to morph this to apply to the new text API rather than opening a new bug. The solution suggested in comment 4 is still what it is needed, AFAICT.

Comment 12

[Simon Montagu :smontagu]

Attachment: Testcase with mixed-direction text and direction setter

Comment 13

[Simon Montagu :smontagu]

nsCanvasRenderingContext2D::drawText is setting TEXT_IS_RTL for all text runs when the direction of the canvas is RTL, but these are two different things: the direction property determines the base direction of the text for bidi resolution, and the text run flag should only be set for text runs containing right-to-left text after bidi resolution.

I also think it's wrong that the x argument to FillText() and StrokeText() is in left-to-right coordinates when the canvas is right-to-left, but that's a problem with the API itself, not our implementation.

Comment 14

[Eric Butler [:ebutler]]

Attachment: ver 0.1

A proposed patch that exposes the function in nsBidiPresUtils to allow canvas to do bidi resolution. The patch creates a small interface so that any implementing class can do bidi resolution and receive the text runs one at a time. Could potentially be used by SVG eventually as well.

Still pretty rough, but all the test cases now render correctly. Contains a reftest as well.

One notable change is the removal of the nsIRenderingContext::GetHints call from nsBidiPresUtils::ProcessText, since (according to Vlad) all nsIRenderingContexts return the same values for that anyway. There is still some cruft in the ProcessText function from this that needs to be removed.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+		-I$(srcdir)/../../../view/public \

You shouldn't need this if you've got 'view' in REQUIRES.

You're not passing in the right initial direction to MeasureText, and you may be able to usefully share some code around the call to ProcessText.

Removing the hints call is good, that's legacy cruft now.

I think I'd rather have the visitor take an integer offset than a PRUnichar*, to make it clear that we're indexing into the string passed to ProcessText and not passing in something random.

I think the overall visitor design is reasonable, although I don't think we should call it a visitor because it does more than a visitor should --- its results are essential to the operation of ProcessText.

isCahcedWidthValid --- typo. Also instead of doing caching in the visitor it would be simpler to guarantee that after a SetText call, there is at most one SetWidth and at most one DrawText call before the next SetText, and do any necessary caching in ProcessText.

I thought maybe we should get rid of SetText and just pass string arguments to GetWidth and DrawText (and have DrawText return the width as well as drawing), but I guess this design gives more flexibility to ProcessText because it can get the width and then decide where to draw, and it's really no harder for the implementors. So this is fine.

Comment 16

[Eric Butler [:ebutler]]

Attachment: ver 0.2

Improved version, includes corrections for most of roc's suggestions. Also rewrote nsCanvasRenderingContext2D::MeasureText to use the DrawText function.

I was unable to change SetText to take an index, because it appears that ProcessText copies and modifies the string passed to it, and then passes that one back to the visitor.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So it does. The main reason is numeral conversion, which strictly speaking wouldn't be necessary anymore after bug 441782 is fixed, but then we'd have to propagate numeral context information through your new interface, which would suck. So let's leave that as-is for now.

Comment 18

[Eric Butler [:ebutler]]

Attachment: ver 1.0

Improved patch. Updated comments, fixed a few problems with cairo paths, and cleaned up code a bit.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsCanvasRenderingContext2D::DrawText should be called DrawOrMeasureText.

+    nsresult rv;
+    float width;
+
+    rv = DrawText(rawText, 0, 0, 0, TEXT_DRAW_OPERATION_MEASURE, &width);

"nsresult rv = ..."

+    nsTextMetrics *textMetrics = new nsTextMetrics(width);
+    if (!textMetrics)
+        return NS_ERROR_OUT_OF_MEMORY;
+
+    NS_ADDREF(*_retval = textMetrics);

Prefer nsRefPtr<nsTextMetrics> textMetrics = ...;
...
  *_retval = textMetrics.forget();

+    virtual ~nsCanvasBidiProcessor() { }

Why do you need a virtual destructor? Also make nsCanvasBidiProcessor an NS_STACK_CLASS.

I see that Vlad has corrupted you into not following the a* convention for parameters. That actually sucks, but I won't raise it now. However, please give m* names for the nsCanvasBidiProcessor fields, and use names like "mFontGroup" and "mAppUnitsPerDevPixel". Also you don't need a 'length' field, you can get it from the textrun. Ditto for "direction".

"(nsBidiDirection) (level & 1)" should be a constructor-style cast.

+  nsIRenderingContextBidiProcessor(nsIRenderingContext* ctx,
+                                 nscoord x,
+                                 nscoord y)

fix indent? Pass const nsPoint& and store nsPoint instead of two nscoords.

Fix the naming of the nsIRenderingContextBidiProcessor stuff --- a* for parameters, m* for fields.

Otherwise looks good.

Comment 20

[Eric Butler [:ebutler]]

Attachment: ver 1.1

Fixed all of roc's suggestions.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // dev pixel conversion factor
+    PRUint32 mAppUnitsPerDevPixel;

You can actually get this from the textrun too. Sorry I didn't notice that the first time.

Otherwise good.

Comment 22

[Eric Butler [:ebutler]]

+    // dev pixel conversion factor
+    PRUint32 mAppUnitsPerDevPixel;

Well, that member is actually required because its passed to MakeTextRun. It's initialized only once, before any text runs are created. It seemed like a better idea to put it in the struct rather than fetch it from the nsPresContext every time its needed.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 329709 [details] [diff] [review]
ver 1.1

Oops right.

Comment 24

[Simon Montagu :smontagu]

Comment on attachment 329709 [details] [diff] [review]
ver 1.1

>+nsCanvasRenderingContext2D::DrawOrMeasureText(const nsAString& aRawText,

>     if (isRTL)
>-        pt.x += (1 - anchorX) * textWidth;
>+        processor.mPt.x += (1 - anchorX) * totalWidth;
>     else
>-        pt.x -= anchorX * textWidth;
>+        processor.mPt.x -= anchorX * totalWidth;

You don't want the |if (isRTL)| part of this any more, because ProcessText fixes up the coordinates for RTL already.

A few nits:

>+    // if text is over aMaxWidth, then scale the text horizonally such that its

"horizontally".

The references to aX in the comments to ProcessText in nsBidiPresUtils.cpp need to be updated.

Can you add a version of the reftest with "direction: rtl;"?

Comment 25

[Eric Butler [:ebutler]]

Attachment: ver 1.2

added changes for suggestions, and corrected another erroneous reftest.

Comment 26

[Eric Butler [:ebutler]]

Attachment: ver 1.3

Added exceptions for the mac to the reftests since they fail on the mac only due to some bizarre pixel rounding with the text.

Comment 27

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/7221a9a9caf7

Comment 28

[Tony Chung [:tchung]]

(In reply to comment #12)
> Created an attachment (id=327744) [details]
> Testcase with mixed-direction text and direction setter

Testcase Fix verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081023 Minefield/3.1b2pre