Closed Bug 370589 Opened 15 years ago Closed 15 years ago

Problems when trees paint bidi text with no other bidi text in the window

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(1 file, 1 obsolete file)

This only shows up in a new-textrun platform (currently, Linux only). The problem is that nsLayoutUtils::DrawString and GetStringWidth check presContext->BidiEnabled() before doing bidi resolution on the string, but when trees are painting their contents (which aren't necessarily in the DOM), they could be painting bidi text even though presContext->BidiEnabled() returns false. Consequently we can end up with a mixed-direction string being passed to the text run which is wrong.
Attached patch fix (obsolete) — Splinter Review
Simple fix: anything that could be dealing with non-DOM text (i.e., trees) needs to tell that to nsLayoutUtils::GetStringWidth/DrawString to bypass the presContext->BidiEnabled check.
Attachment #255308 - Flags: superreview?(dbaron)
Attachment #255308 - Flags: review?(dbaron)
I'm not sure this is the right fix. What is the case where it breaks? Don't XUL elements have their own presContext, different from the DOM presContext? nsTextBoxFrame sets the Bidi enabled flag in its presContext when necessary, and it seems to me that if nsTreeBodyFrame isn't doing that, then that is the bug that should be fixed.
In fact, I originally wrote it that way, in attachment 43995 [details] [diff] [review] to bug 81032, but it got rejected for performance considerations.
Comment on attachment 255308 [details] [diff] [review]
fix

OK. XUL elements don't have their own prescontext, but I think you're right. Trees should detect bidi characters (assuming the macros here are still adequate:
http://lxr.mozilla.org/seamonkey/source/layout/base/nsBidiUtils.h#240
) and enable bidi in the prescontext.
Attachment #255308 - Flags: superreview?(dbaron)
Attachment #255308 - Flags: review?(dbaron)
Okay, this seems better.
Attachment #255308 - Attachment is obsolete: true
Attachment #255647 - Flags: superreview?(dbaron)
Attachment #255647 - Flags: review?(smontagu)
Comment on attachment 255647 [details] [diff] [review]
Enable prescontext bidi mode via XUL trees when necessary

Why not just do what nsTextFragment::SetBidiFlag does?  Or maybe even move it somewhere that both places can use it?

Either way, sr=dbaron (especially if smontagu is happy with it).
Attachment #255647 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 255647 [details] [diff] [review]
Enable prescontext bidi mode via XUL trees when necessary

I'm also OK either way.

>+    if (ch >= 0xD800 || IS_IN_BMP_RTL_BLOCK(ch)) {
>+      maybeRTL = PR_TRUE;
>+    }

Nit: put in a |break| here.
Attachment #255647 - Flags: review?(smontagu) → review+
The nsTextFragment::SetBidiFlag code is precise; we avoid calling it for 8-bit characters, so performance isn't so critical. The tree code I wrote is imprecise and we have to call it for all strings because we don't know a priori whether the string is all 8-bit or not. So there isn't a great fit there. Maybe this is a premature optimization but I guessed not based on comment #3.
checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 255647 [details] [diff] [review]
Enable prescontext bidi mode via XUL trees when necessary

>+  PRBool maybeRTL = PR_FALSE;
>+  for (i = 0; i < length; ++i) {
>+    PRUnichar ch = text[i];
>+    if (ch >= 0xD800 || IS_IN_BMP_RTL_BLOCK(ch)) {
>+      maybeRTL = PR_TRUE;
>+    }
>+  }
>+  if (!maybeRTL)
>+    return;
>+
>+  GetPresContext()->SetBidiEnabled(PR_TRUE);
Personally I'd have gone for this:
for (i = 0; i < length; ++i) {
  PRUnichar ch = text[i];
  if (ch >= 0xD800 || IS_IN_BMP_RTL_BLOCK(ch)) {
    GetPresContext()->SetBidiEnabled(PR_TRUE);
    return;
  }
}
r+sr for that change if you want to land it
You need to log in before you can comment on or make changes to this bug.