Closed Bug 698185 Opened 13 years ago Closed 13 years ago

mixed-direction (bidi) text in <canvas> is broken

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

(Keywords: regression, rtl)

Attachments

(3 files, 2 obsolete files)

Loading the testcase layout/reftests/canvas/text-bidi-ltr-test.html should draw the text "helloשלוםgoodbye" to a canvas. However, in FF7.0 and later, only the word "hello" appears; the rest of the text is missing. This worked fine in earlier releases.

Regression range:
works: 2011-06-29 http://hg.mozilla.org/mozilla-central/rev/613c2f8b57e8
fails: 2011-06-30 http://hg.mozilla.org/mozilla-central/rev/d452aaf438f1

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=613c2f8b57e8&tochange=d452aaf438f1

Suspected culprit:
85e5b0ed7081	L. David Baron — Bug 667948: Convert canvas text measurement widths from app units to pixels *after* they switch from integers to floats. r=roc

(The bug number is wrong there, see bug 667947 comment 26.)

Note that on Win7/D2D, the regression occurs later, in the 2011-07-03 nightly, because of the separate Azure canvas implementation; see bug 667947 comment 28 and 30.
Attached patch possible fix (obsolete) — Splinter Review
Possible approach to fixing this, by adding the necessary scaling in nsBidiPresUtils::ProcessText(). Pushed to tryserver to see if anything else breaks...
Comment on attachment 570485 [details] [diff] [review]
possible fix

>-        aprocessor.DrawText(xOffset, width);
>+        aprocessor.DrawText(xOffset * app2dev, width * app2dev);

This seems like it's snapping text to the nearest pixel (since DrawText takes nscoord parameters), which we should avoid.
While this fixes the mixed-direction text in canvas, it also fails some tests from bug 570378 (having mixed direction runs in <img> alt-text), so this needs to be looked at further anyway.
Attachment #570662 - Flags: review?(dbaron)
This appears to fix the bug, and passes unit tests on try (modulo the usual intermittent oranges!).

However, I'm concerned about the scaling of posResolve values in nsBidiPresUtils::ProcessText ... it looks like these are supposed to be twips, but I don't see how the code would provide that. Moreover, changing the scaling here makes no difference to test results, implying that we have no test coverage that could tell us whether this is broken or not.
Attachment #570485 - Attachment is obsolete: true
Attachment #570664 - Flags: review?(dbaron)
Blocks: 707303
Comment on attachment 570664 [details] [diff] [review]
patch for the bidi brokenness

r=dbaron on the content/canvas/src/ changes

I believe the nsBidiPresUtils.cpp code looks correct as is, so you shouldn't change it.  BidiProcessor implementations (as of the patch that caused this regression) always return the width in twips (as opposed to using different units for different processors), which means the numbers in question are already in twips and don't need to be converted to them.  There's also only a single caller that uses that code:  nsTextBoxFrame::DrawText uses it to underline accesskeys correctly.  It looks to me like all other callers pass null for aPosResolve and don't get to that code.  So unless you've tested underlines of accesskeys, you're probably regressing something by changing the scaling in that code.

That said, if you've run reftests and not hit any failures, it would be really great if you could add a reftest for the xul textbox accesskey underlining. :-)


Sorry for the delay getting to this.

Please remove the "try:" syntax from your commit message before pushing to m-c.
Attachment #570664 - Flags: review?(dbaron) → review+
Comment on attachment 570662 [details] [diff] [review]
reftest for the bug here

r=dbaron
Attachment #570662 - Flags: review?(dbaron) → review+
You're right, no change is needed here in nsBidiPresUtils; however (as mentioned in comment 5) we have no test coverage of this code, so unit tests remain green whether it's correct or broken. :(

I'll put together a couple of reftests for this.
Note that the third testcase here, with the access key in RTL-overridden English text, currently fails - the underline does not appear in the correct position. This is unrelated to the patch here - it's a preexisting bug. It seems that our support for complex bidi within XUL <label>, at least, is less than complete.
Attachment #579796 - Flags: review?(dbaron)
Comment on attachment 579796 [details] [diff] [review]
reftests for access keys in label of xul textbox

r=dbaron

But do these tests start failing with your original nsBidiPresUtils patch?
Attachment #579796 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #10)
> Comment on attachment 579796 [details] [diff] [review] [diff] [details] [review]
> reftests for access keys in label of xul textbox
> 
> r=dbaron
> 
> But do these tests start failing with your original nsBidiPresUtils patch?

Well...yes, but not as clearly as I'd like: they actually generate an unexpected PASS from test #3, because the symptom of the bug is that the underline disappears completely in the bidi case. So to make that manifest as an unexpected failure, I'll add a notref (!=) test against a copy with no access key.
Adding != tests to detect if the underline disappears entirely from view in bidi cases. Just a minor extension of the earlier patch; carrying forward r=dbaron.
Assignee: nobody → jfkthame
Attachment #579796 - Attachment is obsolete: true
Attachment #579977 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: