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)

defect

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.
Blocks: text-dir
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,
Actually, I'm moving that before the setting of |lastChild|.
Why does letting SetBidiEnabled happen lazily mess things up?
We expect it to have happened if any of the inline descendants have a 'direction' set when we begin reflowing a block, presumably.
But shouldn't enabling Bidi (even lazily) trigger a full reflow that will then fix things up?
Attached patch patch (deleted) — Splinter Review
Yep, this works, as I expected.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
sr=hyatt
Comment on attachment 62162 [details] [diff] [review] patch looks good to me. r=smontagu
Attachment #62162 - Flags: review+
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? /be
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.
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.

Attachment

General

Created:
Updated:
Size: