277145 - [svg sr] frame construction

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[tor]

File:

  layout/base/nsCSSFrameConstructor.cpp

Comment 1

[tor]

Attachment: dummy attachment for bug tracking purposes

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this is a request to review all MOZ_SVG code in nsCSSFrameConstructor?

Comment 3

[tor]

Yes, sorry for not mentioning that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

All good.  This will take a few days, though...

Comment 5

[Boris Zbarsky [:bzbarsky]]

In nsCSSFrameConstructor::ConstructDocElementFrame we set isBlockFrame true for
SVG.  Why?  If that's set we set the newly-constructed frame as a float and
absolute containing block and try to construct first-line and first-letter
frames for it in ProcessChildren.  Given that nsSVGOuterSVGFrame.cpp can't
handle floating or abs pos children and that it shouldn't get
first-line/first-letter things, I think this boolean should be set to false.

In ConstructTextFrame, we try to QI aParentFrame to nsISVGTextContainerFrame. 
Why not use a frametype check (much faster)?  And why are we using an nsCOMPtr
for a non-refcounted object?  Finally, can we get in here when the parent is an
SVG node but doesn't have one of the two SVG frames that implement
nsISVGTextContainerFrame?  If not, then the code can just check what the parent
content is (and maybe assert the interface or frametype or something in debug
builds).

In CreateAnonymousFrames(), we set the binding parent for the <use> kids to the
<use> element.  How does this interact with event propagation and retargeting? 
For that matter, what's the expected event target (in the DOM) when a child of
<use> is clicked?

In SVGSwitchProcessChildren, why do we have the eELEMENT check before breaking
the child loop?  Any non-element will never even hit that code.  Perhaps we
should assert that, in fact.

I'll look at ConstructSVGFrame later, but for the nonce I'll point out that it
doesn't handle interaction with tables at all well (see the XXX comments about
ProcessPseudoFrames and so forth).  Partly this is the fault of the braindead
pseudoframes setup, though....

Comment 6

[Boris Zbarsky [:bzbarsky]]

In ConstructSVGFrame:

There's some commented-out mention of nsSVGTableCreator that needs to be removed.

Should an <svg> inside a <foreignObject> really not be considered outermost? 
What does the spec say?  At the moment, it's not.

The mess with claiming that foreignObject is an absolute containing block is
just wrong.  Either it needs to really be one (including fixes to
GetAbsoluteContainingBlock, fixes to blockframe as needed to do reflow right in
this case, etc), or it needs to pass false as the last arg to ConstructBlock.

On a separate note, should a foreignObject with display:inline really construct
a blockframe?  What about display:table?  This part seems wrong as well...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170365 [details]
dummy attachment for bug tracking purposes

I think that's about it...

Comment 8

[Scooter Morris]

(In reply to comment #5)
Boris,
   Thanks for the review!  I'm just starting to go through this, so this will
come in pieces.

> In nsCSSFrameConstructor::ConstructDocElementFrame we set isBlockFrame true for
> SVG.  Why?  If that's set we set the newly-constructed frame as a float and
> absolute containing block and try to construct first-line and first-letter
> frames for it in ProcessChildren.  Given that nsSVGOuterSVGFrame.cpp can't
> handle floating or abs pos children and that it shouldn't get
> first-line/first-letter things, I think this boolean should be set to false.

I don't see this -- perhaps its changed?  The only place where isBlockFrame is
set to true is in the case where its *not* SVG, right?

> 
> In ConstructTextFrame, we try to QI aParentFrame to nsISVGTextContainerFrame. 
> Why not use a frametype check (much faster)?  

It appears that none of the SVG frames implement GetType(), nor do we use the
LayoutAtom stuff at all.  Hmm..

.. And why are we using an nsCOMPtr
> for a non-refcounted object?  Finally, can we get in here when the parent is an
> SVG node but doesn't have one of the two SVG frames that implement
> nsISVGTextContainerFrame?  If not, then the code can just check what the parent
> content is (and maybe assert the interface or frametype or something in debug
> builds).

I'll have to dig into this a little more.  I'm not sure exactly why this was
done this way.

> 
> In CreateAnonymousFrames(), we set the binding parent for the <use> kids to the
> <use> element.  How does this interact with event propagation and retargeting? 
> For that matter, what's the expected event target (in the DOM) when a child of
> <use> is clicked?

<use> is just plain strange.  I'm pretty sure that this is implemented by
copying the content and binding the content into the referring frame.  So, the
child of a <use> should never be displayed.

> 
> In SVGSwitchProcessChildren, why do we have the eELEMENT check before breaking
> the child loop?  Any non-element will never even hit that code.  Perhaps we
> should assert that, in fact.
> 

OK, makes sense.  I've also added code that checks to see if the child is bound
to the parent (hence anonymous), and to continue if it is.

> I'll look at ConstructSVGFrame later, but for the nonce I'll point out that it
> doesn't handle interaction with tables at all well (see the XXX comments about
> ProcessPseudoFrames and so forth).  Partly this is the fault of the braindead
> pseudoframes setup, though....
> 
> 

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't see this -- perhaps its changed? 

