1270174 - Scope SVG ID references in content inserted via Document.insertAnonymousContent() to the inserted element

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mike de Boer [:mikedeboer]]

When I insert an SVG element with document.insertAnonymousContent(), like:

```xml
<svg>
  <mask id="leMask">
    <rect fill="white" height="100%" width="100%" x="0" y="0"/>
    <rect height="18" style="fill: #000; stroke-width: 1; stroke: #666" width="24.899993896484375" x="136" y="19"/>
    <rect height="20.5" style="fill: #000; stroke-width: 1; stroke: #666" width="40.06666564941406" x="136" y="172.5500030517578"/>
  </mask>
  <rect fill="rgba(0,0,0,.2)" height="100%" mask="url(#leMask)" width="100%" x="0" y="0"/>
</svg>
```

... it shows only the <rect>, not the effect of the mask (it's supposed to cut out the two black <rect>s from the layer the mask applies to).

I expect it to show the <rect> _with_ the mask applied, but the ID reference doesn't seem to be understood here.

Is this expected behavior? If so, is this something that we can support for anonymous content - if only for just SVG elements - and would that be something I can implement myself?

Comment 1

[Dão Gottwald [:dao]]

Why do we need this to be anonymous?

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #1)
> Why do we need this to be anonymous?

Because I don't want the nodes to be accessible to content and tooling for web devs.

Comment 3

[Dão Gottwald [:dao]]

Oh, it wasn't clear to me that you're injecting this into web content. Can we not put it on top of tabbrowser?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Can we not put it on top of tabbrowser?

No, because of APZ. Syncing scroll between content and a chrome overlay is possible, but with a lot of overhead, making the scrolling choppy at best. That's why I need the overlay on top of the document directly. That's why I'm attempting to draw all the things inside the content process.

Comment 5

[(no longer active)]

I think you meant to needinfo someone who knows about SVG.  ;-)  I have no idea why the mask isn't applied here.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #5)
> I think you meant to needinfo someone who knows about SVG.  ;-)  I have no
> idea why the mask isn't applied here.

Well, I was thinking that the thing that doesn't seem to work here is that the ID reference to another anonymous node isn't working (most probably by design). I'd like it to work for anonymous SVG nodes at the very least.

Comment 7

[Mike de Boer [:mikedeboer]]

See URL field to see what it _should_ look like.

Comment 8

[Patrick Brosset <:pbro>]

Sorry about the delay.
The anonymous content API we use in devtools sounds like a perfect fit indeed.
I don't know why the ID reference doesn't work here however, from what you are describing, it looks like that's the problem.
I'd have to investigate, but C++ and gecko are not my areas of expertise. I implemented the original API a long time ago as a mentored bug, but that was basically my first contribution to platform (and only contribution so far).
What you are describing should in theory work, and we should fix whatever is preventing this from working. I'm just not sure I'm the best person to do this unfortunately.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Patrick Brosset <:pbro> from comment #8)
> What you are describing should in theory work,

No, not really. There seems to be some misunderstanding as to how anonymous content is sealed off from the rest of the document. It's not like there's a separate anonymous document with a local namespace for IDs. Like all ID reference, the one used in the mask attribute actually points to non-anonymous content.

Comment 10

[Jonathan Watt [:jwatt]]

Being able to scope resource locators does indeed seem desirable, so I send this email to www-style in the hope that authoring need will be met:

http://www.w3.org/mid/[e-mail]

Comment 11

[Jonathan Watt [:jwatt]]

mikedeboer: one thing you could try is using insertAnonymousContent to insert a document sized iframe and then put the mask and referencing rect in that. That should contain the ID reference at least, but ultimately seems like it might not work very well for other reasons.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: WIP: tryout to make sure SVG elements are always added to the ID table

This is what I've tried so far.

To reproduce, follow the following steps:

