Closed Bug 402276 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Canvas: 2D, defect, P5)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: tomer, Assigned: ebutler)

References

()

Details

(Keywords: intl, rtl)

Attachments

(5 files, 5 obsolete files)

Attached image 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 "תצוגה מקדימה".
Moving over a flock of granparadiso RTL bugs to Firefox for triage.
Component: he-IL / Hebrew → General
Keywords: intl
Product: Mozilla Localizations → Firefox
Version: unspecified → Trunk
Component: General → Layout: Canvas
Product: Firefox → Core
QA Contact: hebrew.he → layout.canvas
Summary: [RTL] Places Organizer has reversed text when no preview image available → [RTL] MozDrawText in Canvas draws right-to-left text backwards
Attached file testcase
Flags: blocking1.9?
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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P5
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.
(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.
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.
Flags: blocking1.9+ → blocking1.9-
Blocks: fx3-l10n-he
Note for bugs that are being marked as blocking: this will not be fixed for Firefox 3/Gecko 1.9.
Blocks: fx35-l10n-fa
No longer blocks: Persian-Fx3.5
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Blocks: svg-bidi
No longer blocks: fx35-l10n-fa
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
No longer blocks: svg-bidi
RTL text doesn't work correctly with the new API either.
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: [RTL] MozDrawText in Canvas draws right-to-left text backwards → [RTL] Canvas text routines draw right-to-left text backwards
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.
Assignee: vladimir → ebutler
Status: REOPENED → NEW
Attached patch ver 0.1 (obsolete) — Splinter Review
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.
+		-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.
Attached patch ver 0.2 (obsolete) — Splinter Review
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.
Attachment #329006 - Attachment is obsolete: true
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.
Attached patch ver 1.0 (obsolete) — Splinter Review
Improved patch. Updated comments, fixed a few problems with cairo paths, and cleaned up code a bit.
Attachment #329116 - Attachment is obsolete: true
Attachment #329549 - Flags: superreview?(roc)
Attachment #329549 - Flags: review?(smontagu)
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.

Attached patch ver 1.1 (obsolete) — Splinter Review
Fixed all of roc's suggestions.
Attachment #329549 - Attachment is obsolete: true
Attachment #329549 - Flags: superreview?(roc)
Attachment #329549 - Flags: review?(smontagu)
Attachment #329709 - Flags: superreview?(roc)
Attachment #329709 - Flags: review?(smontagu)
+    // 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.
+    // 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 on attachment 329709 [details] [diff] [review]
ver 1.1

Oops right.
Attachment #329709 - Flags: superreview?(roc) → superreview+
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;"?
Attachment #329709 - Flags: review?(smontagu) → review-
Attached patch ver 1.2 (obsolete) — Splinter Review
added changes for suggestions, and corrected another erroneous reftest.
Attachment #329709 - Attachment is obsolete: true
Attachment #329873 - Flags: review?(smontagu)
Attachment #329873 - Flags: review?(smontagu) → review+
Attached patch ver 1.3Splinter Review
Added exceptions for the mac to the reftests since they fail on the mac only due to some bizarre pixel rounding with the text.
Attachment #329873 - Attachment is obsolete: true
Attachment #330119 - Attachment is patch: true
Attachment #330119 - Attachment mime type: application/octet-stream → text/plain
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/7221a9a9caf7
Status: NEW → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.