Yeah.  I fixed it in bug 283385.

> It appears that none of the SVG frames implement GetType()

Well, GetType() is the standard way to indicate which concrete frame class an
nsIFrame is, at the moment...

> So, the child of a <use> should never be displayed.

You mean no anonymous descendants of a <use> ever render?  That seems unlikely
to me, since then <use> would just not work.

> I've also added code that checks to see if the child is bound to the parent
> (hence anonymous)

Huh?  We're walking the kids of an nsIContent, right?  They should certainly not
be anonymous from the point of view of the nsIContent...

Comment 10

[Scooter Morris]

(In reply to comment #9)
> > I don't see this -- perhaps its changed? 
> 
> Yeah.  I fixed it in bug 283385.
> 
> > It appears that none of the SVG frames implement GetType()
> 
> Well, GetType() is the standard way to indicate which concrete frame class an
> nsIFrame is, at the moment...

I don't think that was known or understood when SVG was initially implemented. 
I'm almost afraid to ask, but how important is it for our frames to implement
GetType()?  Should we open up a separate bug to define frametypes and implement
GetType() all of the SVG Frames?
> 
> > So, the child of a <use> should never be displayed.
> 
> You mean no anonymous descendants of a <use> ever render?  That seems unlikely
> to me, since then <use> would just not work.
> 
> > I've also added code that checks to see if the child is bound to the parent
> > (hence anonymous)
> 
> Huh?  We're walking the kids of an nsIContent, right?  They should certainly not
> be anonymous from the point of view of the nsIContent...
> 
> 
OK, I'm going to admit ignorance and take a much closer look at tor's
implementation before I respond.  I know that <use> is a little strange because
of the content cloned from the target.  I was probably getting confused between
the <use> and the target (which is usually a child of a <defs>).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Should we open up a separate bug to define frametypes and implement
> GetType() all of the SVG Frames?

Probably a good idea in general, yes...  Fix bug 280988 while you're there too?  ;)

Comment 12

[Scooter Morris]

(In reply to comment #9)
> > So, the child of a <use> should never be displayed.
> 
> You mean no anonymous descendants of a <use> ever render?  That seems unlikely
> to me, since then <use> would just not work.

OK, I was confused (not a surprise).  The correct answer is that events are
supposed to go to an SVGElementInstance shadow tree, which isn't implemented yet
(see bug #265895).
> 
> > I've also added code that checks to see if the child is bound to the parent
> > (hence anonymous)
> 
> Huh?  We're walking the kids of an nsIContent, right?  They should certainly not
> be anonymous from the point of view of the nsIContent...
> 
> 
What about anonymously bound XBL children?  That was the issue that was brought
up during the initial review of the code -- that the switch should ignore XBL
bound children.  Perhaps I'm using the wrong test?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> What about anonymously bound XBL children?

They're not children as far as the nsIContent interface is concerned...

Comment 14

[Scooter Morris]

(In reply to comment #13)
> > What about anonymously bound XBL children?
> 
> They're not children as far as the nsIContent interface is concerned..

OK, I'm confused.  I interpreted dbaron's concerns in his review of bug 216563
to mean that the iterator I was using iterated over *all* the children,
including the anonymous (XBL) children (see bug 216563 comment 21).  Did I get
that completely wrong?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  That part was about <switch>, not <use>.  Yes, that should do the
XBL-bound kids too.  Now I'm confused... What skipping did you add and why?

Comment 16

[Scooter Morris]

(In reply to comment #15)
> Oh, I see.  That part was about <switch>, not <use>.  Yes, that should do the
> XBL-bound kids too.  Now I'm confused... What skipping did you add and why?

I added the following:
    // Since 'switch' should only operate on 'real' children, check
    // to see if this is an anonymous child
    if (child->GetBindingParent() == aContent) {
       // This child content is anonymous. Don't use it
       // for our test
       continue;
    }

From what I could tell, XBL bound children will have their GetBindingParent
value set, while "real" children will not.  By skipping over this content for
the purposes of <switch>, I was hoping that this would meet dbaron's concerns.

Comment 17

[Boris Zbarsky [:bzbarsky]]

But then why bother using a content iterator at all, if you want to skip all the
anonymous content?

More precisely, what is, in words, the algorithm that's implementing?  And
should we move this part to a separate bug?

Comment 18

[Scooter Morris]

(In reply to comment #17)
> 
> More precisely, what is, in words, the algorithm that's implementing?  And
> should we move this part to a separate bug?

It already is a separate bug.  This looked like a quick fix that I was going to
put in while I was looking at nsCSSFrameConstructor.  The algorithm that I was
trying to implement was to iterate over all of the "real" children of the
<switch> element, evaluating against the <switch> conditional, and processing
the first child that evaluates to "true".

Nevertheless, I agree that this should be taken up separately (in bug 256624)