1. Set the prefs 'devtools.chrome.enabled' and 'devtools.debugger.remote-enabled' to 'true' in about:config and restart Fx.
2. Open the 'Browser Toolbox' from the menu: 'Tools > Web Developer > Browser Toolbox' and select the 'Console' tab in the newly opened window.
3. Enter the following strings on the command line:
```js
let func = function() {
  const kSVGNS = "http://www.w3.org/2000/svg";
  const kWidth = 800;
  const kHeight = 600;
  let document = content.document;
  let svgNode = document.createElementNS("http://www.w3.org/2000/svg", "svg");
  svgNode.setAttribute("viewBox", `0 0 ${kWidth} ${kHeight}`);
  let mask = svgNode.appendChild(document.createElementNS(kSVGNS, "mask"));
  mask.setAttribute("id", "leMask");
  mask.innerHTML = `<rect x="0" y="0" height="${kHeight}" width="${kWidth}" fill="white"/>
  <rect x="45" y="42" height="42" width="42" style="fill: #000; stroke-width: 1; stroke: #666"/>`;
  let rect = svgNode.appendChild(document.createElementNS(kSVGNS, "rect"));
  rect.setAttribute("x", "0");
  rect.setAttribute("y", "0");
  rect.setAttribute("height", kHeight);
  rect.setAttribute("width", kWidth);
  rect.setAttribute("fill", "rgba(0,0,0,.2)");
  rect.setAttribute("mask", "url(#leMask)");
  document.insertAnonymousContent(svgNode);
}

gBrowser.selectedTab.linkedBrowser.messageManager.loadFrameScript("data:text/javascript;base64," + btoa("(" + func.toString() + ")();"), false);
```

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Jonathan Watt [:jwatt] from comment #10)
> Being able to scope resource locators does indeed seem desirable, so I send
> this email to www-style in the hope that authoring need will be met:

