Last Comment Bug 698185 - mixed-direction (bidi) text in <canvas> is broken
: mixed-direction (bidi) text in <canvas> is broken
Status: RESOLVED FIXED
: regression, rtl
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
: 707303 708259 (view as bug list)
Depends on:
Blocks: 667947 707303
  Show dependency treegraph
 
Reported: 2011-10-29 02:59 PDT by Jonathan Kew (:jfkthame)
Modified: 2011-12-09 07:04 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible fix (3.83 KB, patch)
2011-10-29 09:20 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
reftest for the bug here (1.89 KB, patch)
2011-10-31 05:14 PDT, Jonathan Kew (:jfkthame)
dbaron: review+
Details | Diff | Splinter Review
patch for the bidi brokenness (5.16 KB, patch)
2011-10-31 05:18 PDT, Jonathan Kew (:jfkthame)
dbaron: review+
Details | Diff | Splinter Review
reftests for access keys in label of xul textbox (8.84 KB, patch)
2011-12-07 12:50 PST, Jonathan Kew (:jfkthame)
dbaron: review+
Details | Diff | Splinter Review
reftests for access keys in label of xul textbox - updated (13.74 KB, patch)
2011-12-08 00:00 PST, Jonathan Kew (:jfkthame)
jfkthame: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-10-29 02:59:07 PDT
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) 2011-10-29 09:20:48 PDT
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 2 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-10-29 10:15:48 PDT
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) 2011-10-29 15:41:59 PDT
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) 2011-10-31 05:14:47 PDT
Created attachment 570662 [details] [diff] [review]
reftest for the bug here
Comment 5 Jonathan Kew (:jfkthame) 2011-10-31 05:18:27 PDT
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.
Comment 6 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-12-06 16:20:10 PST
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: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-12-06 16:22:43 PST
Comment on attachment 570662 [details] [diff] [review]
reftest for the bug here

r=dbaron
Comment 8 Jonathan Kew (:jfkthame) 2011-12-07 04:50:45 PST
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) 2011-12-07 12:50:32 PST
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.
Comment 10 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-12-07 13:17:44 PST
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) 2011-12-07 23:58:03 PST
(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) 2011-12-08 00:00:41 PST
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.
Comment 13 Jonathan Kew (:jfkthame) 2011-12-08 01:46:08 PST
*** Bug 708259 has been marked as a duplicate of this bug. ***
Comment 15 David Anderson [:dvander] 2011-12-08 19:26:05 PST
*** Bug 707303 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.