Last Comment Bug 198928 - ::first-letter causes Hebrew text to disappear
: ::first-letter causes Hebrew text to disappear
Status: RESOLVED FIXED
[CSS1-2.4] (correct component?)
: css2, testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Uri Bernstein (Google)
:
:
Mentors:
: 371786 (view as bug list)
Depends on:
Blocks: 264937 334147
  Show dependency treegraph
 
Reported: 2003-03-24 02:09 PST by mehlng
Modified: 2008-07-31 02:40 PDT (History)
12 users (show)
pavlov: blocking1.9+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (441 bytes, text/html)
2004-04-12 09:44 PDT, Prognathous
no flags Details
testcase (with proper encoding) (521 bytes, text/html)
2004-04-12 09:51 PDT, Prognathous
no flags Details
patch (2.29 KB, patch)
2006-03-22 02:34 PST, Uri Bernstein (Google)
no flags Details | Diff | Splinter Review
implement nsFirstLetterFrame::InsertFrames (3.66 KB, patch)
2006-04-09 05:31 PDT, Uri Bernstein (Google)
no flags Details | Diff | Splinter Review
screenshot (20.86 KB, image/jpeg)
2006-08-03 09:32 PDT, Niro
no flags Details
patch (12.04 KB, patch)
2006-12-25 10:33 PST, Uri Bernstein (Google)
no flags Details | Diff | Splinter Review
patch (correct format) (11.49 KB, patch)
2006-12-25 10:40 PST, Uri Bernstein (Google)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description mehlng 2003-03-24 02:09:45 PST
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
causes it)
Works with MS-IE

Reproducible: Always

Steps to Reproduce:
1.load a problematic pagee

Actual Results:  
Mozilla won't display the Hebrew text

Expected Results:  
it should display it

an Example for problematic simplest web page:
<html><head>
<style type="text/css">
div:first-letter
{color: #ff00f0;}
</style></head><body>
<div>
בוא נראה אם אפשר גם בעברית (SOME HEBREW TEXT)
</div>
<div>aaaa aaa aaa</div>
</body></html>
Comment 1 Simon Montagu :smontagu 2003-03-24 14:50:41 PST
Confirming.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2004-04-12 07:54:42 PDT
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.
Comment 3 Prognathous 2004-04-12 09:44:44 PDT
Created attachment 145933 [details]
testcase

Boris, try to compare this testcase in Mozilla and IE6 and you'll see the bug.

Prog.
Comment 4 Prognathous 2004-04-12 09:51:22 PDT
Created attachment 145934 [details]
testcase (with proper encoding)
Comment 5 Mike Kaply [:mkaply] 2004-04-12 11:08:38 PDT
Wow. This one is pretty bad.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2004-04-12 18:15:00 PDT
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?
Comment 7 Simon Montagu :smontagu 2004-04-19 09:13:06 PDT
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?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2004-04-19 09:27:09 PDT
> 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...
Comment 9 Simon Montagu :smontagu 2004-04-19 09:30:56 PDT
(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
this one...
Comment 10 Simon Montagu :smontagu 2004-04-19 09:37:57 PDT
(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.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2004-04-19 13:29:42 PDT
> 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.
Comment 12 Simon Montagu :smontagu 2004-04-19 14:17:27 PDT
> 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
http://lxr.mozilla.org/mozilla/source/layout/base/src/nsBidiPresUtils.cpp#274 is
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
of it.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2004-04-19 14:21:32 PDT
> 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?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2004-04-19 14:23:18 PDT
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...
Comment 15 Simon Montagu :smontagu 2004-04-19 14:49:37 PDT
>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 ;-)
Comment 16 Anne (:annevk) 2005-04-06 04:57:13 PDT
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.
Comment 17 Uri Bernstein (Google) 2006-03-22 02:34:45 PST
Created attachment 215872 [details] [diff] [review]
patch

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.
Comment 18 Uri Bernstein (Google) 2006-03-22 02:37:21 PST
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.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2006-03-24 12:16:32 PST
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?
Comment 20 Uri Bernstein (Google) 2006-03-25 08:03:04 PST
(In reply to comment #19)
> Would the bidi splits for the first-letter frame stick around in the frame
> tree?  

Yes.

> 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).
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2006-03-25 08:07:21 PST
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...
Comment 22 Uri Bernstein (Google) 2006-03-25 08:17:25 PST
(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?
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2006-03-25 08:38:15 PST
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 24 Uri Bernstein (Google) 2006-03-25 08:44:08 PST
Comment on attachment 215872 [details] [diff] [review]
patch

I'll wait with posting a new patch until these questions are answered (by dbaron, I guess).
Comment 25 Uri Bernstein (Google) 2006-03-30 03:49:56 PST
Nominating for blocking 1.9a1 so that this doesn't fall off the radar.
Comment 26 Uri Bernstein (Google) 2006-04-09 05:31:28 PDT
Created attachment 217737 [details] [diff] [review]
implement nsFirstLetterFrame::InsertFrames

Perhaps asking in the form of a review request would be more useful.
Comment 27 Niro 2006-08-03 09:32:21 PDT
Created attachment 231959 [details]
screenshot

The behavior has been change on Trunk, but is still broken.
Look at the screenshot for details. (comparison between Trunk and Firefox 1.5.0.6)
The 2nd line of the testcase looks good on Trunk.
Comment 28 David Baron :dbaron: ⌚️UTC-10 2006-12-21 11:53:44 PST
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.
Comment 29 Uri Bernstein (Google) 2006-12-22 01:17:13 PST
(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?
Comment 30 Uri Bernstein (Google) 2006-12-25 10:33:29 PST
Created attachment 249637 [details] [diff] [review]
patch

Pull InsertFrames, AppendFrames, and RemoveFrames up from nsInlineFrame to nsContainerFrame.
Comment 31 Uri Bernstein (Google) 2006-12-25 10:40:34 PST
Created attachment 249638 [details] [diff] [review]
patch (correct format)

Sorry, previous patch was wrong format.
Comment 32 David Baron :dbaron: ⌚️UTC-10 2006-12-25 10:45:00 PST
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).
Comment 33 Uri Bernstein (Google) 2006-12-25 10:54:48 PST
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
done
Checking in mozilla/layout/generic/nsInlineFrame.h;
/cvsroot/mozilla/layout/generic/nsInlineFrame.h,v  <--  nsInlineFrame.h
new revision: 1.59; previous revision: 1.58
done
Checking in mozilla/layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.265; previous revision: 1.264
done
Checking in mozilla/layout/generic/nsContainerFrame.h;
/cvsroot/mozilla/layout/generic/nsContainerFrame.h,v  <--  nsContainerFrame.h
new revision: 3.74; previous revision: 3.73
done
Comment 34 David Baron :dbaron: ⌚️UTC-10 2006-12-27 07:46:16 PST
(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.
Comment 35 Uri Bernstein (Google) 2007-02-26 23:44:03 PST
*** Bug 371786 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.