[FIX]Duplicated character with XBL, float, word-spacing

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 3 bugs, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
Created attachment 352018 [details]
testcase 1

Testcase 1 shows the character 'j' being duplicated mysteriously.  I have no idea whether the rest of the rendering of this testcase is "correct".
(Reporter)

Comment 1

10 years ago
Created attachment 352019 [details]
testcase 2

This testcase shows the assertion that first alerted me to the presence of this bug:

###!!! ASSERTION: After-spacing inside a ligature!: 'i - 1 <= aStart || aSpacing[i - 1 - aStart].mAfter == 0', file /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp, line 1542
We seem to get a pretty confused frame model here (e.g. the "jk" textframe ends up between the "A" and "B" textframes?).

The content model (including anon content) in DOM inspector looks correct, though...
Flags: blocking1.9.1?
Boris, sounds like you're probably the best person to look into this. Either something in layout gets really confused, or we double notify on the content side, I guess. Not blocking on this though, unless it turns out to be worse than we think...
Assignee: nobody → bzbarsky
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Created attachment 357368 [details] [diff] [review]
Make sure to get the right prevSibling

The basic issue here was that the frame model looked somewhat like this:

  block
    line
      inline
        text
    line
      inline
        text
      placeholder

with the inlines anonymous, and "line" being first-line frames.  The two inlines and the two texts are in-flows caused by the line-breaking the word-spacing triggers.   We found the placeholder as the next sibling, had no prev-sibling, so did an insert at the beginning of the second line frame.  That put our new text frame sorta in between the two in-flows, which then confused things all around.

The same thing happens if I replace the first-line frame with a <span>, but then at some later point the frame model gets fixed up.  Looks like it happens when we ReflowInlineFrame the "A B" textframe, which does a StealFrame on the second part, then we also StealFrame the second part of the inline, then when we don't fit and push the continuations they end up in the right place, before the "jk".  So looks like the reason first-line is needed is that the flow works differently there, somehow...
Attachment #357368 - Flags: superreview?(roc)
Attachment #357368 - Flags: review?(roc)
roc, do you think this is serious enough to take on branches?
Summary: Duplicated character with XBL, float, word-spacing → [FIX]Duplicated character with XBL, float, word-spacing
why can't the prevsibling or nextsibling have continuations?
Because we're going to set the GetNextSibling() of the prevsibling to ourselves or set our GetNextSibling to the nextsibling.  We don't want to have two frames with the same GetNextSibling(), nor do we want a situation where GetNextSibling() != nsnull and != GetNextContinuation(), do we?

That said, those asserts may not be quite right...  FindFrameForContentSibling guarantees that the frame returned is either the primary frame (for nextsibling) or the tail continuation of the last special sibling (for prevsibling).  So perhaps I should replace the GetNextContinuation asserts with ones about the tail continuation?
I think those assertions are OK.

But this looks fragile to me. We really need ChildIterator to find this anonymous content, don't we?
Fragile, agreed.  ChildIterator doesn't seem to have a way of starting at a given node and then going one back or forward from it... If it could, I agree we should use it and rip out all this business of finding anonymous or non-anonymous content before or after.

If you prefer I can try to make ChildIterator handle this use case and see how it goes.  Just have to be careful to not regress insertion performance in the obvious case of no XBL.
OK.  I have that working, in a totally branch-not-ok way, basically by fixing bug 307394.  So I'll move things there, and we can think about fixing this on branches using the hacky patch or something.
Fixed by checkin for bug 307394.  Checked in test.

Do we want to do the original patch on branch, or just not worry about it?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 357368 [details] [diff] [review]
Make sure to get the right prevSibling

Sounds like a "don't worry about it", I guess.
Attachment #357368 - Flags: superreview?(roc)
Attachment #357368 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.