232990 - display frames messed up when switching CSS display: property on XML elements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Axel Hecht]

I uploaded a testcase of pure XML, styled with CSS. The selectors are
dependent on attributes, so I can switch between two rulesets. One is consisting
of a float (the "-") and the blocks. Clicking the "-" triggers XBL to set
the "collapsed" attribute, which triggers the float and the first and the third
div to get display inline. (And change the text node to "+".)
Clicking again removes the attribute, getting back to the old CSS rules.
But I seem to get dangling frames, each time I cycle thru this. Not one, for
each cycle one.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A simpler testcase would be nice.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah... if it's possible to do this without xbl (eg using XHTML elements and
onlick), that would be great.  Also, do you need all of the CSS rules in that sheet?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

"onlick"? that sounds pretty perverted... Do we really have a lick-event?

:)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, it's basically the same impl as onscratch-n-sniff.

Comment 5

[Axel Hecht]

Attachment: trivial binding, get an anonymous float in there

Comment 6

[Axel Hecht]

Attachment: xhtml that uses above binding once, inline CSS

I teared down the testcase and while doing so, I realized that everything works

fine as long as the float is not anonymous content.
So the testcase has two chunks now, one that is not working with XBL and one
that is not working, with explicit content.
I added buttons that do the attribute setting and removing for each of the two.

The CSS is now just inside a xhtml:style.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly simpler

Comment 8

[Boris Zbarsky [:bzbarsky]]

Where do frames for insertion points get constructed?  Are we ending up with an
inline insertion point frame and sticking a block in it, leading to a nasty IB
situation?

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

This fixes it. nsCSSFrameConstructor::FindPreviousSibling uses the index with a
ChildIterator, so nsCSSFrameConstructor::RecreateFramesForContent should use
one when getting the index, otherwise we don't take into account the anonymous
content. Note that there's other callers to nsIContent::IndexOf in
nsCSSFrameConstructor, maybe they should be converted to use a ChildIterator
too?
The parent change looks like it doesn't have any effect, but it seems to be
what was intended.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Peter, is this also fixed by the patch in bug 289377?  It looks to me like it
should be...

I really don't like making the meaning of "aIndexInContainer" vague like this
patch does (it means different things in different cases with this patch).

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> Peter, is this also fixed by the patch in bug 289377?  It looks to me like it
> should be...

It isn't.

> I really don't like making the meaning of "aIndexInContainer" vague like this
> patch does (it means different things in different cases with this patch).

I'm fine with looking for a different way fix it, but we *will* have to deal
with the fact that nsCSSFrameConstructor::FindPreviousSibling uses
aIndexInContainer with an iterator (which does take into account the anonymous
content), while nsCSSFrameConstructor::FindPreviousSibling does not.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Wait.  Back up.  Which FindPreviousSibling call are we concerned with here?  The
one with the XXX comment before it at line 9014 (in ContentInserted)?  And the
problem is that since aIndexInContainer is the index in the non-anonymous child
list this code completely fails?

If that's the case, we should fix ContentInserted, not just fix up
RecreateFramesForContent to work around ContentInserted bugs...

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 181703 [details] [diff] [review]
v1

Ok, so what should aIndexInContainer be in calls to
ContentRemoved/ContentInserted? ContentAppended for example uses an iterator to
get the index that it passes to ContentInserted, while RecreateFramesForContent
uses nsIContent::IndexOf. GetAdjustedParentFrame (called by ContentInserted)
uses nsIContent::ChildAt with that index, while FindPreviousSibling (also
called by ContentInserted) uses an iterator.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Ok, so what should aIndexInContainer be 

Given that these are basically just the nsIDocumentObserver notifications, it
should be whatever nsIDocumentObserver promises.

Then within these methods we should use child iterators as appropriate, _after_
resolving the insertion point (which last should also update the index
appropriately, if this stuff worked right).

The call from ContentAppended to ContentInserted with the mixed-up index sounds
like it's trying to cover up the 50% case and is likely the primary reason
ContentInserted hasn't been fixed before now.  :(

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Version of "slightly simpler" with an updated binding URI

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 307394.  Checked in test.