FWIW, as an author I expect the ID segments in url() to be locally scoped - as in applied to the current subtree (whether it's shadow DOM or anon subtree).

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Created attachment 8752150 [details] [diff] [review]
> WIP: tryout to make sure SVG elements are always added to the ID table

As you noted in comment 2, you don't want this stuff to be observable by content, so inserting the ID into the document's ID table like this would obviously not be good. Since the anonymous content would then be referencing something using the document's ID table it would even be possible for content to change what the reference resolves to to something controlled by the content.

Comment 15

[Jonathan Watt [:jwatt]]

Shadow content (as opposed to anonymous content) has its own ID table, so if you could insert your highlighter content as shadow content instead, that would possibly be a step forward. (I'm speaking as someone who knows little about the shadow DOM specs.)

However, that would leave you with the problem that SVG resolves IDs using nsReferencedElement, and nsReferencedElement uses the document's ID table. Personally I agree with your comment 13 that these ID references should be scoped though. To make ID references in the shadow tree resolve against the relevant ShadowRoot's ID table nsReferencedElement::Reset and nsReferencedElement::ResetWithID would need to be modified to do that. A further problem is that nsReferencedElement is designed to be live to ID changes, but ShadowRoot doesn't have an equivalent of AddIDTargetObserver right now as far as I can see.

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> A further problem is that nsReferencedElement is designed to be live to ID
> changes, but ShadowRoot doesn't have an equivalent of AddIDTargetObserver
> right now as far as I can see.

This is not an issue for me, but I can see it'd be important to have fixed before web components are enabled by default.
I *think* we can use element.createShadowRoot() right now, but that's not as elegant as anonymous content, because it's designed for overlays.
If only SVG documents would have an internal ID table that delegates to the root table if an ID cannot be found in there. IOW: a local ID table that holds node references for all SVG elements inside that document and if the document is not in an anonymous subtree it also puts it in the global ID table. Then lookups are first attempted in the local ID table and if no node reference is found in there, we delegate the lookup to the root ID table.

Perhaps that'd decouple this SVG issue from the anonymous subtree vs. ShadowRoot problem space that encompasses all of DOM? I mean, it'd be fantastic if we could have all the things, but I can see this sitting around for a long time, whereas there might be a good focused solution out there.
References are just so much more important to SVG than in HTML... by which I mean that for HTML there are many alternative approaches to achieve a certain effect, whereas with SVG that is more limited.

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> I *think* we can use element.createShadowRoot() right now, but that's not as
> elegant as anonymous content, because it's designed for overlays.

The shadow content or side affects would also be visible to the content. Using ShadowRootMode "closed" would hide the shadow tree from content, but prevent content from adding its own shadow tree to whichever node you used (presumably the document element).

> If only SVG documents would have an internal ID table that delegates to the
> root table if an ID cannot be found in there. IOW: a local ID table that
> holds node references for all SVG elements inside that document and if the
> document is not in an anonymous subtree it also puts it in the global ID
> table. Then lookups are first attempted in the local ID table and if no node
> reference is found in there, we delegate the lookup to the root ID table.

SVG inline in another document isn't an "SVG document" (there is not separate Document object), it's just a subtree like any other document subtree. Let's call it an SVG subtree for our purposes here.

I'm not really keen on SVG subtrees having their own ID tables since this would affect non-anonymous and non-shadow SVG subtrees. It just seems like the wrong boundary point and adds overhead to the common case where it isn't needed to solve this specific case. IDs are supposed to be unique across all the elements in a document. Shadow content is not "in" the document technically, so it makes sense for that to have its own ID table.

What might make some sense is using the insertAnonymousContent API to insert an anonymous node then attaching the real content that you care about to that node as a shadow tree. (If that makes sense and can be made to work then we should go ahead and fix up the scoping of ID references in SVG to the ShadowRoot's ID table I think.) This may require relaxing some of the restrictions introduced as a result of:

https://bugzilla.mozilla.org/show_bug.cgi?id=1020244#c9

though (not clear to me), but perhaps you can experiment.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Jonathan Watt [:jwatt] from comment #17)
> What might make some sense is using the insertAnonymousContent API to insert
> an anonymous node then attaching the real content that you care about to
> that node as a shadow tree. (If that makes sense and can be made to work
> then we should go ahead and fix up the scoping of ID references in SVG to
> the ShadowRoot's ID table I think.) This may require relaxing some of the
> restrictions introduced as a result of:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1020244#c9
> 
> though (not clear to me), but perhaps you can experiment.

I think you're right on all counts here :) OK, I think I'll start experimenting with this proposal - I think attaching a shadow tree to anonymous nodes sounds like a good fit, even within the boundaries stated in https://bugzilla.mozilla.org/show_bug.cgi?id=1020244#c9. But let's first see if it'd work at all.

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to Dão Gottwald [:dao] from comment #9)
> There seems to be some misunderstanding as to how anonymous
> content is sealed off from the rest of the document. It's not like there's a
> separate anonymous document with a local namespace for IDs. Like all ID
> reference, the one used in the mask attribute actually points to
> non-anonymous content.

Looking more closely, this isn't actually what happens. In a _XUL_ document, anonymous content with an ID is indeed added to the document's ID table:

https://mxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp?rev=45c71ce1c164&mark=968-968#960

However, we're dealing with an HTML document here and, as it turns out, nsReferencedElement has code that uses nsContentUtils::MatchElementId to walk the anonymous tree looking for the specified element:

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsReferencedElement.cpp?rev=8c85bfdc5442&mark=73-74#49

The reason that this isn't working for Mike when he uses insertAnonymousContent is because insertAnonymousContent doesn't introduce an XBL binding (GetXBLBinding() returns false in the code linked to above).

Incidentally, scoping of ID references in SVG is already working for Shadow DOM content because it apparently does introduce an XBL binding and so we enter this same code for that too. (It's just a bit sub-optimal because rather than using the ShadowRoot's ID table we use nsContentUtils::MatchElementId to find the referenced element.)

If we're already scoping SVG ID references for XBL bindings and for Shadow DOM then I think it makes sense to do the same for insertAnonymousContent.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8752749 [details] [diff] [review]
patch

Review of attachment 8752749 [details] [diff] [review]:
-----------------------------------------------------------------

Seeing your patch and comment made my eyes all teary, so I just _had_ to try it right away.

And it works! <3

::: dom/base/nsDocument.cpp
@@ +5476,5 @@
> +  // separately using |child| here.
> +  nsINode* child = aNode;
> +  nsINode* parent = aNode->GetParentNode();
> +  while (parent && parent->IsInNativeAnonymousSubtree()) {
> +    if (parent == container) {

This doesn't compile; s/container/customContainer/

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752749 [details] [diff] [review]
patch

If this doesn't compile, I assume wrong patch was attached.

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752841 [details] [diff] [review]
patch

># HG changeset patch
># User Jonathan Watt <[e-mail]>
># Parent  1553e1458cd45245c9e3019195da12b92fff4117
>Bug 1270174 - Scope SVG ID references in content inserted via Document.insertAnonymousContent() to the inserted element. r=smaug
>
>diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
>--- a/dom/base/nsDocument.cpp
>+++ b/dom/base/nsDocument.cpp
>@@ -5445,16 +5445,55 @@ nsIDocument::RemoveAnonymousContent(Anon
>       break;
>     }
>   }
>   if (mAnonymousContents.IsEmpty()) {
>     shell->GetCanvasFrame()->HideCustomContentContainer();
>   }
> }
> 
>+bool
>+nsIDocument::AnonymousContentContainerContainsNode(nsINode* aNode,
>+                                                   Element** aRootAnonElement)
>+{
>+  if (!aNode->IsInNativeAnonymousSubtree()) {
>+    return false;
>+  }
>+
>+  nsIPresShell* shell = GetShell();
>+  if (!shell || !shell->GetCanvasFrame()) {
>+    return false;
>+  }
>+
>+  nsAutoScriptBlocker scriptBlocker;
>+  nsCOMPtr<Element> customContainer = shell->GetCanvasFrame()
>+                                           ->GetCustomContentContainer();
>+  if (!customContainer) {
>+    return false;
>+  }
>+
>+  // An arbitrary number of elements can be inserted as children of the custom
>+  // container frame.  If aRootAnonElement is passed then we want the one that
>+  // was added that contained aNode, so we need to keep track of that
>+  // separately using |child| here.
>+  nsINode* child = aNode;
>+  nsINode* parent = aNode->GetParentNode();
>+  while (parent && parent->IsInNativeAnonymousSubtree()) {
>+    if (parent == customContainer) {
>+      if (*aRootAnonElement) {
>+        *aRootAnonElement = child->AsElement();
Nothing guarantees here that child is an Element. So either make aNode Element* or aRootAnonElement nsINode**, and if latter, 
just assign child, and don't use AsElement().
If the method is changed as I propose below, this could become
if (parent == customContainer) { 
  return child->IsElement() ? child->AsElement() : nullptr;
}


>+  /**
>+   * Returns true if aNode is a descendant of anonymous content inserted by
>+   * InsertAnonymousContent.  If the optional aRootAnonElement out-param is
>+   * supplied then the root Element of the inserted anonymous content (the
>+   * clone of the aElement that was passed to InsertAnonymousContent) is
>+   * returned.
>+   */
>+  bool AnonymousContentContainerContainsNode(nsINode* aNode,
>+                                             Element** aRootAnonElement = nullptr);
Since the 2nd param is always passed, just return as a normal return value. No need for out param.
And call the method then... maybe
Element* GetRootForAnonymousContent(nsINode* aNode);

>+    if (!binding) {
>+      // This happens, for example, if aFromContent is part of the content
>+      // inserted by a call to nsIDocument::InsertAnonymousContent, which we
>+      // also want to handle.
>+      Element* anonRoot;
>+      if (doc->AnonymousContentContainerContainsNode(aFromContent, &anonRoot) &&
>+          anonRoot) {
this could then become something like
if (anonRoot doc->GetRootForAnonymousContent(aFromContent)) {
...


those fixed, r+

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4e7c1daa98

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93dcf08d56cd

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e4e7c1daa98
https://hg.mozilla.org/mozilla-central/rev/93dcf08d56cd

Comment 28

[Jonathan Watt [:jwatt]]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Seeing your patch and comment made my eyes all teary, so I just _had_ to try
> it right away.
> 
> And it works! <3

\o/