Closed
Bug 115921
Opened 23 years ago
Closed 23 years ago
Regression: BDO doesn't work in Latin-only text
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla0.9.8
People
(Reporter: smontagu, Assigned: dbaron)
References
Details
Attachments
(3 files)
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.
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 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 @@
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,
Assignee | ||
Comment 4•23 years ago
|
||
Actually, I'm moving that before the setting of |lastChild|.
Comment 5•23 years ago
|
||
Why does letting SetBidiEnabled happen lazily mess things up?
Assignee | ||
Comment 6•23 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•23 years ago
|
||
But shouldn't enabling Bidi (even lazily) trigger a full reflow that will then
fix things up?
Assignee | ||
Comment 8•23 years ago
|
||
Yep, this works, as I expected.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
Comment 9•23 years ago
|
||
sr=hyatt
Reporter | ||
Comment 10•23 years ago
|
||
Comment on attachment 62162 [details] [diff] [review]
patch
looks good to me. r=smontagu
Attachment #62162 -
Flags: review+
Reporter | ||
Comment 11•23 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?
Comment 12•23 years ago
|
||
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
Reporter | ||
Comment 13•23 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
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.
Assignee | ||
Comment 14•23 years ago
|
||
Attachment 62162 [details] [diff] checked in 2002-01-06 10:25 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•