333500 - Don't set NS_FRAME_IS_BIDI too enthusiastically

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

Spun off from bug 333433.

------- Comment #4 From Uri Bernstein 2006-04-10 08:19 PDT [reply] -------
On a side note, I question the usefulness of the NS_FRAME_IS_BIDI flag, as in
all cases I know of, it seems to be set for all the frames on a page which
contains any RTL at all.

------- Comment #7 From Simon Montagu  2006-04-10 09:27 PDT  [reply] -------
It shouldn't be being set on frames in an LTR-only paragraph even if other
paragraphs contain RTL. Is it?

------- Comment #8 From Uri Bernstein 2006-04-10 09:51 PDT [reply] -------
I double-checked, and it is. Resolve() is called on the LTR-only paragraph
(should it be?), and it calls AdjustOffsetsForBidi(), which sets the flag.

------- Comment #9 From Simon Montagu  2006-04-10 10:01 PDT  [reply] -------
That seems wrong to me. I think EnsureBidiContinuation() should be setting it.

------- Comment #11 From Uri Bernstein 2006-04-10 10:21 PDT [reply] -------
I tracked this back to the patch on bug 74946, which added setting the flag on
both branches of the |if ( (runLength > 0) && (runLength < fragmentLength) )|.
As far as I can tell, the behaviour didn't change ever since that patch was
landed.

------- Comment #12 From Simon Montagu 2006-04-10 11:26 PDT [reply] -------
I still think it's wrong :) The bit was originally supposed to mean "this frame
either is a nextBidi or has a nextBidi"[1], though it is overloaded for
TextBoxFrames to mean "this frame has some RTL content". Let's continue this
discussion in a new bug.

[1]http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsIFrame.h&rev=3.316&mark=213-214#211

Comment 1

[Simon Montagu :smontagu]

Another case where (IMO) the bit shouldn't be set is something like:

<span>foo</span><span>bar BAZ</span>

(where BAZ represents RTL text)

There shouldn't be any reason for foo to have NS_FRAME_IS_BIDI set.

Comment 2

[Uri Bernstein]

I still thing we should consider whether we need this bit at all.
Suppose we get rid of it (and behave as if it's set whenever the document has RTL). The overall behaviour will be identical to what we have today. What would we be losing (compared to setting the flag only when necessary)? Performance? Is it significant?

Comment 3

[Simon Montagu :smontagu]

I think there is a potential performance win in only setting the bit when it should be set, e.g. on pages that are mostly LTR with one or two words in RTL. This typically occurs on pages with links to all the different localized versions of the same content. Wikipedia pages are an obvious example.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.