115921 - Regression: BDO doesn't work in Latin-only text

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Simon Montagu :smontagu]

This is a spin-off from bug 9100. Testcases for BDO and RLO without any
right-to-left characters (e.g. attachment 61906 [details]) regressed between the 20011208
and 20011209 nightlies builds.

I have pinned this down to dbaron's checkin on nsCSSFrameConstructor.h and .cpp
from bug 109788: the testcase passes in a build from the 12/08 tarball, and if I
apply only that patch it fails.

What seems to be happening is that the bidiEnabled flag is getting set later
than before, and so it is not set at
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsBlockFrame.cpp#741
when we determine whether to do bidi reordering.

Comment 1

[Simon Montagu :smontagu]

Attachment: stacktrace of the first call to SetBidiEnabled in 20011208 nightly

Comment 2

[Simon Montagu :smontagu]

Attachment: stacktrace of the first call to SetBidiEnabled after applying patch

Comment 3

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

Style resolution is supposed to be able to happen lazily, but I guess this is an
optimization that we want.  I'm about to test the following patch:

Index: nsCSSFrameConstructor.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,v
retrieving revision 1.674
diff -u -d -r1.674 nsCSSFrameConstructor.cpp
--- nsCSSFrameConstructor.cpp	12 Dec 2001 12:52:55 -0000	1.674
+++ nsCSSFrameConstructor.cpp	19 Dec 2001 03:06:36 -0000
@@ -7119,6 +7119,15 @@
                               aFrameItems);
 
   nsIFrame* lastChild = aFrameItems.lastChild;
+
+  // Style resolution can normally happen lazily.  However, getting the
+  // Visibility struct can cause |SetBidiEnabled| to be called on the
+  // pres context, and this needs to happen before we start reflow, so
+  // do it now, when constructing frames.  See bug 115291.
+  {
+    const nsStyleVisibility *vis;
+    GetStyleData(styleContext, &vis);
+  }
 
   // Handle specific frame types
   nsresult rv = ConstructHTMLFrame(aPresShell, aPresContext, aState,

Comment 4

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

Actually, I'm moving that before the setting of |lastChild|.

Comment 5

[David Hyatt]

Why does letting SetBidiEnabled happen lazily mess things up?

Comment 6

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

We expect it to have happened if any of the inline descendants have a
'direction' set when we begin reflowing a block, presumably.

Comment 7

[David Hyatt]

But shouldn't enabling Bidi (even lazily) trigger a full reflow that will then
fix things up?

Comment 8

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

Attachment: patch

Yep, this works, as I expected.

Comment 9

[David Hyatt]

sr=hyatt

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 62162 [details] [diff] [review]
patch

looks good to me. r=smontagu

Comment 11

[Simon Montagu :smontagu]

I'd like to add a test for this to the layout regression tests. Do I need
anybody's permission to do that? I would need to check in the testcase to
mozilla/layout/html/tests/block/bugs and add the URL to
mozilla/layout/html/tests/block/bugs/file_list.txt, right? Anything else?

Comment 12

[Brendan Eich [:brendan]]

So what's the better long-term fix?  Is it what hyatt implies in comment #7, or
is this patch the right short and long-term solution?

/be

Comment 13

[Simon Montagu :smontagu]

I don't think that enabling Bidi should trigger a reflow. In the vast majority
of situations, it is enabled either before layout starts, when right-to-left
characters are encountered in the DOM, or after the initial layout, if a change
in preferences (e.g. changing page direction) requires it. In the second case
the reflow is triggered anyway by preference observers.

The case here of Latin-only text with <BDO dir="rtl"> is not something that ever
happens in real-world examples. As I said in
http://bugzilla.mozilla.org/show_bug.cgi?id=9100#c41 I would call it WONTFIX,
were it not for the fact that it keeps coming up in HTML4 test suites. Standards
compliance, like justice, has to be seen to be done.

Comment 14

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

Attachment 62162 [details] [diff] checked in 2002-01-06 10:25 PDT.