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

RESOLVED FIXED in mozilla0.9.8



17 years ago
17 years ago


(Reporter: smontagu, Assigned: David Baron)



Firefox Tracking Flags

(Not tracked)



(3 attachments)



17 years ago
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
when we determine whether to do bidi reordering.

Comment 1

17 years ago
Created attachment 62143 [details]
stacktrace of the first call to SetBidiEnabled in 20011208 nightly

Comment 2

17 years ago
Created attachment 62144 [details]
stacktrace of the first call to SetBidiEnabled after applying patch


17 years ago
Blocks: 9100

Comment 3

17 years ago
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 @@
   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

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

Comment 5

17 years ago
Why does letting SetBidiEnabled happen lazily mess things up?


Comment 6

17 years ago
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

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

Comment 8

17 years ago
Created attachment 62162 [details] [diff] [review]

Yep, this works, as I expected.


17 years ago
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8

Comment 9

17 years ago

Comment 10

17 years ago
Comment on attachment 62162 [details] [diff] [review]

looks good to me. r=smontagu
Attachment #62162 - Flags: review+

Comment 11

17 years ago
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?
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?


Comment 13

17 years ago
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 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

17 years ago
Attachment 62162 [details] [diff] checked in 2002-01-06 10:25 PDT.
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.