::first-letter causes Hebrew text to disappear

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: mehlng, Assigned: Uri Bernstein (Google))

Tracking

({css2, testcase})

Trunk
css2, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSS1-2.4] (correct component?))

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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>
Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: css1
Summary: CSS component first-char won't work with Hebrew text → :first-letter causes Hebrew text to disappear
Whiteboard: [CSS1-2.4]

Updated

13 years ago
OS: Windows XP → All
QA Contact: zach → ian
Hardware: PC → All
Summary: :first-letter causes Hebrew text to disappear → ::first-letter causes Hebrew text to disappear
Whiteboard: [CSS1-2.4] → [CSS1-2.4] (correct component?)
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

13 years ago
Created attachment 145933 [details]
testcase

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

Prog.

Comment 4

13 years ago
Created attachment 145934 [details]
testcase (with proper encoding)
Attachment #145933 - Attachment is obsolete: true

Comment 5

13 years ago
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
this one...
(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
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.
> 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 ;-)
Depends on: 264937

Updated

12 years ago
Keywords: testcase

Comment 16

12 years ago
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.
Keywords: css1 → css2
(Assignee)

Comment 17

11 years ago
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.
Assignee: mozilla → uriber
Status: NEW → ASSIGNED
Attachment #215872 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 18

11 years ago
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.
Blocks: 264937
No longer depends on: 264937
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?
(Assignee)

Comment 20

11 years ago
(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).
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...
(Assignee)

Comment 22

11 years ago
(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?
(Assignee)

Comment 24

11 years ago
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).
Attachment #215872 - Attachment is obsolete: true
Attachment #215872 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 25

11 years ago
Nominating for blocking 1.9a1 so that this doesn't fall off the radar.
Flags: blocking1.9a1?
(Assignee)

Comment 26

11 years ago
Created attachment 217737 [details] [diff] [review]
implement nsFirstLetterFrame::InsertFrames

Perhaps asking in the form of a review request would be more useful.
Attachment #217737 - Flags: superreview?(dbaron)

Updated

11 years ago
Blocks: 334147

Comment 27

11 years ago
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.

Updated

11 years ago
Flags: blocking1.9a1? → blocking1.9+
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.
(Assignee)

Comment 29

11 years ago
(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?
(Assignee)

Comment 30

11 years ago
Created attachment 249637 [details] [diff] [review]
patch

Pull InsertFrames, AppendFrames, and RemoveFrames up from nsInlineFrame to nsContainerFrame.
Attachment #217737 - Attachment is obsolete: true
Attachment #249637 - Flags: superreview?(dbaron)
Attachment #249637 - Flags: review?(dbaron)
Attachment #217737 - Flags: superreview?(dbaron)
(Assignee)

Comment 31

11 years ago
Created attachment 249638 [details] [diff] [review]
patch (correct format)

Sorry, previous patch was wrong format.
Attachment #249637 - Attachment is obsolete: true
Attachment #249638 - Flags: superreview?(dbaron)
Attachment #249638 - Flags: review?(dbaron)
Attachment #249637 - Flags: superreview?(dbaron)
Attachment #249637 - Flags: review?(dbaron)
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).
Attachment #249638 - Flags: superreview?(dbaron)
Attachment #249638 - Flags: superreview+
Attachment #249638 - Flags: review?(dbaron)
Attachment #249638 - Flags: review+
(Assignee)

Comment 33

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 371786
Flags: in-testsuite?

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: ian → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.