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

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: possible fix

Possible approach to fixing this, by adding the necessary scaling in nsBidiPresUtils::ProcessText(). Pushed to tryserver to see if anything else breaks...

Comment 2

[David Baron :dbaron:]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: reftest for the bug here

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch for the bidi brokenness

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.

Comment 6

[David Baron :dbaron:]

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.

Comment 7

[David Baron :dbaron:]

Comment on attachment 570662 [details] [diff] [review]
reftest for the bug here

r=dbaron

Comment 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: reftests for access keys in label of xul textbox

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.

Comment 10

[David Baron :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?

Comment 11

[Jonathan Kew [:jfkthame]]

(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.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: reftests for access keys in label of xul textbox - updated

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.

Comment 14

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8a2699a99d (patch)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9206bb3f87 (canvas reftest)
https://hg.mozilla.org/integration/mozilla-inbound/rev/35196e3bbb3a (accesskey reftests)

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7c8a2699a99d
https://hg.mozilla.org/mozilla-central/rev/ec9206bb3f87
https://hg.mozilla.org/mozilla-central/rev/35196e3bbb3a