Closed Bug 256624 Opened 20 years ago Closed 20 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: scootermorris, Assigned: scootermorris)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
(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!
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.
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).
(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.
(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?
> 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....
Is this closer to the mark?
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)....
(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; }
Attachment #180608 - Attachment is obsolete: true
Exactly.
OK, this should be ready for review.
Attachment #181059 - Attachment is obsolete: true
Attachment #181089 - Flags: review?(tor)
Attachment #181089 - Flags: review?(tor) → review?(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?
So we're arbitrarily deciding that <svg:switch> elements must not have XBL bindings because it's hard to implement? That sucks.
I was under the impression we were deciding that because the SVG spec explicitly said so...
(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.
(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?
> 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.
OK, here is another patch with comments addressed. Thanks, as always, for the comments (and the education!).
Attachment #181089 - Attachment is obsolete: true
Attachment #181089 - Flags: review?(bzbarsky)
Attachment #181311 - Flags: review?(bzbarsky)
Comment on attachment 181311 [details] [diff] [review] Apply switch to DOM content children only (with bz's comments addressed) r+sr=bzbarsky
Attachment #181311 - Flags: superreview+
Attachment #181311 - Flags: review?(bzbarsky)
Attachment #181311 - Flags: review+
Attachment #181311 - Flags: approval1.8b2?
Comment on attachment 181311 [details] [diff] [review] Apply switch to DOM content children only (with bz's comments addressed) a=asa
Attachment #181311 - Flags: approval1.8b2? → approval1.8b2+
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: