Closed Bug 198928 Opened 21 years ago Closed 18 years ago

::first-letter causes Hebrew text to disappear

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mehlng, Assigned: uriber)

References

Details

(Keywords: css2, testcase, Whiteboard: [CSS1-2.4] (correct component?))

Attachments

(3 files, 4 obsolete files)

testcase (with proper encoding)
521 bytes, text/html
Details
screenshot
20.86 KB, image/jpeg
Details
patch (correct format)
11.49 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
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]
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.
Attached file testcase (obsolete) — 
Boris, try to compare this testcase in Mozilla and IE6 and you'll see the bug.

Prog.

        Attachment #145933 -
        Attachment is obsolete: true

    
    

    
  
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

    
  
Keywords: testcase

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter 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.
Assignee: mozilla → uriber
Status: NEW → ASSIGNED

        Attachment #215872 -
        Flags: superreview?(bzbarsky)

    
    

    
  
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?

    
    

    
  
(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...

    
    

    
  
(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]
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)

    
    

    
  
Nominating for blocking 1.9a1 so that this doesn't fall off the radar.
Flags: blocking1.9a1?

    
    

    
  


  
Perhaps asking in the form of a review request would be more useful.

        Attachment #217737 -
        Flags: superreview?(dbaron)

    
  
Blocks: 334147

    
    

    
  

      
      
      
      

        Attached image
          
        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.

    
  
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.

    
    

    
  
(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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  


  
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+

    
    

    
  
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
Closed: 18 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.

    
  
Flags: in-testsuite?

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: