282481 - nsIBoxObject::GetLastChild doesn't work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

It uses a helper function GetChildByOrdinalAt but passes the illegal -1 value.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174500 [details] [diff] [review]
Proposed patch

>Index: nsBoxObject.h
>+  nsresult GetPreviousSibling(nsIFrame* aParentFrame, nsIFrame* aFrmae,
>+                              nsIDOMElement** aResult);

Document what this method does, please.  And maybe make it static?

Apart from that, it seems to me that all of these methods are wrong.  For
example, consider the following markup:

<div id="outer">
  <div style="display: table-cell" />
  <div id="inner>
<div>

This produces the frame model:

Block
  Table
    Table-row-group
      Table-row
	Table-cell
  Block

where the Block, Table, Table-row-group, and Table-row all have the outer div
as mContent.  Now what will GetFirstChild() return for the outer div?  What
about GetLastChild?  What will GetPreviousSibling return on the inner div?  (It
looks to me like all of those will return the outer div...)

Comment 3

[[e-mail]]

(In reply to comment #2)
> Document what this method does, please.
Given a parent frame and a child frame, find the frame whose
next sibling is the given child frame and return its element
>And maybe make it static?
Done.
>Apart from that, it seems to me that all of these methods are wrong.
May I respectfully point out that this is patching layout/xul/base/src?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> > Document what this method does, please.

In the header, not in the bug.  ;)

> May I respectfully point out that this is patching layout/xul/base/src?

May I respectfully point out that this sort of reasoning is why XUL crashes all
over any time you mix XUL and non-XUL display types?  Replace the two
block-display divs in my testcase with XUL boxes if desired; that doesn't affect
what's actually going on.

I'm going to mark reviews on this patch, but please file a followup bug to make
this use something non-broken for its purposes (like our content iterators, say;
those are designed to deal with this crap, I would hope).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174500 [details] [diff] [review]
Proposed patch

r+sr=bzbarsky with the added documentation.

Comment 6

[Boris Zbarsky [:bzbarsky]]

One other note.  While the code may be in layout/xul/base/src, getBoxObjectFor
is exposed on nsIDOMNSDocument, and will happily create box objects for
everything (I could create them for the <pre> in the tagsoup documents we use to
render plaintext).

Worse, people are using boxobjects for that sort of thing.

Given that, it needs to work in all circumstances.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Can we request approval1.8b?

Comment 8

[[e-mail]]

(In reply to comment #4)
>I'm going to mark reviews on this patch, but please file a followup bug to make
>this use something non-broken for its purposes (like our content iterators, say;
>those are designed to deal with this crap, I would hope).
Except they iterate content, which isn't the desired effect...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, what _is_ the desired effect?  If it's specified, we can write a
general-purpose solution for it (either in boxobject or better in layoututils).
 From what I can see, the only difference between what boxobject does and what
content iterators do is that there is the XUL ordinal mess, right?

Comment 10

[[e-mail]]

Fix checked in. Will file a new bug on the implementation.