Closed
Bug 256624
Opened 20 years ago
Closed 20 years ago
<switch> should operate over "real" content children only
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: scootermorris, Assigned: scootermorris)
References
()
Details
Attachments
(1 file, 3 obsolete files)
3.69 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
asa
:
approval1.8b2+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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.
Assignee | ||
Comment 2•20 years ago
|
||
(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!
Assignee | ||
Comment 3•20 years ago
|
||
Comment 4•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
(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.
Assignee | ||
Comment 7•20 years ago
|
||
(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•20 years ago
|
||
> 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....
Assignee | ||
Comment 9•20 years ago
|
||
Is this closer to the mark?
Comment 10•20 years ago
|
||
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)....
Assignee | ||
Comment 11•20 years ago
|
||
(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;
}
Assignee | ||
Updated•20 years ago
|
Attachment #180608 -
Attachment is obsolete: true
Comment 12•20 years ago
|
||
Exactly.
Assignee | ||
Comment 13•20 years ago
|
||
OK, this should be ready for review.
Attachment #181059 -
Attachment is obsolete: true
Attachment #181089 -
Flags: review?(tor)
Assignee | ||
Updated•20 years ago
|
Attachment #181089 -
Flags: review?(tor) → review?(bzbarsky)
Comment 14•20 years ago
|
||
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•20 years ago
|
||
So we're arbitrarily deciding that <svg:switch> elements must not have XBL
bindings because it's hard to implement? That sucks.
Comment 16•20 years ago
|
||
I was under the impression we were deciding that because the SVG spec explicitly
said so...
Assignee | ||
Comment 17•20 years ago
|
||
(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.
Assignee | ||
Comment 18•20 years ago
|
||
(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•20 years ago
|
||
> 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.
Assignee | ||
Comment 20•20 years ago
|
||
OK, here is another patch with comments addressed. Thanks, as always, for the
comments (and the education!).
Assignee | ||
Updated•20 years ago
|
Attachment #181089 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #181089 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•20 years ago
|
Attachment #181311 -
Flags: review?(bzbarsky)
Comment 21•20 years ago
|
||
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+
Assignee | ||
Updated•20 years ago
|
Attachment #181311 -
Flags: approval1.8b2?
Comment 22•20 years ago
|
||
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+
Assignee | ||
Updated•20 years ago
|
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.
Description
•