256624 - <switch> should operate over "real" content children only

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scooter Morris]

This is a followup to bug #216563.  The patch to that bug operated over *all*
children of the <switch>.  A concern was raised that in the case when there are
XBL children, the <switch> should operate *only* over the "real" content children.

Comment 1

[Benjamin Smedberg]

We should have an official response from the SVG WG, and hopefully the XBL
group. You can't just willy-nilly ignore the shadow content, because shadow
content itself may set up a <svg:switch>:

<binding xmlns="xbl">
  <content>
    <children />
    <xbl:switch>
      <blah1 />
      <blah2 />
    </xbl:switch>
  </content>
</binding>

So you would have to enumerate the DOM children of the <svg:switch> on the same
"binding level", remembering that bindings can nest and inherit. I think that we
should stick to switching on the shadow-DOM, and let clients who add an XBL
binding <svg:switch> deal with the slightly odd behavior.

Comment 2

[Scooter Morris]

(In reply to comment #1)
> We should have an official response from the SVG WG, and hopefully the XBL
> group. You can't just willy-nilly ignore the shadow content, because shadow
> content itself may set up a <svg:switch>:
> 
> <binding xmlns="xbl">
>   <content>
>     <children />
>     <xbl:switch>
>       <blah1 />
>       <blah2 />
>     </xbl:switch>
>   </content>
> </binding>
> 
> So you would have to enumerate the DOM children of the <svg:switch> on the same
> "binding level", remembering that bindings can nest and inherit. I think that we
> should stick to switching on the shadow-DOM, and let clients who add an XBL
> binding <svg:switch> deal with the slightly odd behavior.


I went back into my E-Mail, and the SVG WG (and XBL WG) did give a pretty
definitive reply:
-------------------------------------------------
From: Robin Berjon

Hi Scooter,

Scooter wrote:

>     I've submitted a patch to Mozilla to implement SVG <switch> and an issue
has been raised that we would like some input on.  The current patch will
iterate over all direct children of the <switch> element to determine which
element evaluates to true.  This works correctly for all of the test cases we
had.  The question that has arisen is what happens in the presence of anonymous
children such as those bound by XBL?  Should <switch>
> iterate over all children or should it ignore the anonymous children and only
iterate over "real" (i.e. document) children?
>     We are awaiting input from the SVG working group before checking this
patch in, so any input would be greatly appreciated.


The SVG WG and the XBL TF have discussed the problem jointly and come to
the conclusion that <switch> would only operate on the real DOM. This
means it doesn't apply to shadow trees, and doesn't take animations into
account either.

The motivation behind this chiefly has to do with the fact that it adds extra
complexity for no obvious benefit. Also, modifying SVG's behaviour using sXBL is
outside the scope of SVG 1.2. It is possible that this will be made possible in
future versions of SVG that will use future developments made on top of sXBL,
for instance something like XBL v2.0.

Your feedback is much welcome, keep it coming! 

Comment 3

[Scooter Morris]

Attachment: Patch to apply switch *only* to DOM content children

Comment 4

[Benjamin Smedberg]

Will this decision be addressed in a specification, or only in a private email?
Spec-by-email is not a good way to get interoperable implementations. A note in
either the SVG or XBL spec, hopefully with a testcase, is in order here. Note
that the letter does not address switches inside the shadow tree itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So wait.  We have three options here:

1)  <switch> operates on its children in the flattened tree
2)  <switch> operates on its children in the DOM tree
3)  <switch> operates on the intersection of the sets in #1 and #2.

This last is what that patch implements.  I don't see why that's the right
behavior, or even a desirable one, the email in comment 2 notwithstanding (it
seems to assume you're talking about #1 vs #2).

Comment 6

[Robin Berjon]

(In reply to comment #5)
> So wait.  We have three options here:
> 
> 1)  <switch> operates on its children in the flattened tree
> 2)  <switch> operates on its children in the DOM tree
> 3)  <switch> operates on the intersection of the sets in #1 and #2.

