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

RESOLVED FIXED in mozilla11

Status

()

Core
Canvas: 2D
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

({regression, rtl})

Trunk
mozilla11
regression, rtl
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 570485 [details] [diff] [review]
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 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.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 570662 [details] [diff] [review]
reftest for the bug here
Attachment #570662 - Flags: review?(dbaron)
(Assignee)

Comment 5

6 years ago
Created attachment 570664 [details] [diff] [review]
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.
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+
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 579796 [details] [diff] [review]
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.
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+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 579977 [details] [diff] [review]
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.
Assignee: nobody → jfkthame
Attachment #579796 - Attachment is obsolete: true
Attachment #579977 - Flags: review+
(Assignee)

Updated

6 years ago
Duplicate of this bug: 708259
(Assignee)

Comment 14

6 years ago
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)
Target Milestone: --- → mozilla11
Duplicate of this bug: 707303
https://hg.mozilla.org/mozilla-central/rev/7c8a2699a99d
https://hg.mozilla.org/mozilla-central/rev/ec9206bb3f87
https://hg.mozilla.org/mozilla-central/rev/35196e3bbb3a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.