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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 2

[Simon Montagu :smontagu]

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.

Comment 3

[Simon Montagu :smontagu]

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 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Enable prescontext bidi mode via XUL trees when necessary

Okay, this seems better.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

[Simon Montagu :smontagu]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 10

[[e-mail]]

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;
  }
}

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

r+sr for that change if you want to land it