The idea is that 2) is correct (I'm working on getting that into the spec). And for clarity, in cases where 
switch is itself inside anonymous/shadow content it operates on its direct DOM children, not recursive 
anonymous/shadow content.

Comment 7

[Scooter Morris]

(In reply to comment #5)
> So wait.  We have three options here:
> 
> 1)  <switch> operates on its children in the flattened tree
> 2)  <switch> operates on its children in the DOM tree
> 3)  <switch> operates on the intersection of the sets in #1 and #2.
> 
> This last is what that patch implements.  I don't see why that's the right
> behavior, or even a desirable one, the email in comment 2 notwithstanding (it
> seems to assume you're talking about #1 vs #2).

We are.  If the patch implements #3 then its wrong -- I was trying to implement
#2.  What should I have done to test if the child is in the DOM tree?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> What should I have done to test if the child is in the DOM tree?

Just iterate over the DOM tree kids:

PRInt32 childCount = aContent->GetChildCount();
for (PRInt32 i = 0; i < childCount; ++i) {
  nsIContent* child = aContent->GetChildAt(i);

  rv = TestSVGConditions(child,
...

instead of using the ChildIterator stuff, which is very much designed to iterate
over the equivalent of the flattened tree in Mozilla XBL.  The problem with your
patch was that it would miss some kids in the DOM tree altogether, not that it
was testing wrong....

Comment 9

[Scooter Morris]

Attachment: Another attempt to apply switch to only DOM content children

Is this closer to the mark?

Comment 10

[Boris Zbarsky [:bzbarsky]]

That would work, I think, if we set "-moz-binding: none !important" on
<svg:switch> (so that it's DOM children are also its children in the flattened
tree).

If we _don't_ do that, and we do have an XBL binding that moves the switch kids
further down into the flattened tree, we'll create frames for them twice (and
one of them will be in the wrong place)....

Comment 11

[Scooter Morris]

(In reply to comment #10)
> That would work, I think, if we set "-moz-binding: none !important" on
> <svg:switch> (so that it's DOM children are also its children in the flattened
> tree).
> 
> If we _don't_ do that, and we do have an XBL binding that moves the switch kids
> further down into the flattened tree, we'll create frames for them twice (and
> one of them will be in the wrong place)....

Something like this?

Index: layout/svg/base/src/svg.css
===================================================================
RCS file: /cvsroot/mozilla/layout/svg/base/src/svg.css,v
retrieving revision 1.9
diff -p -u -8 -r1.9 svg.css
--- layout/svg/base/src/svg.css 12 Apr 2005 15:46:42 -0000      1.9
+++ layout/svg/base/src/svg.css 18 Apr 2005 21:23:56 -0000
@@ -54,11 +54,15 @@ svg {

   font-family: sans-serif;
 }

 style, script, symbol {
  display: none;
 }

+switch {
+ -moz-binding: none !important;
+}
+
 svg:not(:root), symbol, image, marker, pattern, foreignObject {
  overflow: hidden;
 }

Comment 12

[Boris Zbarsky [:bzbarsky]]

Exactly.

Comment 13

[Scooter Morris]

Attachment: Apply switch to only DOM content children

OK, this should be ready for review.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181089 [details] [diff] [review]
Apply switch to only DOM content children

>Index: layout/base/nsCSSFrameConstructor.cpp
>+    // Skip over children that aren't DOM children (e.g. XBL)
>+    // Note that this means (AFAIK) that you can't use XBL to
>+    // construct a <switch> clause.  <switch> must be a DOM
>+    // child for this to work (which is consistent with the SVG WG
>+    // interpretation.
>+    if (child->GetBindingParent()) {
>+      // We're anonymously bound -- continue
>+      continue;
>+    }

I don't see why you need that block. It looks to me like this will make an XBL
binding whose anon content includes a <switch> with some kids, all real DOM
kids of the <switch> but the whole thing part of an XBL binding, not render
anything.  Is that desirable?  Why?

>+      rv = ConstructFrame(aState, nsCOMPtr<nsIContent>(child),

Why do you need the nsCOMPtr here?

Comment 15

[Benjamin Smedberg]

So we're arbitrarily deciding that <svg:switch> elements must not have XBL
bindings because it's hard to implement? That sucks.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I was under the impression we were deciding that because the SVG spec explicitly
said so...

Comment 17

[Scooter Morris]

(In reply to comment #15)
> So we're arbitrarily deciding that <svg:switch> elements must not have XBL
> bindings because it's hard to implement? That sucks.

No, actually the current implementation operates over all children, including
anonymously bound children, AFAIK.  This bug is to "fix that" since after
conversation with the SVG WG they stated that this was not the intent of switch.
 See bug #216563 for more details.

Comment 18

[Scooter Morris]

(In reply to comment #14)
> (From update of attachment 181089 [details] [diff] [review] [edit])
> >Index: layout/base/nsCSSFrameConstructor.cpp

> 
> I don't see why you need that block. It looks to me like this will make an XBL
> binding whose anon content includes a <switch> with some kids, all real DOM
> kids of the <switch> but the whole thing part of an XBL binding, not render
> anything.  Is that desirable?  Why?

Agreed.  I think that the statement from the WG is that the concern is that
<switch> should operate over DOM *children*.  So I'll remove the block.

> 
> >+      rv = ConstructFrame(aState, nsCOMPtr<nsIContent>(child),
> 
> Why do you need the nsCOMPtr here?
> 
Does GetChildAt return an AddRefed pointer?  Don't I need an nsCOMPtr for
ConstructFrame?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Does GetChildAt return an AddRefed pointer?

No.

> Don't I need an nsCOMPtr for ConstructFrame?

No.

You needed an nsCOMPtr for *iter because that returns
already_AddRefed<nsIContent>.  But you're not using *iter anymore.

Comment 20

[Scooter Morris]

Attachment: Apply switch to DOM content children only (with bz's comments addressed)

OK, here is another patch with comments addressed.  Thanks, as always, for the
comments (and the education!).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181311 [details] [diff] [review]
Apply switch to DOM content children only (with bz's comments addressed)

r+sr=bzbarsky

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 181311 [details] [diff] [review]
Apply switch to DOM content children only (with bz's comments addressed)

a=asa