User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
when applying a certain style (even no style caused it) to the :first-letter,
and applying it over hebrew text the text dissapears.
It happens only when you display the text with Hebrew encoding, non Hebrew one
displays the text correctly (but NOT bidified so the Bidi algorithm probably
Works with MS-IE
Steps to Reproduce:
1.load a problematic pagee
Mozilla won't display the Hebrew text
it should display it
an Example for problematic simplest web page:
בוא נראה אם אפשר גם בעברית (SOME HEBREW TEXT)
<div>aaaa aaa aaa</div>
Confirming what? This bug needs a testcase attached to the bug (the pasted-in
HTML doesn't even say what encoding it's in, so can't be used to test, per
comment 0) to be remotely fixable.
Created attachment 145933 [details]
Boris, try to compare this testcase in Mozilla and IE6 and you'll see the bug.
Created attachment 145934 [details]
testcase (with proper encoding)
Wow. This one is pretty bad.
The problem here is that nsFirstLetterFrame does not implement InsertFrames, so
when CreateBidiContinuation tries to use that method, the frames just get lost
(and some nice asserts fire in nsFrame::InsertFrames).
Shouldn't nsContainerFrame implement this method, if nothing else? But even if
that were happening, I suspect sticking random content into a first-letter frame
would be bad.... chances are, the bidi code needs to blow away the letter frames
and then rewrap after creating bidi continuations, right?
I don't see why CreateBidiContinuation needs to do anything to the
nsFirstLetterFrame in the first place. Assuming that the content of an
nsFirstLetterFrame is only one character long, it can only be in one direction, no?
> Assuming that the content of an nsFirstLetterFrame is only one character long
That assumption fails, eg, when there is a quote at the beginning of the line...
But in this case there is no such. And no, I don't know why
CreateBidiContinuation is messing with this frame. It would help if it were
documented in this code precisely what it's trying to accomplish and why...
(In reply to comment #2)
> Confirming what?
Umm, yeah, sorry. I guess I must have been in a bit of a hurry when I confirmed
(In reply to comment #8)
> > Assuming that the content of an nsFirstLetterFrame is only one character long
> That assumption fails, eg, when there is a quote at the beginning of the line...
So in that case the quote and the first alphabetical character should both be
rendered with the first-letter style? Does any spec address what should happen
when they are not contiguous after reordering? Example:
<p dir="ltr">"YERUSHALAYIM" is the Hebrew for Jerusalem.</p>
Maybe that is getting off-topic, though.
> Does any spec address
Maybe. In a very wacky way. I've posted a question on the subject to
www-style. See http://lists.w3.org/Archives/Public/www-style/2004Apr/0244.html
It's not that off-topic, since it raises the question of just _what_ is
considered the "first letter" in bidi situations and hence what we should
attempt to do.
> Assuming that the content of an nsFirstLetterFrame is only one character long
Maybe there is another problem with this assumption. The way we find the length
of the frame's content, at
to do GetContent() on the frame, QI to an nsITextContent, do GetText() on that,
and then GetLength() on the returned nsTextFragment. I suspect that this is
returning the length of the entire content node, not just the first-letter part
> I suspect that this is returning the length of the entire content node
Yes, it is. Isn't this also a problem when linebreaking happens? Or do you
want to include the content of the continuations in those cases, I guess?
The reason I ask is that nsTextFrame does have an mContentLength member we could
use if we want the actual content length of that one frame...
>Isn't this also a problem when linebreaking happens?
I think that Bidi resolution always happens before linebreaking, but I am
getting less and less sure of my ground here ;-)
The issue bz raised was solved:
Hixie: "Similarly, if the first letter(s) of the block are not at the start of
the line (for example due to bidirectional reordering), then the UA need not
create the pseudo-element(s)."
... this will be added in the next CSS 2.1 draft.
Created attachment 215872 [details] [diff] [review]
This works around the fact that nsFirstLetterFrame does not implement InsertFrames() by splitting the nsFirstLetterFrame first, and then using SetInitialChildList() to populate the nsFirstLetterFrame continuation with the text continuation.
Alternatrively, we could do what comment #6 suggests (inplement InsertFrames() in nsFirstLetterFrame). I'm only requesting sr now, so we can decide on the preferred approach.
To answer a question raised above:
First-letter is handled in two phases: first, the entire text frame which should be first-lettered is wrapped in a nsFirstLetterFrame, and only later it is split into a frame containing the first letter itself, and a continuation frame, containing the rest of the text.
Bidi resolution happens between these two stages, that is, when there is one nsFirstLetterFrame, containing the entire text (which is potentially bidi). Therefore, this frame must be split by bidi resolution.
The additional splitting of the first letter itself from the rest of the text only happens on the primary frame, which is why creating bidi nsFirstLetterFrame continuations is fine: the first-letter style is not applied to the first letter of each bidi run, but only to the actual first letter.
Fixing this (using either approach) would also fix bug 264937. Since this bug has more discussion I posted the patch here, so I'm reversing the dependency.
Would the bidi splits for the first-letter frame stick around in the frame tree? If so, would nsCSSFrameConstructor::RemoveLetterFrames still work right?
Also, given that we split inlines would we need to split the letter frame even if it implemented InsertFrames?
(In reply to comment #19)
> Would the bidi splits for the first-letter frame stick around in the frame
> If so, would nsCSSFrameConstructor::RemoveLetterFrames still work right?
As far as I can tell, yes. It uses RemoveFrame() to get rid of the continuations, and RemoveFrame now removes the bidi continuation(s) as well (see bug 299065 comment 61).
> Also, given that we split inlines would we need to split the letter frame even
> if it implemented InsertFrames?
Yes. (Good point, I haven't thought of it).
It seems to me that the right thing to do then is to implement InsertFrames (so we don't have to special-case the letter frame here) and then just use the normal bidi code for splitting...
(In reply to comment #21)
> It seems to me that the right thing to do then is to implement InsertFrames (so
> we don't have to special-case the letter frame here) and then just use the
> normal bidi code for splitting...
So should it be implemented in nsFirstLetterFrame, or,as you suggested in comment #6, in nsContainerFrame?
If it's implemented in nsContainerFrame, would the implementation be any different than the one currently in nsInlineFrame? If not, should I just move the implementation from nsInlineFrame to nsContainerFrame?
I'd put it on container frame unless there are good reasons not to. SetInitialChildList and GetFirstChild are there already, so...
The dirty-child reflow setup would be different in nsContainerFrame and nsInlineFrame, I would think.
In fact, perhaps that's the argument for not putting AppendFrames, etc on nsContainerFrame -- it doesn't really know how to reflow itself well. But is that changing on reflow branch? That is, maybe we should put the code on letterframe right now (with asserts that the frame list is nextBidi?) and then revisit this in a followup that depends on reflow branch?
Comment on attachment 215872 [details] [diff] [review]
I'll wait with posting a new patch until these questions are answered (by dbaron, I guess).
Nominating for blocking 1.9a1 so that this doesn't fall off the radar.
Created attachment 217737 [details] [diff] [review]
Perhaps asking in the form of a review request would be more useful.
Created attachment 231959 [details]
The behavior has been change on Trunk, but is still broken.
Look at the screenshot for details. (comparison between Trunk and Firefox 18.104.22.168)
The 2nd line of the testcase looks good on Trunk.
So, now that the reflow branch has landed, maybe I should try answering these questions:
I think moving nsInlineFrame::InsertFrames, AppendFrames, and RemoveFrames to nsContainerFrame makes sense, except that I'm really not a big fan of the nextBidi hack. Most callers of InsertFrames, RemoveFrames, etc., go through the frame manager; this one is already different by not doing so.
Then again, what we really need to do is make bidi reordering happen at frame construction time rather than during reflow -- and then we wouldn't even have to worry about suppressing anything.
So I suppose moving the three methods to nsContainerFrame is fine, but I'd like to see more of this stuff happen during frame construction rather than reflow. (Same for ::first-letter.)
I'm a little uncomfortable with the patch above since implementing InsertFrames without AppendFrames and RemoveFrames seems broken.
(In reply to comment #28)
> I think moving nsInlineFrame::InsertFrames, AppendFrames, and RemoveFrames to
> nsContainerFrame makes sense,
I'll try to make a patch sometime soon (after I fully recover from a disk crash, and find some spare time).
> except that I'm really not a big fan of the nextBidi hack.
I'm not sure what is the "nextBidi hack" you're referring to.
> Then again, what we really need to do is make bidi reordering happen at frame
> construction time rather than during reflow -- and then we wouldn't even have
> to worry about suppressing anything.
Should a bug be filed for this?
Created attachment 249637 [details] [diff] [review]
Pull InsertFrames, AppendFrames, and RemoveFrames up from nsInlineFrame to nsContainerFrame.
Created attachment 249638 [details] [diff] [review]
patch (correct format)
Sorry, previous patch was wrong format.
Comment on attachment 249638 [details] [diff] [review]
patch (correct format)
I actually reviewed the previous patch...
Anyway, r+sr=dbaron, although you might want to reduce the indentation of the variable names in both the .cpp and .h files (they're separated from the types a lot -- probably because there used to be an nsIPresContext* parameter to line them up with).
Checked in, with reduced indentation for parameters:
Checking in mozilla/layout/generic/nsInlineFrame.cpp;
/cvsroot/mozilla/layout/generic/nsInlineFrame.cpp,v <-- nsInlineFrame.cpp
new revision: 3.267; previous revision: 3.266
Checking in mozilla/layout/generic/nsInlineFrame.h;
/cvsroot/mozilla/layout/generic/nsInlineFrame.h,v <-- nsInlineFrame.h
new revision: 1.59; previous revision: 1.58
Checking in mozilla/layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v <-- nsContainerFrame.cpp
new revision: 1.265; previous revision: 1.264
Checking in mozilla/layout/generic/nsContainerFrame.h;
/cvsroot/mozilla/layout/generic/nsContainerFrame.h,v <-- nsContainerFrame.h
new revision: 3.74; previous revision: 3.73
(In reply to comment #29)
> > Then again, what we really need to do is make bidi reordering happen at frame
> > construction time rather than during reflow -- and then we wouldn't even have
> > to worry about suppressing anything.
> Should a bug be filed for this?
Filed bug 365130.
*** Bug 371786 has been marked as a duplicate of this bug. ***