Patrick Brosset <:pbro> Assignee
	Description • 10 years ago

Relevant thread: https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/fiWpoAGnc8Q

To sum up: we would like, in the devtools, to be able to leverage the newly added (bug 924692) CreateAnonymousContent in the CanvasFrame to insert our highlighter overlays (here's one for example: https://www.youtube.com/watch?v=hMBsRgSGr0k).

For now, we append our overlays in the browser chrome XUL structure, which isn't possible on FxOS, Fennec or when E10S is activated.

	(no longer active)
	Comment 1 • 10 years ago

Accessing the native anonymous content from JS is not something that we've allowed so far.  In the current places where we have these elements, Gecko makes all kinds of assumptions about the structure of the DOM.  Exposing these elements to JS means that all of those assumptions may become invalid.

Can we avoid having to grant access to the chrome JS by adding other APIs that do what you want inside Gecko?

	Patrick Brosset <:pbro> Assignee
	Comment 2 • 10 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #1)
> Accessing the native anonymous content from JS is not something that we've
> allowed so far.  In the current places where we have these elements, Gecko
> makes all kinds of assumptions about the structure of the DOM.  Exposing
> these elements to JS means that all of those assumptions may become invalid.
Ok, I understand, although from this message [1] I was under the impression that the native anonymous content in the canvas frame would be the way to go for our needs.
[1] https://groups.google.com/d/msg/mozilla.dev.tech.layout/fiWpoAGnc8Q/xkrDm7m82AYJ

> Can we avoid having to grant access to the chrome JS by adding other APIs
> that do what you want inside Gecko?
Sure, what kind of APIs do you have in mind?
Just to be clear, I think the requirements can be summed up as:
- being able to inject our own HTML/CSS-based highlighter elements on top of the page,
- making these elements invisible to content code (shouldn't show up when using DOM APIs),
- should work in all applications/devices that use gecko (our devtools can remotely debug firefox desktop, fennec, fxos, thunderbird)

Thanks for your help.

	(no longer active)
	Comment 3 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #1)
> > Accessing the native anonymous content from JS is not something that we've
> > allowed so far.  In the current places where we have these elements, Gecko
> > makes all kinds of assumptions about the structure of the DOM.  Exposing
> > these elements to JS means that all of those assumptions may become invalid.
> Ok, I understand, although from this message [1] I was under the impression
> that the native anonymous content in the canvas frame would be the way to go
> for our needs.
> [1]
> https://groups.google.com/d/msg/mozilla.dev.tech.layout/fiWpoAGnc8Q/
> xkrDm7m82AYJ

Yes, I wrote that.  ;-)  And I still believe what I said there.  But that doesn't mean we need to give chrome JS access to these elements.

> > Can we avoid having to grant access to the chrome JS by adding other APIs
> > that do what you want inside Gecko?
> Sure, what kind of APIs do you have in mind?
> Just to be clear, I think the requirements can be summed up as:
> - being able to inject our own HTML/CSS-based highlighter elements on top of
> the page,

So, arbitrary access to these elements from chrome JS?  :(

> - making these elements invisible to content code (shouldn't show up when
> using DOM APIs),

Those elements are not exposed through DOM APIs in any way.

> - should work in all applications/devices that use gecko (our devtools can
> remotely debug firefox desktop, fennec, fxos, thunderbird)

This also comes for free.

	Patrick Brosset <:pbro> Assignee
	Comment 4 • 10 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> > - being able to inject our own HTML/CSS-based highlighter elements on top of
> > the page,
> 
> So, arbitrary access to these elements from chrome JS?  :(
Why the sad face :( I clearly don't know enough about gecko's needs and assumptions when it comes to native anon content to understand the implications here.
Could we write the highlighter in C++ in Gecko, using this anon content, and only let the chrome JS toggle it through a simple API? 
> > - making these elements invisible to content code (shouldn't show up when
> > using DOM APIs),
> 
> Those elements are not exposed through DOM APIs in any way.
Perfect
> > - should work in all applications/devices that use gecko (our devtools can
> > remotely debug firefox desktop, fennec, fxos, thunderbird)
> 
> This also comes for free.
Great, making our highlighter work on all platform really is what we're after.

	(no longer active)
	Comment 5 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #3)
> > > - being able to inject our own HTML/CSS-based highlighter elements on top of
> > > the page,
> > 
> > So, arbitrary access to these elements from chrome JS?  :(
> Why the sad face :( I clearly don't know enough about gecko's needs and
> assumptions when it comes to native anon content to understand the
> implications here.

Well, it's nice to be able to control what happens to the native anon content so that we can make assumptions about how it looks and works inside Gecko.  Otherwise relying on things can be very difficult.

> Could we write the highlighter in C++ in Gecko, using this anon content, and
> only let the chrome JS toggle it through a simple API? 

Yes!  FWIW this is the approach that I recommended for the touch caret for Firefox OS.  It would be great if we can do something similar for the highlighter as well.

	Patrick Brosset <:pbro> Assignee
	Comment 6 • 10 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #5)
> > Could we write the highlighter in C++ in Gecko, using this anon content, and
> > only let the chrome JS toggle it through a simple API? 
> 
> Yes!  FWIW this is the approach that I recommended for the touch caret for
> Firefox OS.  It would be great if we can do something similar for the
> highlighter as well.
Sounds good to me.

Can you confirm my understanding of how the touch caret element is generated:
- There's a CreateAnonymousContent method in the CanvasFrame that basically creates an anonymous DOM element and appends it to the frame
- TouchCaret.cpp is then responsible for positioning and toggling that element
- The rest is done in ua.css where the caret element is styles (basically just with a few images).

If we can have something similar for the highlighter and drive this from JS chrome code, that would be awesome.
One thing though: the highlighter (and in fact highlighterS, since we already have 2 of them, and plan on having more) is a lot more complex than the touch caret. It is based on multiple XUL/SVG elements and requires a lot of styling. I'll include screenshots of the highlighters we have now.

Let me know if the nature of the highlighters could potentially cause problems with the approach.

	(no longer active)
	Comment 8 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #5)
> > > Could we write the highlighter in C++ in Gecko, using this anon content, and
> > > only let the chrome JS toggle it through a simple API? 
> > 
> > Yes!  FWIW this is the approach that I recommended for the touch caret for
> > Firefox OS.  It would be great if we can do something similar for the
> > highlighter as well.
> Sounds good to me.
> 
> Can you confirm my understanding of how the touch caret element is generated:
> - There's a CreateAnonymousContent method in the CanvasFrame that basically
> creates an anonymous DOM element and appends it to the frame
> - TouchCaret.cpp is then responsible for positioning and toggling that
> element
> - The rest is done in ua.css where the caret element is styles (basically
> just with a few images).

Yes, that's correct.

> If we can have something similar for the highlighter and drive this from JS
> chrome code, that would be awesome.
> One thing though: the highlighter (and in fact highlighterS, since we
> already have 2 of them, and plan on having more) is a lot more complex than
> the touch caret. It is based on multiple XUL/SVG elements and requires a lot
> of styling. I'll include screenshots of the highlighters we have now.
> 
> Let me know if the nature of the highlighters could potentially cause
> problems with the approach.

I don't think this is problematic.  But just to double check my sanity, let's ask roc.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 10 years ago

I'd like to minimize the amount of stuff that gets baked into Gecko.

Maybe we could have an API like the following:
[ChromeOnly] interface WindowAnonymousContent {
  void setAttributeForElement(DOMString elementId, DOMString attributeName, DOMString value);
  void CSSStyleDeclaration getStyleForElement(DOMString elementId);
};
partial interface Window {
  [ChromeOnly] WindowAnonymousContent addAnonymousContent(Element elementToClone);
  [ChromeOnly] void removeAnonymousContent(WindowAnonymousContent content);
};

addAnonymousContent would clone some given element subtree and insert the clone, so you don't get direct access to the nodes but you can make them be anything you want. WindowAnonymousContent.setAttributeForElement and WindowAnonymousContent.style would let you dynamically modify elements in the subtree, again without leaking references to actual elements.
We would set !important CSS rules to ensure this anonymous content is always absolutely positioned children of the canvas frame, and also force display:block and a few other things so we don't have to recreate frames for the root of the anonymous subtree.

	Patrick Brosset <:pbro> Assignee
	Comment 10 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> I'd like to minimize the amount of stuff that gets baked into Gecko.
> 
> Maybe we could have an API like the following:
> [ChromeOnly] interface WindowAnonymousContent {
>   void setAttributeForElement(DOMString elementId, DOMString attributeName,
> DOMString value);
>   void CSSStyleDeclaration getStyleForElement(DOMString elementId);
> };
> partial interface Window {
>   [ChromeOnly] WindowAnonymousContent addAnonymousContent(Element
> elementToClone);
>   [ChromeOnly] void removeAnonymousContent(WindowAnonymousContent content);
> };
> 
> addAnonymousContent would clone some given element subtree and insert the
> clone, so you don't get direct access to the nodes but you can make them be
> anything you want. WindowAnonymousContent.setAttributeForElement and
> WindowAnonymousContent.style would let you dynamically modify elements in
> the subtree, again without leaking references to actual elements.
> We would set !important CSS rules to ensure this anonymous content is always
> absolutely positioned children of the canvas frame, and also force
> display:block and a few other things so we don't have to recreate frames for
> the root of the anonymous subtree.
I have to say this sounds very good to me.
This gives us an anonymous, absolutely positioned, element that works the same on all devices, and which we can create from our chrome JS code and modify dynamically.
Another advantage of this API is that the amount of code rewrite on our side is going to be very minor.

Thanks a lot Roc and Ehsan for your help! I guess we only need someone assigned to the bug now.

There's just one thing that this API will make a bit harder compared to what we have today is if we need to dynamically update the content (textcontent) of one of the nodes in the subtree of the anonymous element.
If we need to remove/add nodes, we can simply call removeAnonymousContent and addAnonymousContent again, but in some cases, we want to simply update the textcontent of a given span for instance, to display the size of the highlighted element.

	Patrick Brosset <:pbro> Assignee
	Comment 11 • 10 years ago

I could try to create a patch for this but this would only be my second patch that involves c++ (and I'm completely new to c++, don't have much experience).
If someone can mentor me, I can try and get something ready.

My understanding is that from nsDocShell, or nsGlobalWindow (or any window-level chrome-only accessible interface), we could add a few methods (which roc described) to allow insertion/removal of anonymous content.

It'd be easy to get a reference to the current presShell, and from there to the nsCanvasFrame.
It looks like we can cast it to nsIAnonymousContentCreator and use this to append the node.

Ehsan, who you think could mentor me on this?

	(no longer active)
	Comment 12 • 10 years ago

How soon do you need this?  If you're planning to start working on this in a week or two I would be happy to mentor you but I will be swamped this week and the next at least.

	Patrick Brosset <:pbro> Assignee
	Comment 13 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> How soon do you need this?  If you're planning to start working on this in a
> week or two I would be happy to mentor you but I will be swamped this week
> and the next at least.

Waiting for a couple of weeks sounds ok. We desperately need this for e10s (but also for fennec and fxos). Indeed, when debugging one of these targets, our devtools highlighter is really just a css dashed outline we add to the content node being highlighted.
This is bad because it doesn't show the box-model regions nor extra information about the node size or attributes like our normal highlighter does. Also the outline may mess up with the content page, if the highlighted node already has an outline.
If we don't have a solution when e10s is activated by default this would be a huge step back for the devtools.

So it's pretty important, but not super urgent.

	(no longer active)
	Comment 14 • 10 years ago

Ok, gotcha.  Please ping me again though to make sure that I don't forget.  :-)

	Patrick Brosset <:pbro> Assignee
	Comment 15 • 10 years ago

I've spent some time looking through the code to figure out a way to get started.

It looks like every kind of Frame object implement CreateAnonymousContent which is called when the frame is created.
The CanvasFrame uses this to create the touch and selection carets.

However, we need something dynamic because we want to insert/remove the highlighter whenever we need to. So what I'm trying to do now (to familiarize myself with the code) is I'm using CreateAnonymousContent to create a new element in the CanvasFrame (DevToolsHighlighterContainer or something like that).
This element is styled via ua.css something like this:

pointer-events: none;
position: absolute;
top: 0;
left: 0;
z-index: 2147483648;

so that it's always on top and absolutely positioned, and does not interfere pointer events.
Next, I'm adding a new method to nsIDocShell, available to chrome js: InsertDevToolsHighlighterElement(nsIDOMElement*) which clones the element and appends it to the highlighter container in the canvas frame.

I haven't got that fully working yet and this is just to experiment for now. I'll check the approach with Ehsan when he has time to mentor me.

	(no longer active)
	Comment 16 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #15)
> I've spent some time looking through the code to figure out a way to get
> started.
> 
> It looks like every kind of Frame object implement CreateAnonymousContent
> which is called when the frame is created.
> The CanvasFrame uses this to create the touch and selection carets.

Yes.  Any frame class that wishes to have anonymous content implements the nsIAnonymousContentCreator interface.  CreateAnonymousContent is a method of that interface which needs to be overridden, and it's where the elements are usually created.

> However, we need something dynamic because we want to insert/remove the
> highlighter whenever we need to. So what I'm trying to do now (to
> familiarize myself with the code) is I'm using CreateAnonymousContent to
> create a new element in the CanvasFrame (DevToolsHighlighterContainer or
> something like that).
> This element is styled via ua.css something like this:
> 
> pointer-events: none;
> position: absolute;
> top: 0;
> left: 0;
> z-index: 2147483648;
> 
> so that it's always on top and absolutely positioned, and does not interfere
> pointer events.

This seems to be closer to my idea than to roc's.  In comment 9, roc is suggesting an alternative way of doing things, by exposing an API to create anonymous content on demand.  That is a different model than nsIAnonymousContentCreator (which is invoked during frame construction.)

> Next, I'm adding a new method to nsIDocShell, available to chrome js:
> InsertDevToolsHighlighterElement(nsIDOMElement*) which clones the element
> and appends it to the highlighter container in the canvas frame.
> 
> I haven't got that fully working yet and this is just to experiment for now.
> I'll check the approach with Ehsan when he has time to mentor me.

Is there any reason to not do what comment 9 suggests?

	Patrick Brosset <:pbro> Assignee
	Comment 17 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16)
> This seems to be closer to my idea than to roc's.  In comment 9, roc is
> suggesting an alternative way of doing things, by exposing an API to create
> anonymous content on demand.  That is a different model than
> nsIAnonymousContentCreator (which is invoked during frame construction.)
> 
> ...
> 
> Is there any reason to not do what comment 9 suggests?
No other reason than me not knowing how to go about dynamically inserting anonymous content in the canvas frame. That's why I thought I would start experimenting with this other solution, but merely as a mean to learn, not as a final solution.
Thanks for the feedback so far.

	(no longer active)
	Comment 18 • 10 years ago

I don't think it's worth your time trying to solve this twice.  :-)  Let's just focus on the final solution.

I don't think we currently support dynamically creating these elements.  One option would be to recreate the canvas frame, but I think roc wants to avoid that.  Needinfo-ing him because I don't know exactly how he was planning to handle the creation of these anonymous content.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 10 years ago

I suggest having nsCanvasFrame always create an anonymous container element that can be used to hold these dynamically created children. Normally it will be empty. The dynamically created anonymous elements will not be truly anonymous; they'll be regular children of that anonymous container.

The anonymous container would be position:absolute, left:0, top:0, width:100%, height:100%.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #10)
> There's just one thing that this API will make a bit harder compared to what
> we have today is if we need to dynamically update the content (textcontent)
> of one of the nodes in the subtree of the anonymous element.
> If we need to remove/add nodes, we can simply call removeAnonymousContent
> and addAnonymousContent again, but in some cases, we want to simply update
> the textcontent of a given span for instance, to display the size of the
> highlighted element.

Sure. We could add "void setTextContent(DOMString elementId, DOMString text)" to WindowAnonymousContent to address that.

	Patrick Brosset <:pbro> Assignee
	Comment 20 • 10 years ago

part 1 WIP
Just adding a new container element in nsCanvasFrame::CreateAnonymousContent.

	Patrick Brosset <:pbro> Assignee
	Comment 21 • 10 years ago

part 2 WIP
Trying to clone and insert an element into the canvasFrame's custom container via a new method on nsIDocShell. This doesn't work for now.

	(no longer active)
	Comment 22 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> I suggest having nsCanvasFrame always create an anonymous container element
> that can be used to hold these dynamically created children. Normally it
> will be empty. The dynamically created anonymous elements will not be truly
> anonymous; they'll be regular children of that anonymous container.

Makes sense.

> The anonymous container would be position:absolute, left:0, top:0,
> width:100%, height:100%.

And also presumably a z-index:maxnumber.

> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #10)
> > There's just one thing that this API will make a bit harder compared to what
> > we have today is if we need to dynamically update the content (textcontent)
> > of one of the nodes in the subtree of the anonymous element.
> > If we need to remove/add nodes, we can simply call removeAnonymousContent
> > and addAnonymousContent again, but in some cases, we want to simply update
> > the textcontent of a given span for instance, to display the size of the
> > highlighted element.
> 
> Sure. We could add "void setTextContent(DOMString elementId, DOMString
> text)" to WindowAnonymousContent to address that.

Sounds good to me!

Patrick, please ping me if you needed help either on irc or on the bug.

	Patrick Brosset <:pbro> Assignee
	Comment 23 • 10 years ago

The basics of the API now work with this patch \o/
For instance, this works:

> let browser = gBrowser.selectedTab.linkedBrowser;
> let win = browser.contentWindow;
> let docShell = browser.docShell;
> 
> let node = win.document.querySelector("div");
> let customContent = docShell.insertCustomContent(node);
> 
> customContent.setTextContentForElement("an-element", "noooo");
> customContent.setAttributeForElement("an-element", "id", "something");
> customContent.setTextContentForElement("something", "yeeees");
> 
> docShell.removeCustomContent(customContent);

customContent.getStyleForElement(id) hasn't been implemented yet.

Ehsan, this is where I need mentoring from you. Although this works, the patch is probably awful! I'm not freeing customContent objects when they get removed nor checking for null values, also I'm not sure what to return from idl method implementations when things go wrong, and there's probably a better way to store customContent objects, rather than using an nsTArray and having to loop.
Oh, and I've had to do a really crappy getElementById myself.

So far, I've been able to make this work by looking for inspiration from other part of the code, but I need someone to validate the approach and guide me on the main principals to make this clean.
Thanks!

(note that there are 2 patches: first one just adds an empty customcontent container to the canvasframe, second one is where the API is really implemented).

	(no longer active)
	Comment 24 • 10 years ago

Comment on attachment 8477272 [details] [diff] [review]
bug1020244-2-docshell-api-to-insert-content WIP.patch

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

This is a great start, Patrick!

The biggest comment that I have is about the overall approach.  I don't think this API belongs to the docshell.  Note that these elements are bound to the canvas frame, which is part of the presentation for the document.  A docshell can however survive a given document.  So for example if you use this API on the docshell to create one of these elements, and then navigate away from the page, those elements will no longer be there.  I think this belongs on Window, as roc originally suggested.

Also, you probably want to use WebIDL to implement the interface, and not XPCOM interfaces.  Fortunately our WebIDL bindings layer is documented very well: https://developer.mozilla.org/en/Mozilla/WebIDL_bindings.  You want to add [ChromeOnly] methods to make sure these functions are not accessible by normal web content.

The other issue to think about is the lifetime of these elements.  If for some reason we destroy the canvas frame, these elements are usually destroyed.  Here is a simple test case which does that.  We should figure out what we want to happen in this case.  I guess we may want to ensure that these elements are stored somewhere that can survive a frame destruction, and somehow reattach them later.  nsTextEditorState is one example of code that persists the native anonymous content after recreating frames.

<iframe src="data:text/html,foo"></iframe>
<script>
  var i = document.querySelector("iframe");
  i.style.display = "none";
  // flush layout.  At this point, the canvas frame for the iframe is destroyed.
  document.clientWidth;
  i.style.display = "";
  // flush layout.  At this point, we have a new canvas frame.  What should happen to these custom anon content nodes at this point?
  document.clientWidth;
</script>

::: docshell/base/nsCustomAnonymousContent.cpp
@@ +9,5 @@
> +NS_IMPL_ISUPPORTS(nsCustomAnonymousContent, nsICustomAnonymousContent)
> +
> +nsCustomAnonymousContent::nsCustomAnonymousContent(nsINode* aCustomContent)
> +{
> +  mCustomContent = aCustomContent;

Nit: it's preferred to use initializer lists where possible.

@@ +28,5 @@
> +{
> +  mozilla::dom::Element* element = GetElementById(aElementId);
> +
> +  if (element) {
> +    nsINode* node = static_cast<nsINode*>(element);

Not sure why this is needed.

@@ +59,5 @@
> +  mozilla::dom::Element* element = GetElementById(aElementId);
> +
> +  if (element) {
> +
> +  }

??

@@ +84,5 @@
> +      break;
> +    }
> +  }
> +
> +  return foundElement;

I think this stuff can be simplified through the document's mIdentifierMap.

::: docshell/base/nsCustomAnonymousContent.h
@@ +15,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICUSTOMANONYMOUSCONTENT
> +
> +  nsCustomAnonymousContent(nsINode* aCustomContent);

Nit: make the ctor explicit please.

@@ +16,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICUSTOMANONYMOUSCONTENT
> +
> +  nsCustomAnonymousContent(nsINode* aCustomContent);
> +  nsINode* GetCustomContentNode();

Nit: make this const please.

::: docshell/base/nsDocShell.cpp
@@ +13251,5 @@
> +
> +    // FIXME: AS SOON AS THERE'S A LINK IN THE CLONED ELEMENT, FIREFOX CRASHES
> +    // WITH: ABORT: aRelevantLinkVisited should only be set when we have a
> +    // separate style: 'aRulesIfVisited || !aRelevantLinkVisited', file
> +    // layout/style/nsStyleContext.cpp, line 155

I don't know why that happens...  You might want to ask someone like roc or dbaron.

@@ +13263,5 @@
> +    // Insert the element into the container
> +    rv = container->AppendChildTo(clonedElement->AsContent(), true);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCustomAnonymousContent* customContent = new nsCustomAnonymousContent(clonedElement);

You probably want to stick this into an nsRefPtr.  You should almost never use raw pointers for objects you create yourself.

@@ +13268,5 @@
> +    NS_ENSURE_TRUE(customContent, NS_ERROR_OUT_OF_MEMORY);
> +    nsCOMPtr<nsICustomAnonymousContent> localRef(customContent);
> +
> +    *aCustomContent = localRef;
> +    NS_ADDREF(*aCustomContent);

You could do this like:

nsCOMPtr<nsICustomAnoymousContent> customContent = new nsCustomAnonymousContent(...);
customContent.forget(aCustomContent);

@@ +13300,5 @@
> +        nsINode* node = customContent->GetCustomContentNode();
> +
> +        // Get the canvas frame custom content container element
> +        nsIContent* container = static_cast<nsIContent*>(
> +          presShell->GetCanvasFrame()->GetCustomContentContainer());

Where does this magic live?

::: docshell/base/nsICustomAnonymousContent.idl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * The interface to nsICustomAnonymousContent.
> + * Custom anonymous content can be inserted in the root CanvasFrame via 

Nit: trailing space

::: docshell/base/nsIDocShell.idl
@@ +1011,5 @@
> +  /**
> +   * Insert a clone of the provided element as an anonymous content node in the 
> +   * root canvas frame of this docShell
> +   */
> +  nsICustomAnonymousContent insertCustomContent(in nsIDOMElement elt);

Please update the uuid of all XPCOM interfaces that you modify.  You can use ./mach update-uuids for that.

	Patrick Brosset <:pbro> Assignee
	Comment 25 • 10 years ago

Thanks Ehsan for the feedback. I'm going to update the patch.
I've also added some questions and remarks inline below:

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #24)
> The biggest comment that I have is about the overall approach.  I don't
> think this API belongs to the docshell.  Note that these elements are bound
> to the canvas frame, which is part of the presentation for the document.  A
> docshell can however survive a given document.  So for example if you use
> this API on the docshell to create one of these elements, and then navigate
> away from the page, those elements will no longer be there.  I think this
> belongs on Window, as roc originally suggested.
Definitely. Indeed, I have noticed that when navigating to another page and trying to reuse the customContent created on the previous page, Firefox was crashing :)
I'll move the API over.

> Also, you probably want to use WebIDL to implement the interface, and not
> XPCOM interfaces.  Fortunately our WebIDL bindings layer is documented very
> well: https://developer.mozilla.org/en/Mozilla/WebIDL_bindings.  You want to
> add [ChromeOnly] methods to make sure these functions are not accessible by
> normal web content.
Ok, so this means insertCustomContent and removeCustomContent should instead be defined in Window.webidl, right?
And nsICustomAnonymousContent should be defined in a webidl interface instead?

> The other issue to think about is the lifetime of these elements.  If for
> some reason we destroy the canvas frame, these elements are usually
> destroyed.  Here is a simple test case which does that.  We should figure
> out what we want to happen in this case.  I guess we may want to ensure that
> these elements are stored somewhere that can survive a frame destruction,
> and somehow reattach them later.  nsTextEditorState is one example of code
> that persists the native anonymous content after recreating frames.
> 
> <iframe src="data:text/html,foo"></iframe>
> <script>
>   var i = document.querySelector("iframe");
>   i.style.display = "none";
>   // flush layout.  At this point, the canvas frame for the iframe is
> destroyed.
>   document.clientWidth;
>   i.style.display = "";
>   // flush layout.  At this point, we have a new canvas frame.  What should
> happen to these custom anon content nodes at this point?
>   document.clientWidth;
> </script>
You're right. For the devtools use case, it would make sense that these custom elements survive frame destruction/re-creation for as long as the docShell still exists. A docShell is what the devtools target most of the time, so it would make it easier if we could insert custom content when the devtools toolbox is first opened, and then only remove it either when the toolbox or the tab (i.e. the docShell) is closed.

Having said that, we could totally re-insert the custom content after page navigation. If the lifetime of the CanvasFrame is the same as the one of the Window, that would be easy. We will be inserting custom content only in the top level content window anyway (not in nested iframes).

> ::: docshell/base/nsCustomAnonymousContent.cpp
> @@ +28,5 @@
> > +{
> > +  mozilla::dom::Element* element = GetElementById(aElementId);
> > +
> > +  if (element) {
> > +    nsINode* node = static_cast<nsINode*>(element);
> 
> Not sure why this is needed.
Turns out it's not. I needed to use SetTextContent which is defined on nsINode but not on Element, so I thought I needed to cast here. I'm still unsure why this works though.

> @@ +84,5 @@
> > +      break;
> > +    }
> > +  }
> > +
> > +  return foundElement;
> 
> I think this stuff can be simplified through the document's mIdentifierMap.
I'm not sure how. Can you elaborate?
In this method, I need to find the element whose ID is aElementId and is a descendant of mCustomContent. So if I use the document's mIdentifierMap, I may be finding elements that aren't inside mCustomContent.

> @@ +16,5 @@
> > +  NS_DECL_ISUPPORTS
> > +  NS_DECL_NSICUSTOMANONYMOUSCONTENT
> > +
> > +  nsCustomAnonymousContent(nsINode* aCustomContent);
> > +  nsINode* GetCustomContentNode();
> 
> Nit: make this const please.
So, I tried to do that and first, I'm not sure why exactly this is needed and, second, that led to using const nsINode* in a lot of other places that use GetCustomContentNode, and I had plenty of compiler errors that I haven't been able to solve so far. I will take another look at that.

> @@ +13300,5 @@
> > +        nsINode* node = customContent->GetCustomContentNode();
> > +
> > +        // Get the canvas frame custom content container element
> > +        nsIContent* container = static_cast<nsIContent*>(
> > +          presShell->GetCanvasFrame()->GetCustomContentContainer());
> 
> Where does this magic live?
Sorry, I don't understand the question.

	Patrick Brosset <:pbro> Assignee
	Comment 26 • 10 years ago

Roc, as suggested by Ehsan, I'm asking you the question.

See: https://bugzilla.mozilla.org/attachment.cgi?id=8477272&action=diff#a/docshell/base/nsDocShell.cpp_sec3
In method InsertCustomContent, whenever I try to clone the DOM element and if that element contains a link (<a>), the call fails with: 
ABORT: aRelevantLinkVisited should only be set when we have a separate style: 'aRulesIfVisited || !aRelevantLinkVisited', file layout/style/nsStyleContext.cpp, line 155

Do you know what I can do to avoid this?

	(no longer active)
	Comment 27 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #25)
> > Also, you probably want to use WebIDL to implement the interface, and not
> > XPCOM interfaces.  Fortunately our WebIDL bindings layer is documented very
> > well: https://developer.mozilla.org/en/Mozilla/WebIDL_bindings.  You want to
> > add [ChromeOnly] methods to make sure these functions are not accessible by
> > normal web content.
> Ok, so this means insertCustomContent and removeCustomContent should instead
> be defined in Window.webidl, right?
> And nsICustomAnonymousContent should be defined in a webidl interface
> instead?

Yes to both.

> > The other issue to think about is the lifetime of these elements.  If for
> > some reason we destroy the canvas frame, these elements are usually
> > destroyed.  Here is a simple test case which does that.  We should figure
> > out what we want to happen in this case.  I guess we may want to ensure that
> > these elements are stored somewhere that can survive a frame destruction,
> > and somehow reattach them later.  nsTextEditorState is one example of code
> > that persists the native anonymous content after recreating frames.
> > 
> > <iframe src="data:text/html,foo"></iframe>
> > <script>
> >   var i = document.querySelector("iframe");
> >   i.style.display = "none";
> >   // flush layout.  At this point, the canvas frame for the iframe is
> > destroyed.
> >   document.clientWidth;
> >   i.style.display = "";
> >   // flush layout.  At this point, we have a new canvas frame.  What should
> > happen to these custom anon content nodes at this point?
> >   document.clientWidth;
> > </script>
> You're right. For the devtools use case, it would make sense that these
> custom elements survive frame destruction/re-creation for as long as the
> docShell still exists. A docShell is what the devtools target most of the
> time, so it would make it easier if we could insert custom content when the
> devtools toolbox is first opened, and then only remove it either when the
> toolbox or the tab (i.e. the docShell) is closed.

That is very hard to implement.  We can, however, reattach the previously created native anon content after a frame reconstruction as I suggested before.

> Having said that, we could totally re-insert the custom content after page
> navigation. If the lifetime of the CanvasFrame is the same as the one of the
> Window, that would be easy. We will be inserting custom content only in the
> top level content window anyway (not in nested iframes).

The lifetime of CanvasFrame is shorter than the window.  So we need to deal with this somehow.

> > ::: docshell/base/nsCustomAnonymousContent.cpp
> > @@ +28,5 @@
> > > +{
> > > +  mozilla::dom::Element* element = GetElementById(aElementId);
> > > +
> > > +  if (element) {
> > > +    nsINode* node = static_cast<nsINode*>(element);
> > 
> > Not sure why this is needed.
> Turns out it's not. I needed to use SetTextContent which is defined on
> nsINode but not on Element, so I thought I needed to cast here. I'm still
> unsure why this works though.

Element inherits from nsINode though, so you should be able to call SetTextContent directly on |element|.

> > @@ +84,5 @@
> > > +      break;
> > > +    }
> > > +  }
> > > +
> > > +  return foundElement;
> > 
> > I think this stuff can be simplified through the document's mIdentifierMap.
> I'm not sure how. Can you elaborate?

Look at the implementation of nsDocument::GetElementById.  Basically we have a hashtable of IDs to elements.

> In this method, I need to find the element whose ID is aElementId and is a
> descendant of mCustomContent. So if I use the document's mIdentifierMap, I
> may be finding elements that aren't inside mCustomContent.

you should be able to look up this element in the hashtable and if you found one, walk up its parent chain to ensure that the element is indeed in the native anon subtree that you care about.

> > @@ +16,5 @@
> > > +  NS_DECL_ISUPPORTS
> > > +  NS_DECL_NSICUSTOMANONYMOUSCONTENT
> > > +
> > > +  nsCustomAnonymousContent(nsINode* aCustomContent);
> > > +  nsINode* GetCustomContentNode();
> > 
> > Nit: make this const please.
> So, I tried to do that and first, I'm not sure why exactly this is needed

It's better style, it signals that the method doesn't modify the object.

> and, second, that led to using const nsINode* in a lot of other places that
> use GetCustomContentNode, and I had plenty of compiler errors that I haven't
> been able to solve so far. I will take another look at that.

Hmm, what did you make const?  I was suggesting the method, perhaps you changed nsINode* to const nsINode*?

> > @@ +13300,5 @@
> > > +        nsINode* node = customContent->GetCustomContentNode();
> > > +
> > > +        // Get the canvas frame custom content container element
> > > +        nsIContent* container = static_cast<nsIContent*>(
> > > +          presShell->GetCanvasFrame()->GetCustomContentContainer());
> > 
> > Where does this magic live?
> Sorry, I don't understand the question.

Where does GetCustomContentContainer come from?  I can't find it here: <http://mxr.mozilla.org/mozilla-central/search?string=GetCustomContentContainer>

	Patrick Brosset <:pbro> Assignee
	Comment 28 • 10 years ago

Ehsan, moving the API to WebIDL, on Window, proved harder than I anticipated.
Here's a diff of my latest changes: https://pastebin.mozilla.org/6179336

It does *not* compile right now. There are probably several problems with it. I tried following other examples and the WebIDL documentation as much as possible but there are still things I'm missing.

Here's the error I'm currently facing:

 0:12.22 ../../dist/include/nsCOMPtr.h:561:7: error: no matching constructor for initialization of 'nsCOMPtr_base'
 0:12.22     : NSCAP_CTOR_BASE(aRawPtr)
 0:12.22       ^               ~~~~~~~
 0:12.22 ../../dist/include/nsCOMPtr.h:483:30: note: expanded from macro 'NSCAP_CTOR_BASE'
 0:12.22   #define NSCAP_CTOR_BASE(x) nsCOMPtr_base(x)
 0:12.22                              ^
 0:12.22 /Users/pbrosset/dev/hg/fx-team/dom/base/nsGlobalWindow.cpp:10505:5: note: in instantiation of member function 'nsCOMPtr<mozilla::dom::WindowAnonymousContent>::nsCOMPtr' requested here
 0:12.22     new WindowAnonymousContent(clonedElement);
 0:12.22     ^
 0:12.22 ../../dist/include/nsCOMPtr.h:419:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'mozilla::dom::WindowAnonymousContent *' to 'const nsCOMPtr_base' for 1st argument
 0:12.22 class nsCOMPtr_base
 0:12.22       ^
 0:12.22 ../../dist/include/nsCOMPtr.h:422:12: note: candidate constructor not viable: no known conversion from 'mozilla::dom::WindowAnonymousContent *' to 'nsISupports *' for 1st argument
 0:12.22   explicit nsCOMPtr_base(nsISupports* aRawPtr = 0) : mRawPtr(aRawPtr) {}
 0:12.22            ^

	(no longer active)
	Comment 29 • 10 years ago

I think if you replace nsCOMPtr with nsRefPtr in the code below, that should fix this build error.  nsCOMPtr should only be used for XPCOM interfaces.

+  nsCOMPtr<WindowAnonymousContent> anonymousContent =
+    new WindowAnonymousContent(clonedElement);
+  mAnonymousContents.AppendElement(anonymousContent);

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> Roc, as suggested by Ehsan, I'm asking you the question.
> 
> See:
> https://bugzilla.mozilla.org/attachment.cgi?id=8477272&action=diff#a/
> docshell/base/nsDocShell.cpp_sec3
> In method InsertCustomContent, whenever I try to clone the DOM element and
> if that element contains a link (<a>), the call fails with: 
> ABORT: aRelevantLinkVisited should only be set when we have a separate
> style: 'aRulesIfVisited || !aRelevantLinkVisited', file
> layout/style/nsStyleContext.cpp, line 155
> 
> Do you know what I can do to avoid this?

I don't. Let's ask dbaron.

	Patrick Brosset <:pbro> Assignee
	Comment 31 • 10 years ago

Now using WebIDL bindings and moved the API to Window instead.
I haven't yet taken care of re-inserting the content on canvasframe re-creation, and there's a whole lot of FIXMEs and TODOs in the code, so still largely a draft, but making progress.

Ehsan: if you could take a look at my changes to Window.webidl, nsGlobalWindow.h/.cpp, as well as WindowAnonymousContent.webidl/.h/.cpp, that'd be great.

	(no longer active)
	Comment 32 • 10 years ago

Comment on attachment 8481184 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe WIP.patch

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

This is on the right track!

::: dom/base/WindowAnonymousContent.cpp
@@ +37,5 @@
> +    return;
> +  }
> +
> +  mozilla::ErrorResult rv;
> +  element->SetTextContent(aText, rv);

I'd mark this method [Throws] and just pass aRv here.

@@ +54,5 @@
> +    return;
> +  }
> +
> +  mozilla::ErrorResult rv;
> +  element->SetAttribute(aName, aValue, rv);

Ditto.

@@ +61,5 @@
> +  // }
> +}
> +
> +nsICSSDeclaration*
> +WindowAnonymousContent::GetStyleForElement(const nsAString& aElementId)

Same comments as before...

::: dom/base/WindowAnonymousContent.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_WindowAnonymousContent_h__
> +#define mozilla_dom_WindowAnonymousContent_h__
> +
> +#include "mozilla/dom/Element.h"

Please make the destructor out of line, and forward declare this.

@@ +18,5 @@
> +public:
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(WindowAnonymousContent)
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(WindowAnonymousContent)
> +
> +  explicit WindowAnonymousContent(nsCOMPtr<nsINode> aCustomContent);

Nit: nsINode*

@@ +20,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(WindowAnonymousContent)
> +
> +  explicit WindowAnonymousContent(nsCOMPtr<nsINode> aCustomContent);
> +  nsCOMPtr<nsINode> GetContentNode();
> +  nsISupports* GetParentObject() const { return nullptr; }

You should return a better parent.  You can get one from the content node, for example.

::: dom/base/nsGlobalWindow.cpp
@@ +215,5 @@
>  #include "mozilla/dom/HashChangeEvent.h"
>  #include "mozilla/dom/MozSelfSupportBinding.h"
>  #include "mozilla/dom/PopStateEvent.h"
>  #include "mozilla/dom/PopupBlockedEvent.h"
> +#include "WindowAnonymousContent.h"

Nit: mozilla/dom/...

@@ +10469,5 @@
>    return compStyle.forget();
>  }
>  
> +WindowAnonymousContent*
> +nsGlobalWindow::InsertAnonymousContent(Element& aElement, ErrorResult& aError)

Nit: the common convention is to name the second arg aRv.

@@ +10475,5 @@
> +  FORWARD_TO_OUTER_OR_THROW(InsertAnonymousContent, (aElement, aError),
> +                            aError, nullptr);
> +
> +  if (!mDocShell) {
> +    // FIXME: returning nullptr here crashes the browser

See my comment on the webidl file.

@@ +10487,5 @@
> +    return nullptr;
> +  }
> +
> +  // Get the canvas frame custom content container element
> +  nsIContent* container = static_cast<nsIContent*>(

It'd be really nice if you could avoid this cast.  But please make container an nsCOMPtr<nsIContent>.

@@ +10523,5 @@
> +{
> +  FORWARD_TO_OUTER_OR_THROW(RemoveAnonymousContent, (aContent, aError), aError, );
> +
> +  if (!mDocShell) {
> +    // FIXME: should we throw?

yes.

@@ +10529,5 @@
> +  }
> +
> +  nsCOMPtr<nsIPresShell> presShell = mDocShell->GetPresShell();
> +  if (!presShell) {
> +    return;

Is this the right thing to do?

@@ +10536,5 @@
> +  // Iterate over know customContents to get and remove the right one
> +  for (int32_t i = mAnonymousContents.Length() - 1; i >= 0; --i) {
> +    if (mAnonymousContents[i] == &aContent) {
> +      // Get the node from the customContent
> +      nsINode* node = aContent.GetContentNode();

nsCOMPtr<nsINode>

@@ +10540,5 @@
> +      nsINode* node = aContent.GetContentNode();
> +
> +      // Get the canvas frame custom content container element
> +      nsIContent* container = static_cast<nsIContent*>(
> +        presShell->GetCanvasFrame()->GetCustomContentContainer());

See the above comment.

::: dom/bindings/Bindings.conf
@@ +1594,5 @@
>  },
>  
> +'WindowAnonymousContent': {
> +    'headerFile': 'WindowAnonymousContent.h',
> +},

You can remove this.

::: dom/webidl/Window.webidl
@@ +402,5 @@
> + * The supplied content is cloned and inserted into the window's CanvasFrame.
> + */
> +partial interface Window {
> +  [ChromeOnly, Throws]
> +  WindowAnonymousContent insertAnonymousContent(Element aElement);

You need to decide whether this method can return null or not.  I suggest to keep it [Throws] and keep the return value non-nullable.  Then, in the implementation, you need to call .Throw() on the ErrorResult argument in order to get an exception to be thrown from the JS.  This should address those crashes when you return nullptr from this method.

If however you actually want to return null from this function, you need to make the return type |WindowAnonymousContent?|.

::: dom/webidl/WindowAnonymousContent.webidl
@@ +15,5 @@
> + * DOM cannot be traversed from the node, so that Gecko can remain in control of
> + * the inserted content.
> + */
> +
> +[ChromeOnly] interface WindowAnonymousContent {

Nit: [ChromeOnly] on its own line.

Also, why not call this AnonymousContent?

@@ +19,5 @@
> +[ChromeOnly] interface WindowAnonymousContent {
> +  /**
> +   * Set the text content of an element inside this custom anonymous content
> +   */
> +  // FIXME: should throw when id is not found, or return a boolean at least?

throw.

@@ +26,5 @@
> +  /**
> +   * Set the value of an attribute of an element inside this custom anonymous
> +   * content
> +   */
> +  // FIXME: should throw when id is not found, or return a boolean at least?

throw.

	Patrick Brosset <:pbro> Assignee
	Comment 33 • 10 years ago

I think I've addressed most of the review comments in this new patch. It's not yet ready for review, but I'm progressing.

One thing I haven't yet been able to figure out is where in the code is the canvas frame destruction/construction handled.
At first I wasn't sure when that would happen, but this causes it for example:

document.documentElement.style.display = "none";
let forceSyncReflow = document.documentElement.offsetHeight;
document.documentElement.style.display = "block";

Running this right after having inserted anonymous content in the canvas frame will make that content go away because the canvas frame is being reconstructed for obvious reasons.

I've looked at nsTextEditorState which you said was preserving its editor's state across frames reconstruction but haven't been able to understand much so far.

I need to find a way of being notified that the canvas frame is being recreated so that I can re-append the anonymous content elements to it if any.

	(no longer active)
	Comment 34 • 10 years ago

Frames are transient objects that you cannot hold alive in any way, so you must be prepared to deal with their destruction.  The function that gets run when the canvas frame gets destroyed is nsCanvasFrame::DestroyFrom.  In that function, you can see that we "destroy" the anonymous content we have there (note that DestroyAnonymousContent is kind of a misnomer, what that function really does is unbind the content node from the DOM tree.)

Now, your goal is to reuse these anonymous elements.  The first step to doing that would be to add code to nsCanvasFrame::DestroyFrom to put the native anon content somewhere where we can access it when (if) we get a new nsCanvasFrame.  I think the most obvious place to put it would be on the nsIDocument of the mContent (see nsCanvasFrame::CreateAnonymousContent for example where we get the nsIDocument.)  You can probably store it on nsIDocument directly through an nsCOMPtr<nsIContent> property.

Later on, when you get a new nsCanvasFrame, all of the usual frame construction stuff happens until you get to nsCanvasFrame::CreateAnonymousContent.  In that function, you'd have to check to see if the document has one of these special properties, in which case you can read the property and just use the nsIContent stored there directly instead of creating a new element from scratch, and if you don't find the property (which will happen the first time you get an nsCanvasFrame, for example) then you'll create a native anon element as normal.

	Patrick Brosset <:pbro> Assignee
	Comment 35 • 10 years ago

Here's an update.

The anonymous content container is now stored on the owner document while the canvasFrame is being reconstructed as advised. Let me know if the code I added for this is what you had in mind.

I've also added a couple of new methods to the AnonymousContent webidl to ease tests later on (I know I'll need them for the devtools highlighter tests).

I've been able to migrate most of the highlighter using this patch (see bug 985597) so this means, at least feature-wise, I'm almost there.

I see 3 major remaining things:

- getelementbyid in AnonymousContent.cpp which I still haven't refactored
- I haven't written any tests so far
- it doesn't work at all for XUL windows, in fact it crashes with the following information:

* thread #1: tid = 0x5db5f2, 0x000000010190dfbf XUL`nsGlobalWindow::InsertAnonymousContent(mozilla::dom::Element&, mozilla::ErrorResult&) [inlined] nsCOMPtr<mozilla::dom::Element>::get() const at nsCOMPtr.h:816, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xa0)
    frame #0: 0x000000010190dfbf XUL`nsGlobalWindow::InsertAnonymousContent(mozilla::dom::Element&, mozilla::ErrorResult&) [inlined] nsCOMPtr<mozilla::dom::Element>::get() const at nsCOMPtr.h:816
   813    // Prefer the implicit conversion provided automatically by
   814    // |operator T*() const|. Use |get()| to resolve ambiguity or to get a
   815    // castable pointer.
-> 816    T* get() const { return reinterpret_cast<T*>(mRawPtr); }
   817  
   818    // Makes an nsCOMPtr act like its underlying raw pointer type whenever it is
   819    // used in a context where a raw pointer is expected. It is this operator

	(no longer active)
	Comment 36 • 10 years ago

Comment on attachment 8485727 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe WIP.patch

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

Can you please attach interdiffs so that I don't have to read the entire patch every time?  (I just skimmed over the patch this time.)  Thanks!

About the crash with XUL documents, that's weird.  They should both have a canvas frame and a document.  Please attach a stack trace.

::: content/base/public/nsIDocument.h
@@ +2720,5 @@
> +
> +public:
> +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> +  // can be restored whenever the frame is reconstructed
> +  nsCOMPtr<nsIContent> mFrameCustomContentContainer;

Why not make this an Element pointer?  Also, it shouldn't be public.  And please move it further up next to mXPathEvaluator, for example, for better packing.

::: dom/base/AnonymousContent.cpp
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnonymousContent, mContentNode)

You need to tell the cycle collector about mOwner as well.  (Same in other places where you're adding strong references like this.)

@@ +17,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AnonymousContent, AddRef)
> +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AnonymousContent, Release)
> +
> +AnonymousContent::AnonymousContent(nsINode* aContentNode) :

Please stick to passing Element* pointers around.

::: dom/base/moz.build
@@ +67,5 @@
>      'ScriptSettings.h',
>      'StructuredCloneTags.h',
>      'SubtleCrypto.h',
>      'URL.h',
> +    'URLSearchParams.h'

Please revert this hunk.

::: dom/base/nsGlobalWindow.cpp
@@ +10549,5 @@
> +        return;
> +      }
> +
> +      // Remove the entry in mAnonymousContents
> +      mAnonymousContents.RemoveElementAt(i);

FWIW, I'd remove this from mAnonymousContents first before trying to remove it from the container.

::: dom/base/nsGlobalWindow.h
@@ +879,5 @@
>    void GetSidebar(mozilla::dom::OwningExternalOrWindowProxy& aResult,
>                    mozilla::ErrorResult& aRv);
>    already_AddRefed<mozilla::dom::External> GetExternal(mozilla::ErrorResult& aRv);
>  
> +  mozilla::dom::AnonymousContent*

Please make this method return an already_AddRefed.

::: dom/webidl/AnonymousContent.webidl
@@ +18,5 @@
> +
> +[ChromeOnly]
> +interface AnonymousContent {
> +  /**
> +   * Get the text content of an element inside this custom anonymous content

Nit: period at the end of sentences please.

::: dom/webidl/Window.webidl
@@ +402,5 @@
> + * The supplied content is cloned and inserted into the window's CanvasFrame.
> + */
> +partial interface Window {
> +  [ChromeOnly, Throws]
> +  AnonymousContent insertAnonymousContent(Element aElement);

Please document that this clones aElement.

::: layout/generic/nsCanvasFrame.cpp
@@ +114,5 @@
> +  // document if available
> +  if (doc->mFrameCustomContentContainer) {
> +    mCustomContentContainer = do_QueryInterface(doc->mFrameCustomContentContainer);
> +    aElements.AppendElement(mCustomContentContainer);
> +    doc->mFrameCustomContentContainer = nullptr;

This could be simplified a lot by just storing one reference to this container node.

@@ +135,5 @@
> +                                          er);
> +    NS_ENSURE_SUCCESS(er.ErrorCode(), er.ErrorCode());
> +
> +    nsAutoString classValue;
> +    classValue.AppendLiteral("moz-custom-content-container");

You can just construct classValue from a string.

::: layout/generic/nsCanvasFrame.h
@@ +86,5 @@
>    {
>      return mSelectionCaretsEndElement;
>    }
>  
> +  mozilla::dom::Element* GetCustomContentContainer()

Nit: const.

@@ +142,5 @@
>  
>    nsCOMPtr<mozilla::dom::Element> mTouchCaretElement;
>    nsCOMPtr<mozilla::dom::Element> mSelectionCaretsStartElement;
>    nsCOMPtr<mozilla::dom::Element> mSelectionCaretsEndElement;
> +  nsCOMPtr<mozilla::dom::Element> mCustomContentContainer;

Why do you store two references to this?  Isn't the nsIDocument reference enough here?

	Patrick Brosset <:pbro> Assignee
	Comment 37 • 10 years ago

Here's a stacktrace for the crashes that occur when attempting to insert anonymous content in a XUL window.

This happens when, in nsGlobalWindow.cpp, we do:

  // Get the canvas frame custom content container element
  nsCOMPtr<nsIContent> container = presShell->GetCanvasFrame()
                                            ->GetCustomContentContainer();

which leads to nsCanvasFrame.cpp:

dom::Element*
nsCanvasFrame::GetCustomContentContainer() const
{
  nsCOMPtr<nsIDocument> doc = mContent->OwnerDoc();
  return doc->GetFrameCustomContentContainer();
}

This fails as this is nullptr apparently, so mContent can't be accessed.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 38 • 10 years ago

XUL documents don't have canvas frames, so this approach won't work for them, at least not directly.

If it's really important to support XUL documents, then I think we need to duplicate a lot of what you're adding to nsCanvasFrame into nsRootBoxFrame as well. That's the closest equivalent for XUL documents.

	Patrick Brosset <:pbro> Assignee
	Comment 39 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> XUL documents don't have canvas frames, so this approach won't work for
> them, at least not directly.
> 
> If it's really important to support XUL documents, then I think we need to
> duplicate a lot of what you're adding to nsCanvasFrame into nsRootBoxFrame
> as well. That's the closest equivalent for XUL documents.
No it's not really important for now. The only drawback I can see is when inspecting the browser chrome using the browser toolbox. But we can revert back to using what we were using before (simple css outline on the highlighted element) so it's not a big deal.
Thanks for your help.

	Patrick Brosset <:pbro> Assignee
	Comment 40 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #36)
> Comment on attachment 8485727 [details] [diff] [review]
> bug1020244-insert-anonymous-content-in-canvasframe WIP.patch
> 
> Review of attachment 8485727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please attach interdiffs so that I don't have to read the entire
> patch every time?  (I just skimmed over the patch this time.)  Thanks!
I'll attach an interdiff this time but it unfortunately doesn't show everything (it fails to diff the 2 patch files entirely).

> About the crash with XUL documents, that's weird.  They should both have a
> canvas frame and a document.  Please attach a stack trace.
See roc's previous comment. What I'm doing now is adding a condition for |presShell->GetCanvasFrame()| so that it throws (instead of crash) for xul windows.

> ::: content/base/public/nsIDocument.h
> @@ +2720,5 @@
> > +
> > +public:
> > +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> > +  // can be restored whenever the frame is reconstructed
> > +  nsCOMPtr<nsIContent> mFrameCustomContentContainer;
> 
> Why not make this an Element pointer?  Also, it shouldn't be public.  And
> please move it further up next to mXPathEvaluator, for example, for better
> packing.
Done, also added a getter and setter.

> ::: dom/base/AnonymousContent.cpp
> @@ +12,5 @@
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnonymousContent, mContentNode)
> 
> You need to tell the cycle collector about mOwner as well.  (Same in other
> places where you're adding strong references like this.)
Done.

> @@ +17,5 @@
> > +
> > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AnonymousContent, AddRef)
> > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AnonymousContent, Release)
> > +
> > +AnonymousContent::AnonymousContent(nsINode* aContentNode) :
> 
> Please stick to passing Element* pointers around.
Done.
I'm afraid though that I'm still very unclear about what each type of pointers do and when I should use one or the other. So there are probably many other places to fix in my code. I've read a few resources on MDN, but I'm missing some basic knowledge I think.

> ::: dom/base/moz.build
> @@ +67,5 @@
> >      'ScriptSettings.h',
> >      'StructuredCloneTags.h',
> >      'SubtleCrypto.h',
> >      'URL.h',
> > +    'URLSearchParams.h'
> 
> Please revert this hunk.
Done.

> ::: dom/base/nsGlobalWindow.cpp
> @@ +10549,5 @@
> > +        return;
> > +      }
> > +
> > +      // Remove the entry in mAnonymousContents
> > +      mAnonymousContents.RemoveElementAt(i);
> 
> FWIW, I'd remove this from mAnonymousContents first before trying to remove
> it from the container.
Done.

> ::: dom/base/nsGlobalWindow.h
> @@ +879,5 @@
> >    void GetSidebar(mozilla::dom::OwningExternalOrWindowProxy& aResult,
> >                    mozilla::ErrorResult& aRv);
> >    already_AddRefed<mozilla::dom::External> GetExternal(mozilla::ErrorResult& aRv);
> >  
> > +  mozilla::dom::AnonymousContent*
> 
> Please make this method return an already_AddRefed.
Done.

> ::: dom/webidl/AnonymousContent.webidl
> @@ +18,5 @@
> > +
> > +[ChromeOnly]
> > +interface AnonymousContent {
> > +  /**
> > +   * Get the text content of an element inside this custom anonymous content
> 
> Nit: period at the end of sentences please.
Done.

> ::: dom/webidl/Window.webidl
> @@ +402,5 @@
> > + * The supplied content is cloned and inserted into the window's CanvasFrame.
> > + */
> > +partial interface Window {
> > +  [ChromeOnly, Throws]
> > +  AnonymousContent insertAnonymousContent(Element aElement);
> 
> Please document that this clones aElement.
Well the comment already says that: "The supplied content is cloned ...". Should it be more explicit?

> ::: layout/generic/nsCanvasFrame.cpp
> @@ +114,5 @@
> > +  // document if available
> > +  if (doc->mFrameCustomContentContainer) {
> > +    mCustomContentContainer = do_QueryInterface(doc->mFrameCustomContentContainer);
> > +    aElements.AppendElement(mCustomContentContainer);
> > +    doc->mFrameCustomContentContainer = nullptr;
> 
> This could be simplified a lot by just storing one reference to this
> container node.
Indeed, it is now only stored at nsIDocument level and always accessed through the getter/setter.

> @@ +135,5 @@
> > +                                          er);
> > +    NS_ENSURE_SUCCESS(er.ErrorCode(), er.ErrorCode());
> > +
> > +    nsAutoString classValue;
> > +    classValue.AppendLiteral("moz-custom-content-container");
> 
> You can just construct classValue from a string.
Done.

> ::: layout/generic/nsCanvasFrame.h
> @@ +86,5 @@
> >    {
> >      return mSelectionCaretsEndElement;
> >    }
> >  
> > +  mozilla::dom::Element* GetCustomContentContainer()
> 
> Nit: const.
Done.

> @@ +142,5 @@
> >  
> >    nsCOMPtr<mozilla::dom::Element> mTouchCaretElement;
> >    nsCOMPtr<mozilla::dom::Element> mSelectionCaretsStartElement;
> >    nsCOMPtr<mozilla::dom::Element> mSelectionCaretsEndElement;
> > +  nsCOMPtr<mozilla::dom::Element> mCustomContentContainer;
> 
> Why do you store two references to this?  Isn't the nsIDocument reference
> enough here?
As said above, I changed that.

	Patrick Brosset <:pbro> Assignee
	Comment 41 • 10 years ago

Here's the only interdiff I could get, it doesn't show all changes though.

Oh and, one thing I forgot to mention: I started to add mochitest-plain tests in dom/base/test for the new APIs. Please do let me know if these are the right types of tests for this feature?

	(no longer active)
	Comment 42 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> XUL documents don't have canvas frames, so this approach won't work for
> them, at least not directly.

Ah, right, I forgot!

> If it's really important to support XUL documents, then I think we need to
> duplicate a lot of what you're adding to nsCanvasFrame into nsRootBoxFrame
> as well. That's the closest equivalent for XUL documents.

Hmm, are we fine with the fallback on, let's say, about:newtab?  It's not only the browser chrome that uses XUL.

	(no longer active)
	Comment 43 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #41)
> Oh and, one thing I forgot to mention: I started to add mochitest-plain
> tests in dom/base/test for the new APIs. Please do let me know if these are
> the right types of tests for this feature?

That's a fine place!

	(no longer active)
	Comment 44 • 10 years ago

Comment on attachment 8487205 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe.patch

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

Note that you need to add a test which makes sure that we at least don't crash on XUL documents.

::: content/base/public/nsIDocument.h
@@ +723,5 @@
> +
> +  /**
> +   * Get the frame custom content container
> +   */
> +  Element* GetFrameCustomContentContainer()

Nit: const.

@@ +2733,5 @@
>    bool mDidFireDOMContentLoaded:1;
> +
> +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> +  // can be restored whenever the frame is reconstructed
> +  Element* mFrameCustomContentContainer;

This should be an nsCOMPtr.

::: dom/webidl/Window.webidl
@@ +402,5 @@
> + * The supplied content is cloned and inserted into the window's CanvasFrame.
> + */
> +partial interface Window {
> +  [ChromeOnly, Throws]
> +  AnonymousContent insertAnonymousContent(Element aElement);

You didn't document the cloning here!

	David Baron :dbaron:
	Comment 45 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> > Roc, as suggested by Ehsan, I'm asking you the question.
> > 
> > See:
> > https://bugzilla.mozilla.org/attachment.cgi?id=8477272&action=diff#a/
> > docshell/base/nsDocShell.cpp_sec3
> > In method InsertCustomContent, whenever I try to clone the DOM element and
> > if that element contains a link (<a>), the call fails with: 
> > ABORT: aRelevantLinkVisited should only be set when we have a separate
> > style: 'aRulesIfVisited || !aRelevantLinkVisited', file
> > layout/style/nsStyleContext.cpp, line 155
> > 
> > Do you know what I can do to avoid this?
> 
> I don't. Let's ask dbaron.

We should probably just remove the assertion per the discussion in bug 575675.  (Probably best done in that bug.)

	Patrick Brosset <:pbro> Assignee
	Comment 46 • 10 years ago

This bug has taken a little step back for a while as I'm busy with other things at the moment. I made a little progress still. Here's an updated patch that addresses the following:

> Note that you need to add a test which makes sure that we at least don't
> crash on XUL documents.
Added dom/base/test/test_anonymousContent_xul_window.xul test

> ::: content/base/public/nsIDocument.h
> @@ +723,5 @@
> > +
> > +  /**
> > +   * Get the frame custom content container
> > +   */
> > +  Element* GetFrameCustomContentContainer()
> 
> Nit: const.
Done

> @@ +2733,5 @@
> >    bool mDidFireDOMContentLoaded:1;
> > +
> > +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> > +  // can be restored whenever the frame is reconstructed
> > +  Element* mFrameCustomContentContainer;
> 
> This should be an nsCOMPtr.
Not sure how to get this done, if I change it to nsCOMPtr<Element> the compiler complains as class Element is only forward declared here. And if I include Element.h, I see a whole lot of compilation errors that I'm not sure how to fix.

> ::: dom/webidl/Window.webidl
> @@ +402,5 @@
> > + * The supplied content is cloned and inserted into the window's CanvasFrame.
> > + */
> > +partial interface Window {
> > +  [ChromeOnly, Throws]
> > +  AnonymousContent insertAnonymousContent(Element aElement);
> 
> You didn't document the cloning here!
Done.

I also refactored AnonymousContent::GetElementById.
And I also removed the FIXME comment related to the browser crashing when closing an element that contains a link, since that's been fixed by dbaron.

	(no longer active)
	Comment 48 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #46)
> > @@ +2733,5 @@
> > >    bool mDidFireDOMContentLoaded:1;
> > > +
> > > +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> > > +  // can be restored whenever the frame is reconstructed
> > > +  Element* mFrameCustomContentContainer;
> > 
> > This should be an nsCOMPtr.
> Not sure how to get this done, if I change it to nsCOMPtr<Element> the
> compiler complains as class Element is only forward declared here. And if I
> include Element.h, I see a whole lot of compilation errors that I'm not sure
> how to fix.

In order to fix that, move the body of the getter and setter methods you have to nsIDocumentInlines.h, and make sure to include that header in all .cpp files that use those methods.

	(no longer active)
	Comment 49 • 10 years ago

Comment on attachment 8491306 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v2.patch

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

::: dom/base/AnonymousContent.cpp
@@ +123,5 @@
> +
> +Element*
> +AnonymousContent::GetElementById(const nsAString& aElementId)
> +{
> +  for (nsIContent* kid = mContentNode->GetFirstChild(); kid; kid = kid->GetNextNode(mContentNode)) {

I assume that it's fine to keep this working only for the direct children?

Also, you may want to add a comment here saying that this can be made faster if needed in the future.

	Patrick Brosset <:pbro> Assignee
	Comment 50 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #49)
> Comment on attachment 8491306 [details] [diff] [review]
> bug1020244-insert-anonymous-content-in-canvasframe v2.patch
> 
> Review of attachment 8491306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/AnonymousContent.cpp
> @@ +123,5 @@
> > +
> > +Element*
> > +AnonymousContent::GetElementById(const nsAString& aElementId)
> > +{
> > +  for (nsIContent* kid = mContentNode->GetFirstChild(); kid; kid = kid->GetNextNode(mContentNode)) {
> 
> I assume that it's fine to keep this working only for the direct children?
Does GetNextNode only return direct children though? It seems to me that it traverses descendants of the parameter.

	Patrick Brosset <:pbro> Assignee
	Comment 51 • 10 years ago

I think I fixed Ehsan's latest feedback comments.
A first round of formal review would be great.

	(no longer active)
	Comment 53 • 10 years ago

Comment on attachment 8498990 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v3.patch

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

Thsi is great, Patrick!  Please see my comments below.

::: content/base/public/nsIDocument.h
@@ +713,5 @@
>    bool DidDocumentOpen() {
>      return mDidDocumentOpen;
>    }
>  
> +  void SetFrameCustomContentContainer(nsCOMPtr<mozilla::dom::Element> aFrameCustomContentContainer);

Nit: make the arg a raw pointer.

@@ +714,5 @@
>      return mDidDocumentOpen;
>    }
>  
> +  void SetFrameCustomContentContainer(nsCOMPtr<mozilla::dom::Element> aFrameCustomContentContainer);
> +  nsCOMPtr<mozilla::dom::Element> GetFrameCustomContentContainer() const;

Return a raw pointer.

@@ +2737,5 @@
>    bool mDidFireDOMContentLoaded:1;
> +
> +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> +  // can be restored whenever the frame is reconstructed
> +  nsCOMPtr<Element> mFrameCustomContentContainer;

This needs to participate in cycle collection, otherwise you'd create a non-collectable cycle.

::: dom/base/AnonymousContent.cpp
@@ +124,5 @@
> +Element*
> +AnonymousContent::GetElementById(const nsAString& aElementId)
> +{
> +  // This can be made faster in the future if needed.
> +  for (nsIContent* kid = mContentNode->GetFirstChild(); kid; kid = kid->GetNextNode(mContentNode)) {

Nit: nsCOMPtr.

Also, please break the lines to make this fit 80 columns.

@@ +129,5 @@
> +    if (!kid->IsElement()) {
> +      continue;
> +    }
> +    nsIAtom* id = kid->AsElement()->GetID();
> +    if (id && id->Equals(aElementId)) {

It would be a bit better to atomize aElementId once before the loop and just do pointer comparison here.

::: dom/base/AnonymousContent.h
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_AnonymousContent_h__

Nit: no trailing underscore, please.

@@ +15,5 @@
> +namespace dom {
> +
> +class Element;
> +
> +class AnonymousContent MOZ_FINAL : public nsWrapperCache

I don't see a way for the script to re-gain access to an AnonymousContent object if it loses its reference to it, so this doesn't need to be wrapper-cached.

@@ +56,5 @@
> +private:
> +  ~AnonymousContent();
> +  Element* GetElementById(const nsAString& aElementId);
> +  nsCOMPtr<Element> mContentNode;
> +  nsCOMPtr<nsIDocument> mOwner;

Can't you get the document from mContentNode?

::: dom/base/nsGlobalWindow.cpp
@@ +10399,5 @@
>  
> +already_AddRefed<AnonymousContent>
> +nsGlobalWindow::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
> +{
> +  FORWARD_TO_OUTER_OR_THROW(InsertAnonymousContent, (aElement, aRv), aRv, nullptr);

Why do you want to run this on the outer window?  I would assume it should go on the inner window...

@@ +10424,5 @@
> +
> +  // Clone the node to avoid returning a direct reference
> +  nsCOMPtr<nsINode> clonedElement = node->CloneNode(true, aRv);
> +  if (aRv.Failed()) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);

You don't need to change the error code here.

@@ +10432,5 @@
> +  // Insert the element into the container
> +  nsresult rv;
> +  rv = container->AppendChildTo(clonedElement->AsContent(), true);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);

Neither here.

@@ +10439,5 @@
> +
> +  nsRefPtr<AnonymousContent> anonymousContent =
> +    new AnonymousContent(clonedElement->AsElement());
> +  mAnonymousContents.AppendElement(anonymousContent);
> +  

Trailing whitespace is a deadly sin. ;-)

@@ +10447,5 @@
> +void
> +nsGlobalWindow::RemoveAnonymousContent(AnonymousContent& aContent,
> +                                       ErrorResult& aRv)
> +{
> +  FORWARD_TO_OUTER_OR_THROW(RemoveAnonymousContent, (aContent, aRv), aRv, );

Ditto.

@@ +10477,5 @@
> +
> +      // Remove the node from the container
> +      container->RemoveChild(*node, aRv);
> +      if (aRv.Failed()) {
> +        aRv.Throw(NS_ERROR_UNEXPECTED);

Ditto.

::: dom/base/nsGlobalWindow.h
@@ +109,5 @@
>  class OwningExternalOrWindowProxy;
>  class Selection;
>  class SpeechSynthesis;
>  class WakeLock;
> +class AnonymousContent;

Nit: alphabetical order, please.

::: dom/base/test/test_anonymousContent_manipulate_content.html
@@ +32,5 @@
> +    "Textcontent for the test element is correct after update");
> +
> +  // Test getting/setting/removing attributes
> +  is(anonymousContent.getAttributeForElement("test-element", "class"),
> +    "test-class", "Class attribute for the test element is correct");

Please remove the attribute from the element in the normal DOM and test that doing that doesn't affect the anonymous content.

@@ +38,5 @@
> +  anonymousContent.setAttributeForElement("test-element", "class",
> +    "updated-test-class");
> +  is(anonymousContent.getAttributeForElement("test-element", "class"),
> +    "updated-test-class",
> +    "Class attribute for the test element is correct after update");

Please test that this doesn't change the attribute of the normal element.

@@ +42,5 @@
> +    "Class attribute for the test element is correct after update");
> +
> +  anonymousContent.removeAttributeForElement("test-element", "class");
> +  is(anonymousContent.getAttributeForElement("test-element", "class"), null,
> +    "Class attribute for the test element was removed");

Ditto, ensure that this cannot remove the attribute of the normal element.

@@ +49,5 @@
> +  let style = anonymousContent.getStyleForElement("test-element");
> +  style.backgroundColor = "red";
> +  let style2 = anonymousContent.getStyleForElement("test-element");
> +  is(style2.backgroundColor, "red",
> +    "The background was correctly set for the test element");

Add a test to ensure that style === style2.

::: dom/base/test/test_anonymousContent_xul_window.xul
@@ +14,5 @@
> +  <box>This is a test box</box>
> +
> +  <script type="application/javascript;version=1.8">
> +    // Insert content
> +    let chromeWindow = SpecialPowers.wrap(window);

This is a chrome test, so window should already have the [ChromeOnly] properties.

::: dom/webidl/AnonymousContent.webidl
@@ +1,1 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Note that you should get review from a DOM peer on the WebIDL changes.

::: dom/webidl/Window.webidl
@@ +398,5 @@
> +/**
> + * Chrome window anonymous content management.
> + * This is a Chrome only API that allows inserting absolutely positioned
> + * anonymous content on top of the current page displayed in the window.
> + * The supplied content is cloned and inserted into the window's CanvasFrame.

Please add a comment saying that this only works for HTML documents.

@@ +402,5 @@
> + * The supplied content is cloned and inserted into the window's CanvasFrame.
> + */
> +partial interface Window {
> +  /**
> +   * Clones the provided element and inserts it into the window's CanvasFrame.

Nit: Deep-clones.

::: layout/generic/nsCanvasFrame.cpp
@@ +112,5 @@
>    }
>  
> +  // Either create the custom content container or restore it from the owner
> +  // document if available
> +  dom::Element* customContentContainer = doc->GetFrameCustomContentContainer();

Nit: please make this an nsCOMPtr.

Also, you can add a |using namespace mozilla::dom;| to the beginning of the file if you don't like repeating the dom:: prefix.

@@ +114,5 @@
> +  // Either create the custom content container or restore it from the owner
> +  // document if available
> +  dom::Element* customContentContainer = doc->GetFrameCustomContentContainer();
> +  if (customContentContainer) {
> +    aElements.AppendElement(customContentContainer);

You can hoist this out of the if block down below, and make the condition |!customContentContainer|.

@@ +116,5 @@
> +  dom::Element* customContentContainer = doc->GetFrameCustomContentContainer();
> +  if (customContentContainer) {
> +    aElements.AppendElement(customContentContainer);
> +  } else {
> +    nsRefPtr<dom::NodeInfo> nodeInfo;

Keeping the element on the document, but its creation logic on nsCanvasFrame feels weird.  Perhaps move this code there and expose it as an nsIDocument member?

@@ +122,5 @@
> +                                                   kNameSpaceID_XHTML,
> +                                                   nsIDOMNode::ELEMENT_NODE);
> +    NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
> +
> +    rv = NS_NewHTMLElement(&customContentContainer,

With the comment above address, you want to use getter_AddRefs() here.  That helper gives you an nsIFoo** that, once the callee assigns into it, will automagically stick the pointer into the guts of the nsCOMPtr.

@@ +126,5 @@
> +    rv = NS_NewHTMLElement(&customContentContainer,
> +                           nodeInfo.forget(),
> +                           mozilla::dom::NOT_FROM_PARSER);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    aElements.AppendElement(customContentContainer);

With my comment above, this problem goes away, but technically with your current patch it's possible for us to add an anon content container element which doesn't have the attrs below (because those calls might fail) which breaks down the whole system. :)

@@ +160,5 @@
>    if (mSelectionCaretsEndElement) {
>      aElements.AppendElement(mSelectionCaretsEndElement);
>    }
> +
> +  nsCOMPtr<nsIDocument> doc = mContent->OwnerDoc();

Is mContent guaranteed to be non-null here?  (I don't think so.)

@@ +161,5 @@
>      aElements.AppendElement(mSelectionCaretsEndElement);
>    }
> +
> +  nsCOMPtr<nsIDocument> doc = mContent->OwnerDoc();
> +  dom::Element* customContentContainer = doc->GetFrameCustomContentContainer();

nsCOMPtr.

Also, why not just call GetCustomContentContainer()?

@@ +184,5 @@
> +  // Custom anonymous contents have the same lifetime as the owner document, so
> +  // whenever the canvas frame is destroyed, they are unbound like the touch
> +  // caret but remain stored at the document level and restored again if/when
> +  // the frame gets created.
> +  nsCOMPtr<nsIDocument> doc = mContent->OwnerDoc();

Again, is mContent guaranteed to be non-null here?  And why not just call GetCustomContentContainer()?

::: layout/style/ua.css
@@ +390,5 @@
>    margin: 0px;
>    visibility: hidden;
>  }
> +
> +/* Custom content container in the CanvasFrame, absolutely positioned on top of

Not: s/absolutely/fixed/

@@ +401,5 @@
> +  left: 0;
> +  width: 100%;
> +  height: 100%;
> +
> +  z-index: 2147483648;

I tried to think of other things you may need to have to take care of here, and couldn't think of anything...

	Patrick Brosset <:pbro> Assignee
	Comment 54 • 10 years ago

Thanks Ehsan for the review.
I think I have addressed all points.
Here is a few comments:

> @@ +2737,5 @@
> >    bool mDidFireDOMContentLoaded:1;
> > +
> > +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> > +  // can be restored whenever the frame is reconstructed
> > +  nsCOMPtr<Element> mFrameCustomContentContainer;
> 
> This needs to participate in cycle collection, otherwise you'd create a
> non-collectable cycle.
I've been looking for the right place to do this and added 
NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFrameCustomContentContainer)
in nsDocument.cpp
I hope that's ok.

> ::: dom/base/nsGlobalWindow.cpp
> @@ +10399,5 @@
> >  
> > +already_AddRefed<AnonymousContent>
> > +nsGlobalWindow::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
> > +{
> > +  FORWARD_TO_OUTER_OR_THROW(InsertAnonymousContent, (aElement, aRv), aRv, nullptr);
> 
> Why do you want to run this on the outer window?  I would assume it should
> go on the inner window...
After reading https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows, I think you're right, but tbh this doc page doesn't seem to be complete, and I'm not 100% sure about the differences between outer and inner.

> ::: dom/webidl/AnonymousContent.webidl
> @@ +1,1 @@
> > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> Note that you should get review from a DOM peer on the WebIDL changes.
Ok, will do.

> @@ +116,5 @@
> > +  dom::Element* customContentContainer = doc->GetFrameCustomContentContainer();
> > +  if (customContentContainer) {
> > +    aElements.AppendElement(customContentContainer);
> > +  } else {
> > +    nsRefPtr<dom::NodeInfo> nodeInfo;
> 
> Keeping the element on the document, but its creation logic on nsCanvasFrame
> feels weird.  Perhaps move this code there and expose it as an nsIDocument
> member?
You're right. I've created a new nsIDocument member called GetOrCreateCustomContentContainer() which basically does everything I originally had in nsCanvasFrame. I've also simplified the code a bit.

	Patrick Brosset <:pbro> Assignee
	Comment 55 • 10 years ago

Ehsan, here's the latest patch. Will try to attach an interdiff in a sec.
Henri, as Ehsan suggested, it would be good to get a review from you on the WebIDL changes.

	David Baron :dbaron:
	Comment 57 • 10 years ago

Is there a summary somewhere that describes:
 * when we call into the privileged JS code, and
 * what prevents that code from doing things that it's not allowed to do at that point?

	(no longer active)
	Comment 58 • 10 years ago

Comment on attachment 8500956 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v4.patch

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

r- for the CC comment, but this is really close!

::: content/base/public/nsIDocument.h
@@ +2749,5 @@
>    bool mDidFireDOMContentLoaded:1;
> +
> +  // Temporarily stores the nsCanvasFrame's custom content container so that it
> +  // can be restored whenever the frame is reconstructed
> +  nsCOMPtr<Element> mFrameCustomContentContainer;

Nit: please move this up next to mXPathEvaluator for better packing.

::: content/base/public/nsIDocumentInlines.h
@@ +6,5 @@
>  #ifndef nsIDocumentInlines_h
>  #define nsIDocumentInlines_h
>  
>  #include "nsIDocument.h"
> +#include "mozilla/dom/Element.h"

Please remove this include (and make these methods out of line if you need to.)

@@ +30,5 @@
> +  return mFrameCustomContentContainer;
> +}
> +
> +inline Element*
> +nsIDocument::GetOrCreateFrameCustomContentContainer()

I don't think it's worth keeping this inline, as it's not very small and it's not often called.  Please make it out of line and move it to nsDocument.cpp.

::: dom/base/AnonymousContent.h
@@ +15,5 @@
> +namespace dom {
> +
> +class Element;
> +
> +class AnonymousContent MOZ_FINAL

This class should still participate in cycle collection.

	Patrick Brosset <:pbro> Assignee
	Comment 59 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #57)
> Is there a summary somewhere that describes:
>  * when we call into the privileged JS code, and
>  * what prevents that code from doing things that it's not allowed to do at
> that point?

This new window API is going to be used first from
toolkit/devtools/server/actors/highlighter.js
This module contains a devtools "actor" that will use the API to inject an overlay element on the page, to highlight the currently selected element in the devtools inspector.
The devtools is split into 2 parts:
- the UI part, or debugger, which is the toolbox the user uses, and
- the server part, or debuggee, which is composed of the debugger-server, that listens on a port for devtools debugger protocol messages, and of actors which role is to gather data about the page being debugged.
Let me know if this answers the "when we call into the privileged JS code" question.

As for the "what prevents that code from doing things that it's not allowed to do at that point?" question:
The API only exposes 2 sets of methods:
- window.insertAnonymousContent and window.removeAnonymousContent
- anonymousContent.setAttribute/removeAttribute/getStyle/setTextContent/...
When insertAnonymousContent is called, the provided DOM node is deep-cloned, and the clone is inserted into the nsCanvasFrame. Once done, the API doesn't allow consumers to get a reference of the cloned DOM node at all. The node can only be manipulated by the second set of methods (attributes can be changed, textcontent too, and style properties too).
Consumers can then remove a given anonymouscontent node.

Does this answer the questions?

	Patrick Brosset <:pbro> Assignee
	Comment 60 • 10 years ago

Thanks Ehsan for the reviews!
Here's a new patch that changes the following things:
- revert changes made to nsIDocumentInlines.h and moved all new methods to nsDocument.cpp (it's a little hard to pick a good place to add new methods in files that are as long as this one, let me know if the place I chose is ok)
- made AnonymousContent participate in cycle collection (at least, I think I did :) I've been reading https://developer.mozilla.org/en-US/docs/Interfacing_with_the_XPCOM_cycle_collector and looking at how other classes did it)
- moved the definition of mFrameCustomContentContainer in nsIDocument.h next to mXPathEvaluator

	(no longer active)
	Comment 62 • 10 years ago

Comment on attachment 8503062 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v5.patch

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

::: dom/base/AnonymousContent.h
@@ +20,5 @@
> +class AnonymousContent MOZ_FINAL : public nsISupports
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(AnonymousContent)

You don't need to implement nsISupport for cycle collection if you use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS.  Please look at other examples in the code base to see how other classes do this.  You can use NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING to get an AddRef and Release.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 63 • 10 years ago

Comment on attachment 8503062 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v5.patch

I think I'm not the right person to review the WebIDL changes. I suggest smaug instead.

I'm a bit unhappy about the _moz_anonclass attribute selector in the UA style sheet, though. We've been trying to get rid of _moz attributes in the DOM, so it's sad to see one added. Would it be feasible to have a CSS pseudoclass instead of adding a bogus attribute to the DOM?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 64 • 10 years ago

Comment on attachment 8503062 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v5.patch

I don't mind using _moz attribute in native anonymous content, assuming
selector performance doesn't suffer.


>+nsIDocument::GetOrCreateFrameCustomContentContainer()
>+{
>+  nsCOMPtr<Element> container = GetFrameCustomContentContainer();
Why you need nsCOMPtr here?

>+
>+  if (!container) {
Perhaps 
  if (container) {
    return container;
  }

  nsCOMPtr<Element> newContainer = CreateHTMLElement(nsGkAtoms::div);
  ...

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnonymousContent)
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_INTERFACE_MAP_END
So do we need nsISupports for anything?


>+Element*
>+AnonymousContent::GetElementById(const nsAString& aElementId)
...
>+  for (nsCOMPtr<nsIContent> kid = mContentNode->GetFirstChild(); kid;
No need for nsCOMPtr here. nsIContent* should be fine.



>+nsGlobalWindow::RemoveAnonymousContent(AnonymousContent& aContent,
>+                                       ErrorResult& aRv)
>+{
>+  FORWARD_TO_INNER_OR_THROW(RemoveAnonymousContent, (aContent, aRv), aRv, );
>+  nsIDocShell* docShell = GetDocShell();
>+
>+  if (!docShell) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIPresShell> presShell = docShell->GetPresShell();
>+  if (!presShell) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+
>+  // Iterate over know customContents to get and remove the right one
>+  for (int32_t i = mAnonymousContents.Length() - 1; i >= 0; --i) {
>+    if (mAnonymousContents[i] == &aContent) {
>+      // Get the node from the customContent
>+      nsCOMPtr<Element> node = aContent.GetContentNode();
>+
>+      // Get the canvas frame custom content container element
>+      nsCOMPtr<nsIContent> container = presShell->GetCanvasFrame()
>+                                                ->GetCustomContentContainer();
What guarantees GetCanvasFrame doesn't return null?





>@@ -1190,16 +1197,18 @@ private:
>                                     bool aDoJSFixups,
>                                     bool aNavigate,
>                                     nsIArray *argv,
>                                     nsISupports *aExtraArgument,
>                                     nsIPrincipal *aCalleePrincipal,
>                                     JSContext *aJSCallerContext,
>                                     nsIDOMWindow **aReturn);
> 
>+  nsTArray<nsRefPtr<mozilla::dom::AnonymousContent>> mAnonymousContents;
So what happens to mAnonymousContents when we reuse the (inner) window for another document?
We have elements from the old document and we'd try to use them with the new document?
That would break stuff.
Also, mAnonymousContents isn't cycle collected.

I think nsDocument should store mAnonymousContents.




>+interface AnonymousContent {
>+  /**
>+   * Get the text content of an element inside this custom anonymous content.
>+   */
>+  [Throws]
>+  DOMString getTextContentForElement(DOMString elementId);
>+
>+  /**
>+   * Set the text content of an element inside this custom anonymous content.
>+   */
>+  [Throws]
>+  void setTextContentForElement(DOMString elementId, DOMString text);
I wonder if we could get/set innerHTML. That would be more flexible than just textContent.
But that depends also on whether our native anon content handling in layout can deal changes coming from innerHTML.



> 
>+/**
>+ * Chrome window anonymous content management.
>+ * This is a Chrome-only API that allows inserting fixed positioned anonymous
>+ * content on top of the current page displayed in the window.
>+ * The supplied content is cloned and inserted into the window's CanvasFrame.
>+ * Note that this only works for HTML documents.
>+ */
>+partial interface Window {
>+  /**
>+   * Deep-clones the provided element and inserts it into the window's
>+   * CanvasFrame.
>+   * Returns an AnonymousContent instance that can be used to manipulate the
>+   * inserted element.
>+   */
>+  [ChromeOnly, NewObject, Throws]
>+  AnonymousContent insertAnonymousContent(Element aElement);
>+
>+  /**
>+   * Removes the element inserted into the window's CanvasFrame given an
>+   * AnonymousContent instance.
>+   */
>+  [ChromeOnly, Throws]
>+  void removeAnonymousContent(AnonymousContent aContent);
>+};
I wonder if we really want this API on Window, or should it be on Document.



>+  // Either create the custom content container or restore it from the owner
>+  // document if available.
>+  nsCOMPtr<Element> customContentContainer = doc->GetOrCreateFrameCustomContentContainer();
>+  aElements.AppendElement(customContentContainer);
Why you need nsCOMPtr here? Wouldn't Element* work just fine.
Also, the implementation of GetOrCreateFrameCustomContentContainer may return null, so shouldn't you null check here?



>+  nsCOMPtr<Element> customContentContainer = GetCustomContentContainer();
>+  if (customContentContainer) {
>+    aElements.AppendElement(customContentContainer);
>+  }
Why you need nsCOMPtr here? Wouldn't Element* work just fine.




>+nsCanvasFrame::GetCustomContentContainer() const
>+{
>+  if (!mContent) {
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsIDocument> doc = mContent->OwnerDoc();
>+  return doc->GetFrameCustomContentContainer();
I don't see any reason for the nsCOMPtr.
return mContent->OwnerDoc()->GetFrameCustomContentContainer(); would be faster.



>+}


>+/* Custom content container in the CanvasFrame, fixed positioned on top of
>+   everything else, not reacting to pointer events */
>+div[\_moz_anonclass="mozCustomContentContainer"].moz-custom-content-container {
Someone more familiar with selector matching should review this. We want the best performing selector here.

	Patrick Brosset <:pbro> Assignee
	Comment 65 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #62)
> Comment on attachment 8503062 [details] [diff] [review]
> bug1020244-insert-anonymous-content-in-canvasframe v5.patch
> 
> Review of attachment 8503062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/AnonymousContent.h
> @@ +20,5 @@
> > +class AnonymousContent MOZ_FINAL : public nsISupports
> > +{
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_CLASS(AnonymousContent)
> 
> You don't need to implement nsISupport for cycle collection if you use
> NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS.  Please look at other examples in the
> code base to see how other classes do this.  You can use
> NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING to get an AddRef and
> Release.
Thanks Ehsan for the review. I have made the changes accordingly.

(In reply to Olli Pettay [:smaug] from comment #64)
> Comment on attachment 8503062 [details] [diff] [review]
> bug1020244-insert-anonymous-content-in-canvasframe v5.patch
Thanks Olli. I'm updating my patch right now with your feedback.
Some answers/comments:

> >+nsIDocument::GetOrCreateFrameCustomContentContainer()
> >+{
> >+  nsCOMPtr<Element> container = GetFrameCustomContentContainer();
> Why you need nsCOMPtr here?
Where can I get documentation about when to use one vs. when not? It's my first patch I've had to use those and so far I've just been following what Ehsan and you have been saying without really understanding why.

> >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnonymousContent)
> >+  NS_INTERFACE_MAP_ENTRY(nsISupports)
> >+NS_INTERFACE_MAP_END
> So do we need nsISupports for anything?
Not anymore, I've changed that part according to Ehsan's suggestions and that MAP part is gone.

> >+interface AnonymousContent {
> >+  /**
> >+   * Get the text content of an element inside this custom anonymous content.
> >+   */
> >+  [Throws]
> >+  DOMString getTextContentForElement(DOMString elementId);
> >+
> >+  /**
> >+   * Set the text content of an element inside this custom anonymous content.
> >+   */
> >+  [Throws]
> >+  void setTextContentForElement(DOMString elementId, DOMString text);
> I wonder if we could get/set innerHTML. That would be more flexible than
> just textContent.
> But that depends also on whether our native anon content handling in layout
> can deal changes coming from innerHTML.
It would probably be more flexible indeed. The first consumer (and maybe only consumer for a while) for this API (devtools highlighter) doesn't need to set innerHTML though, the current API works great for our need. So let's keep this for a later patch if need be.

> >+/**
> >+ * Chrome window anonymous content management.
> >+ * This is a Chrome-only API that allows inserting fixed positioned anonymous
> >+ * content on top of the current page displayed in the window.
> >+ * The supplied content is cloned and inserted into the window's CanvasFrame.
> >+ * Note that this only works for HTML documents.
> >+ */
> >+partial interface Window {
> >+  /**
> >+   * Deep-clones the provided element and inserts it into the window's
> >+   * CanvasFrame.
> >+   * Returns an AnonymousContent instance that can be used to manipulate the
> >+   * inserted element.
> >+   */
> >+  [ChromeOnly, NewObject, Throws]
> >+  AnonymousContent insertAnonymousContent(Element aElement);
> >+
> >+  /**
> >+   * Removes the element inserted into the window's CanvasFrame given an
> >+   * AnonymousContent instance.
> >+   */
> >+  [ChromeOnly, Throws]
> >+  void removeAnonymousContent(AnonymousContent aContent);
> >+};
> I wonder if we really want this API on Window, or should it be on Document.
Sounds good to me. Roc was the one to propose the original API. I don't know if Window was proposed for a particular reason other than just making it available globally for the current "page", in which case Document would work just as nicely. It would also be more logical if I move mAnonymousContents to be stored on nsDocument.

> >+/* Custom content container in the CanvasFrame, fixed positioned on top of
> >+   everything else, not reacting to pointer events */
> >+div[\_moz_anonclass="mozCustomContentContainer"].moz-custom-content-container {
> Someone more familiar with selector matching should review this. We want the
> best performing selector here.
Can you suggest a name for this? I don't know who to ask.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 66 • 10 years ago

nsCOMPtr<Element> container = GetFrameCustomContentContainer();
> > Why you need nsCOMPtr here?
> Where can I get documentation about when to use one vs. when not?
Use nsCOMPtr/nsRefPtr when you need to keep an object alive (i.e. nothing else is guaranteed to keep it alive).

> > >+/* Custom content container in the CanvasFrame, fixed positioned on top of
> > >+   everything else, not reacting to pointer events */
> > >+div[\_moz_anonclass="mozCustomContentContainer"].moz-custom-content-container {
> > Someone more familiar with selector matching should review this. We want the
> > best performing selector here.
> Can you suggest a name for this? I don't know who to ask.
dbaron?

	Patrick Brosset <:pbro> Assignee
	Comment 67 • 10 years ago

David, we could use your help on making sure the new selector I'm adding to ua.css isn't going to cause performance issues.
The patch in this bug adds a new (empty by default) native anonymous content element in nsCanvasFrame (just like touch caret) and a new chrome-level API to create content in this element.
We want this element to be fixed positioned on top of the page:

div[\_moz_anonclass="mozCustomContentContainer"].moz-custom-content-container {
  pointer-events: none;

  position: fixed;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;

  z-index: 2147483648;
}

We're wondering if the _moz_anonclass attribute used here is the best choice.
(btw, I guess I can get rid of the moz-custom-content-container class here, I don't think it's useful at all).

	Patrick Brosset <:pbro> Assignee
	Comment 68 • 10 years ago

Here's an updated patch.

	Patrick Brosset <:pbro> Assignee
	Comment 69 • 10 years ago

And here's the interdiff for this patch.

The main change (apart from addressing the review comments) is the move of the API from Window to Document. This helped simplify some of the code.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 70 • 10 years ago

Comment on attachment 8504086 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v6.patch

>+nsIDocument::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsINode> node = do_QueryInterface(&aElement);
You don't need node variable. Element inherits nsINode

>+nsIDocument::RemoveAnonymousContent(AnonymousContent& aContent,
>+                                       ErrorResult& aRv)
Align params.
>+  /**
>+   * Deep-clones the provided element and inserts it into the CanvasFrame.
>+   * Returns an AnonymousContent instance that can be used to manipulate the
>+   * inserted element.
>+   */
>+  [ChromeOnly, NewObject, Throws]
>+  AnonymousContent insertAnonymousContent(Element aElement);
None of the tests seem to call insertAnonymousContent several times.


>+
>+  /**
>+   * Removes the element inserted into the CanvasFrame given an AnonymousContent
>+   * instance.
>+   */
>+  [ChromeOnly, Throws]
>+  void removeAnonymousContent(AnonymousContent aContent);
Could you add tests which call insertAnonymousContent but don't call removeAnonymousContent explicitly 
(such test might catch some leaks)

I think we need some reftests too.

But a followup patch on top of this one is ok.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 71 • 10 years ago

the layout/ bits should be reviewed by some layout peer.

	Patrick Brosset <:pbro> Assignee
	Comment 72 • 10 years ago

Thanks Olli for the review. Here's a new patch with the expected changes (interdiff of the latest changes coming up soon).

Roc: as suggested by Olli, could you please take a look at the layout changes in this patch?

Olli, Ehsan: should I ask you 2 for an extra review?

	Patrick Brosset <:pbro> Assignee
	Comment 73 • 10 years ago

Interdiff of the latest changes.

	Patrick Brosset <:pbro> Assignee
	Comment 74 • 10 years ago

I just discovered a nasty bug in my implementation.

The nsCanvasFrame frame can be reconstructed (for example when toggling the |document.documentElement.style.display| from block to none then to block again).
|nsCanvasFrame::DestroyFrom| does call |nsContentUtils::DestroyAnonymousContent| for the new container, like it does for the other anonymous elements, but our new container is stored at nsDocument level, so that the next time the frame gets reconstructed (in |nsCanvasFrame::CreateAnonymousContent|), we just get the container instead of creating a new one and append it.

I swear I've seen this work in the past, but it doesn't seem to anymore.
The inserted anonymousContent still works as expected, all its methods can be called as usual. For instance:

let style = getStyleForElement(id);
style.color = "red";

does work, and later querying |style.color| does return "red". But the element isn't updated on screen. The color is still the one that was set before. Unless we cause another frame reconstruction. In which case it does switch to red.

If I remove: |nsContentUtils::DestroyAnonymousContent(&container)| from |nsCanvasFrame::DestroyFrom| then it works as expected.

	David Baron :dbaron:
	Comment 75 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #67)
> David, we could use your help on making sure the new selector I'm adding to
> ua.css isn't going to cause performance issues.
> The patch in this bug adds a new (empty by default) native anonymous content
> element in nsCanvasFrame (just like touch caret) and a new chrome-level API
> to create content in this element.
> We want this element to be fixed positioned on top of the page:
> 
> div[\_moz_anonclass="mozCustomContentContainer"].moz-custom-content-
> container {
>   pointer-events: none;
> 
>   position: fixed;
>   top: 0;
>   left: 0;
>   width: 100%;
>   height: 100%;
> 
>   z-index: 2147483648;
> }
> 
> We're wondering if the _moz_anonclass attribute used here is the best choice.

Is there something that prevents Web pages from setting _moz_anonclass attributes?  It's not clear to me whether we should really be using this part of the API surface (or whether the existing touch caret stuff that's there should be).

I wonder if we should have a pseudo-class that marks all of our own anonymous content, usable only from our UA sheets, that we can use to distinguish all of these cases reliably.  (That pseudo-class plus a CSS class, with no attribute selector, would then be sufficient.)

> (btw, I guess I can get rid of the moz-custom-content-container class here,
> I don't think it's useful at all).

The class is probably useful because it should help performance.  See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS .

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 76 • 10 years ago

Hmm, is this needinfo for a review or what? If for a review, could you just ask a review :)
(needinfos and review requests end up to different queues which I tend to process separately.)

	(no longer active)
	Comment 77 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #74)
> I just discovered a nasty bug in my implementation.
> 
> The nsCanvasFrame frame can be reconstructed (for example when toggling the
> |document.documentElement.style.display| from block to none then to block
> again).
> |nsCanvasFrame::DestroyFrom| does call
> |nsContentUtils::DestroyAnonymousContent| for the new container, like it
> does for the other anonymous elements, but our new container is stored at
> nsDocument level, so that the next time the frame gets reconstructed (in
> |nsCanvasFrame::CreateAnonymousContent|), we just get the container instead
> of creating a new one and append it.
> 
> I swear I've seen this work in the past, but it doesn't seem to anymore.
> The inserted anonymousContent still works as expected, all its methods can
> be called as usual. For instance:
> 
> let style = getStyleForElement(id);
> style.color = "red";
> 
> does work, and later querying |style.color| does return "red". But the
> element isn't updated on screen. The color is still the one that was set
> before. Unless we cause another frame reconstruction. In which case it does
> switch to red.
> 
> If I remove: |nsContentUtils::DestroyAnonymousContent(&container)| from
> |nsCanvasFrame::DestroyFrom| then it works as expected.

DestroyAnonymousContent does two things:

1. Null out the nsCOMPtr you pass into it.
2. Unbinds the node from the tree.

Please investigate which one is biting you here.

	(no longer active)
	Comment 78 • 10 years ago

Comment on attachment 8504645 [details] [diff] [review]
interdiff.diff

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

::: b/content/base/src/nsDocument.cpp
@@ +5180,4 @@
>  already_AddRefed<AnonymousContent>
>  nsIDocument::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
>  {
> +  if (!mFrameCustomContentContainer || !&aElement) {

C++ references are supposed to not be null (even though in reality they can be.)  Do you have any reason to suspect that here?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 79 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #75)
> Is there something that prevents Web pages from setting _moz_anonclass
> attributes?  It's not clear to me whether we should really be using this
> part of the API surface (or whether the existing touch caret stuff that's
> there should be).
> 
> I wonder if we should have a pseudo-class that marks all of our own
> anonymous content, usable only from our UA sheets, that we can use to
> distinguish all of these cases reliably.  (That pseudo-class plus a CSS
> class, with no attribute selector, would then be sufficient.)

Yes, we need that.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 80 • 10 years ago

Comment on attachment 8504644 [details] [diff] [review]
bug1020244-insert-anonymous-content-in-canvasframe v7.patch

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

If this is ready for review, please split it up. You can definitely put the content-side API in one patch and the layout code in a different patch. Probably put the tests in a third patch.

	David Baron :dbaron:
	Comment 81 • 10 years ago

I filed a separate bug 1082899 for implementing a pseudo-class to match native-anonymous content, which I believe should be sufficient here and for the touch-caret stuff, although I haven't looked closely at the touch-caret bits.  I'd rather have that work happen in a separate bug than squeeze into this already-large one.

Are you able to pick that up as well?

	Patrick Brosset <:pbro> Assignee
	Comment 82 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #77)
> DestroyAnonymousContent does two things:
> 
> 1. Null out the nsCOMPtr you pass into it.
> 2. Unbinds the node from the tree.
> 
> Please investigate which one is biting you here.
Unbinding the node from the tree is what causes the problem after frame reconstruction.

Passing |false| to both parameters if the |Element::UnbindFromTree(bool aDeep, bool aNullParent)| method seems to get rid of the problem (as in, changing the style of an inserted element actually works), but I don't think that's the right solution. For one, if I apply my devtools highlighter patch on top and try to use the highlighter after frame reconstruction, this causes a crash in RestyleTracker::AddPendingRestyle (I can attach a backtrace if needed).

Any idea how to overcome this problem?
One thing I could do is split the auto-re-insertion code in a separate patch and deliver the basic API first (where any content inserted in the canvasFrame goes away when the frame gets reconstructed). I need to split the patch anyway.

	Patrick Brosset <:pbro> Assignee
	Comment 83 • 10 years ago

Olli, you've already reviewed this as part of a bigger patch that is now split.
This part is for the AnonymousContent WebIDL and implementation.

	Patrick Brosset <:pbro> Assignee
	Comment 84 • 10 years ago

This second part contains:

- changes to Document.webidl: the new insertAnonymousContent and removeAnonymousContent methods,
- changes to nsCanvasFrame to append the inserted anonymous content on frame construction and unbind it on frame destruction,
- the mechanism to store the content at document level and re-insert it in the canvasframe on reconstruction if available,
- changes to ua.css to correctly position the inserted content in the page.

I believe this is ready for review, even if there are 2 know things to be changed:

- the _moz_anonclass="mozCustomContentContainer" attribute should be changed to use a chrome-only pseudo-class (see bug 1082899) instead.
- there's a problem with modifying the inserted content after frame reconstruction (see comment 74).

	Patrick Brosset <:pbro> Assignee
	Comment 85 • 10 years ago

And that's the 3rd part: tests only

Olli: since you last reviewed, I've added a new test that calls insertAnonymousContent multiple times and never calls removeAnonymousContent

	(no longer active)
	Comment 86 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #82)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #77)
> > DestroyAnonymousContent does two things:
> > 
> > 1. Null out the nsCOMPtr you pass into it.
> > 2. Unbinds the node from the tree.
> > 
> > Please investigate which one is biting you here.
> Unbinding the node from the tree is what causes the problem after frame
> reconstruction.
> 
> Passing |false| to both parameters if the |Element::UnbindFromTree(bool
> aDeep, bool aNullParent)| method seems to get rid of the problem (as in,
> changing the style of an inserted element actually works), but I don't think
> that's the right solution. For one, if I apply my devtools highlighter patch
> on top and try to use the highlighter after frame reconstruction, this
> causes a crash in RestyleTracker::AddPendingRestyle (I can attach a
> backtrace if needed).
> 
> Any idea how to overcome this problem?

If everything is working fine, after the frame reconstruction, the same anonymous content that was unbound before should be bound back to the tree here: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4104>.  That should trigger all of the children to be recursively bound here <http://mxr.mozilla.org/mozilla-central/source/content/base/src/Element.cpp#1363> as well.  Can you please check to see which one of those is not working?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 87 • 10 years ago

Comment on attachment 8505500 [details] [diff] [review]
bug1020244-canvasframe-anoncontent-docapi.patch

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

::: dom/webidl/Document.webidl
@@ +356,5 @@
>  };
>  
> +/**
> + * Chrome document anonymous content management.
> + * This is a Chrome-only API that allows inserting fixed positioned anonymous

"chrome-only".

Also, need to clarify that although this is only accessible to chrome, it can be used on non-chrome documents.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 88 • 10 years ago

Comment on attachment 8505500 [details] [diff] [review]
bug1020244-canvasframe-anoncontent-docapi.patch

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

::: layout/generic/nsCanvasFrame.cpp
@@ +134,5 @@
>    }
> +
> +  if (mContent) {
> +    aElements.AppendElement(mContent->OwnerDoc()->GetFrameCustomContentContainer());
> +  }

Don't check mContent for null here. It can't be null.

@@ +157,5 @@
> +  // the frame gets created.
> +  if (mContent) {
> +    nsCOMPtr<Element> container = mContent->OwnerDoc()->GetFrameCustomContentContainer();
> +    nsContentUtils::DestroyAnonymousContent(&container);
> +  }

nsContentUtils::DestroyAnonymousContent runs UnbindFromTree off a scriptrunner. So when we reframe an nsCanvasFrame, we probably end up in nsCSSFrameConstructor::GetAnonymousContent calling BindToTree on this element *before* we do UnbindFromTree. That probably explains badness you're seeing.

How about this:
-- Let nsCanvasFrame create and manage the custom-content-container just like any other anonymous content.
-- Give the document a list of the custom content children.
-- nsCanvasFrame::CreateAnonymousContent adds those children as the children of the custom-content-container it creates.
-- nsCanvasFrame::DestroyFrom replaces the document's custom content children with clones of its current contents. The originals will be destroyed when the custom-content-container goes away. This avoids having to deal with moving nodes between containers.

How does that sound?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 89 • 10 years ago

Comment on attachment 8505501 [details] [diff] [review]
bug1020244-canvasframe-anoncontent-tests.patch

>+  for (let i = 0; i < INSERTED_NB; i ++) {
>+    anonymousContents.push(chromeDocument.insertAnonymousContent(testElement));

Perhaps make each anonymouscontent to have an expando pointing to document
(to make the cycles worse and possibly catching different kinds of leaks).

let anon = chromeDocument.insertAnonymousContent(testElement);
anon.dummyExpando = testElement.ownerDocument;
anonymousContents.push(anon);

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 90 • 10 years ago

Comment on attachment 8505500 [details] [diff] [review]
bug1020244-canvasframe-anoncontent-docapi.patch

>+  ErrorResult rv;
>+  container->SetAttribute(NS_LITERAL_STRING("_moz_anonclass"),
>+                          NS_LITERAL_STRING("mozCustomContentContainer"),
>+                          rv);
So this may change assuming layout folks want some new pseudo class.


>+nsIDocument::InsertAnonymousContent(Element& aElement, ErrorResult& aRv)
>+{
>+  if (!mFrameCustomContentContainer || !&aElement) {
drop  || !&aElement

>+  // Iterate over know customContents to get and remove the right one
>+  for (int32_t i = mAnonymousContents.Length() - 1; i >= 0; --i) {
>+    if (mAnonymousContents[i] == &aContent) {
>+      // Get the node from the customContent
>+      nsCOMPtr<Element> node = aContent.GetContentNode();
>+
>+      // Remove the node from its container
>+      mFrameCustomContentContainer->RemoveChild(*node, aRv);
>+      if (aRv.Failed()) {
>+        return;
>+      }
>+
>+      // Remove the entry in mAnonymousContents
>+      mAnonymousContents.RemoveElementAt(i);
You want to call RemoveElementAt(i) before RemoveChild().
RemoveChild() may in theory cause scripts to run.

(r+ assuming you fix the issues roc found)

	Patrick Brosset <:pbro> Assignee
	Comment 91 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> nsContentUtils::DestroyAnonymousContent runs UnbindFromTree off a
> scriptrunner. So when we reframe an nsCanvasFrame, we probably end up in
> nsCSSFrameConstructor::GetAnonymousContent calling BindToTree on this
> element *before* we do UnbindFromTree. That probably explains badness you're
> seeing.
> 
> How about this:
> -- Let nsCanvasFrame create and manage the custom-content-container just
> like any other anonymous content.
> -- Give the document a list of the custom content children.
> -- nsCanvasFrame::CreateAnonymousContent adds those children as the children
> of the custom-content-container it creates.
> -- nsCanvasFrame::DestroyFrom replaces the document's custom content
> children with clones of its current contents. The originals will be
> destroyed when the custom-content-container goes away. This avoids having to
> deal with moving nodes between containers.
> 
> How does that sound?
This sounds good, but what about the AnonymousContent::GetStyleForElement which returns a reference to the style for a given element? Consumers will expect this reference to still work even if the canvasframe was reframed. But if we clone the nodes, it won't, right?

	Patrick Brosset <:pbro> Assignee
	Comment 92 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #91)
> This sounds good, but what about the AnonymousContent::GetStyleForElement
> which returns a reference to the style for a given element? Consumers will
> expect this reference to still work even if the canvasframe was reframed.
> But if we clone the nodes, it won't, right?
I guess we could replace AnonymousContent::GetStyleForElement by AnonymousContent::SetStyleForElement(id, name, value) and AnonymousContent::GetStyleForElement(id, name)

	Patrick Brosset <:pbro> Assignee
	Comment 93 • 10 years ago

Carrying r+ forward as this is just a minor API change. I removed AnonymousContent::GetStyleForElement because:
- the same can be achieved via AnonymousContent::G/SetAttributeForElement
- the only current consumer of this API (devtools) can do without this method without any problem
- it prevented me from implementing Roc's proposal, which I think is good.

	Patrick Brosset <:pbro> Assignee
	Comment 94 • 10 years ago

Roc: let me know if this is what you had in mind in your previous comment.
nsCanvasFrame now builds and destroys the custom content container like it does the touch caret. This element isn't stored at document level.

At document level though, we do have a list of AnonymousContent objects (which we already had before this patch).
Whenever the frame gets destroyed, we replace each of the AnonymousContent's contentNode with a clone.

Upon frame construction, we iterate over the document's AnonymousContent list and insert the corresponding contentNode.

This seems to pass all my tests so far.

	Patrick Brosset <:pbro> Assignee
	Comment 95 • 10 years ago

Carrying R+ over for this one too as I've just removed occurrences of "getStyleForElement" in this test patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 97 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #91)
> This sounds good, but what about the AnonymousContent::GetStyleForElement
> which returns a reference to the style for a given element? Consumers will
> expect this reference to still work even if the canvasframe was reframed.
> But if we clone the nodes, it won't, right?

Urgh, right.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 98 • 10 years ago

Comment on attachment 8506133 [details] [diff] [review]
bug1020244-canvasframe-anoncontent-docapi v2.patch

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

Excellent!

It's a bit sad to lose the GetStyleForElement API. The CSS DOM interface is nicer to work with than strings, and will get nicer as CSSOM evolves. But we can re-add it later if we work out a way to handle the lifetime issues, without breaking the current API.

	Patrick Brosset <:pbro> Assignee
	Comment 99 • 10 years ago

Carrying r+ over for a minor change on this patch (added an |if shell->GetCanvasFrame()| to avoid the thing from crashing with XUL windows).

	Patrick Brosset <:pbro> Assignee
	Comment 100 • 10 years ago

Also R+'ing this myself as this is a minor test change to make sure the canvasframe is recreated synchronously in dom/base/test/test_anonymousContent_append_after_reflow.html

Pending T-type try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9ade22b48fe9

	Patrick Brosset <:pbro> Assignee
	Comment 101 • 10 years ago

Try build is mostly green apart from a few test failures.

The test that fails is editor/libeditor/tests/test_selection_move_commands.xul
I don't exactly know what the editor module is apart that it's a text/html editor. I don't where it's used these days (thunderbird?). The project home page seems dated: http://www-archive.mozilla.org/editor/

Ehsan, you seem to be module owner, maybe you can help me figure out the problem here.

The test failures are:

1366 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/editor/libeditor/tests/test_selection_move_commands.xul | node after cmd_wordNext - got [object HTMLHtmlElement], expected [object HTMLBodyElement]
1367 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/editor/libeditor/tests/test_selection_move_commands.xul | offset after cmd_wordNext - got 0, expected 23
1371 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/editor/libeditor/tests/test_selection_move_commands.xul | node after cmd_selectWordPrevious - got [object Text], expected [object Text]

The test apparently fails because of this patch: bug1020244-canvasframe-anoncontent-docapi v3.patch

The only thing that seems odd in this test is its usage of domwindowutil's refresh driver functions (advanceTimeAndRefresh and restoreNormalRefresh). I remember talking to bbirtles who suggested not using these as he was (is) working on the web animations API impl, maybe that's what's causing my patch to break this test?

	Patrick Brosset <:pbro> Assignee
	Comment 102 • 10 years ago

The failing assertions come from win.getSelection().anchorNode and .anchorOffset
So it looks like my patch somehow impacted the logic in nsSelection.cpp in a way that it doesn't return the right values anymore, even if no actual content is inserted into the canvasFrame native anonymous container.

	(no longer active)
	Comment 103 • 10 years ago

The editor component is used for two things:

* Plaintext editing for things such as <input type=text>, <textarea> and <xul:textbox>.
* HTML editing for things such as contenteditable and design mode.

The test in question moves the selection in different directions, and it could be possible that our selection starts to somehow see the native anonymous content that your patch has added.  We should definitely protect against that, but it's hard to tell exactly where this happens without debugging.

I suggest you narrow down the exact place in the test where the failure happens and step through the corresponding code to see where we start to jump into the native anonymous subtree.

	(no longer active)
	Comment 104 • 10 years ago

(Note that most of these commands end up calling nsFrameSelection::MoveCaret to do the actual movement of the selection.)

	Patrick Brosset <:pbro> Assignee
	Comment 105 • 10 years ago

I'm leaning towards a problem with how SelectionCarets.cpp interacts with the CanvasFrame.
I see that it uses various nsLayoutUtils methods to get frame objects or coordinates and the new container I added might be messing with this.
If I add 'display:none' to the container style in ua.css, the test passes again.
So it's not that the container is in the canvasFrame, it's that it's shown.

	Patrick Brosset <:pbro> Assignee
	Comment 106 • 10 years ago

Ehsan, I think I need your debugging skills here. I've been circling around the issue all day without actually finding it + I'm super slow debugging this test.

For info, test_selection_move_commands.xul fails when, at line 182, it executes:
testMoveCommand("cmd_wordNext", body, 23);
which executes the command "cmd_wordNext" (which moves the caret to the next word) and asserts that the current selection's anchor node is body.

With my patch, the anchor node is html. I haven't yet found the place where the anchor node gets switched to html. In all the debugging I've done in nsSelection.cpp (in MoveCaret and TakeFocus for instance), the focus node seemed to correctly be body.

	Patrick Brosset <:pbro> Assignee
	Comment 107 • 10 years ago

Oops, forgot to flag NI? for my last comment.

	(no longer active)
	Comment 108 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #105)
> I'm leaning towards a problem with how SelectionCarets.cpp interacts with
> the CanvasFrame.
> I see that it uses various nsLayoutUtils methods to get frame objects or
> coordinates and the new container I added might be messing with this.
> If I add 'display:none' to the container style in ua.css, the test passes
> again.
> So it's not that the container is in the canvasFrame, it's that it's shown.

MoveCaret uses the frame tree in order to navigate to different directions, so the fact that making the container display:none makes the test pass (given that making something display:none makes it not have a frame) suggests that we end up examining the frame corresponding either to the container node you have added or a child of it and then end up going into the weeds.

When running the cmd_wordNext in question, inside MoveCaret you should get to a call to nsIFrame::PeekOffset.  That should probably call nsIFrame::PeekOffset itself (PeekOffset is a virtual function but it's not overridden by most frame types).  In that function when you get to the eSelectWord case, there is a loop where we call nsIFrame::GetFrameFromDirection, and my suspicion is that somewhere in that loop we end up examining a native anonymous frame corresponding to the container node or one of its descendants.  Can you please find which frame that is?  Inside the debugger, when you get to an nsIFrame*, you can call DumpFrameTree() on it in order to dump the full frame tree.  That should help you figure out which frame in the tree you're examining in each iteration of that loop.

	Patrick Brosset <:pbro> Assignee
	Comment 109 • 10 years ago

Awesome! Thanks Ehsan, that helps a lot.
Turns out I wasn't far after all, I've been looking at nsIFrame::PeekOffset at some stage, but couldn't really grasp the logic. I'll spend more time debugging it and use the useful DumpFrameTree.

	Patrick Brosset <:pbro> Assignee
	Comment 110 • 10 years ago

Indeed, dumping the ft shows my new native-anonymous div frame at the end of the tree, below the html block.
So most probably, the loop in nsIFrame::PeekOffset goes all the way to that frame.

nsIFrame::GetFrameFromDirection is to blame, it loops through all the frames in a given direction as long as they are selectable.
So I looked at nsFrame::IsSelectable and realized that "selectable" frames are frames that do not have the user-select style property set to none.

So one way to solve my failing test (and it does work) is to simply add -moz-user-select:none to my container in ua.css.
It makes sense to add it anyway I think, if we take the devtools highlighter use case, the API is aimed at injecting overlays on top of web content, but in a non intrusive way (pointer-events:none was added for this reason in fact).

Or should GetFrameFromDirection be aware of the fact that a frame can be from a native-anon node and skip it if it's the case?

	Patrick Brosset <:pbro> Assignee
	Comment 111 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9b0723d82de6

	(no longer active)
	Comment 112 • 10 years ago

Right now, the IsRootOfNativeAnonymousSubtree() check in nsFrame::GetLastLeaf is what prevents us from descending into the native anonymous subtree of let's say the text area in this test case |foo<textarea>bar</textarea>| when you try to start selecting f and press Shift+Right to extend the selection, for example.

I think it makes sense to add a similar check to GetFrameFromDirection (or perhaps just checking to see if the frame has the NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE flag set) and skipping those.  But I don't think we should modify the IsSelectable logic or make the container -moz-user-select:none.

roc, do you think that makes sense?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 113 • 10 years ago

Yes.

	Patrick Brosset <:pbro> Assignee
	Comment 114 • 10 years ago

Would something like this be ok?

	David Baron :dbaron:
	Comment 115 • 10 years ago

bug 1082899 landed, so please use :moz-native-anonymous instead of more _moz_anonclass attributes

	(no longer active)
	Comment 116 • 10 years ago

Comment on attachment 8510271 [details] [diff] [review]
bug1020244-skip-anonymous-frames v1.patch

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

Yes, exactly!  (I assume that this fixes that failing test. ;)

	Patrick Brosset <:pbro> Assignee
	Comment 117 • 10 years ago

Thanks Ehsan for the review.

Here's a new docapi patch where I've folded the skip-anonymous-frames patch in.
So carrying R+ forward on this one.

Also, I've made a minor change to ua.css in this patch as per comment 115 (i.e. now that bug 1082899 has landed, we can use the new chrome-only pseudo-class to avoid native anonymous elements styles to impact web content).

New pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aaec6432babd

	Patrick Brosset <:pbro> Assignee
	Comment 118 • 10 years ago

Landed this to fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/e661e86180ea
remote:   https://hg.mozilla.org/integration/fx-team/rev/8a8090c2051e
remote:   https://hg.mozilla.org/integration/fx-team/rev/cf419403a2eb

Had to rebase the patches though, attaching the new ones here in a minute.

	Patrick Brosset <:pbro> Assignee
	Comment 119 • 10 years ago

rebased

	Patrick Brosset <:pbro> Assignee
	Comment 120 • 10 years ago

rebased

	Patrick Brosset <:pbro> Assignee
	Comment 121 • 10 years ago

rebased

	Ryan VanderMeulen [:RyanVM]
	Comment 122 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/e661e86180ea
https://hg.mozilla.org/mozilla-central/rev/8a8090c2051e
https://hg.mozilla.org/mozilla-central/rev/cf419403a2eb

	(no longer active)
	Comment 123 • 10 years ago

Congrats, Patrick! :)

	Patrick Brosset <:pbro> Assignee
	Comment 124 • 10 years ago

Thanks Ehsan! Would never have done it without your help.

	Patrick Brosset <:pbro> Assignee
	Comment 125 • 10 years ago

Ehsan, I have a quick follow-up question about this new API:

We made the content inserted in the canvasFrame is fixed positioned, at top z-index, with pointer-events:none on purpose to make it not impact the page in any way (in ua.css).

But there is still a way it impacts the page: anytime e.g. the width/height of an inserted element is changed, that causes a reflow.

It's not a problem in itself, and those reflows are necessary, but it becomes a problem in the devtools because reflows are something we expose via the tools:
- you can turn "reflow logging" on the webconsole for instance
- also the box-model view in the inspector uses reflows to update itself.

So this means that, as soon as we use this API to display the highlighter, and as soon as this highlighter changes shape, that causes reflows which, in turn, are logged in the webconsole, or force the box-model view to refresh necessarily.

So I was wondering if you'd have ideas on how to somehow make our highlighter's reflows "contained". Or if we'd have a way to skip those reflows when listening to them (we use a weakReflowObserver).

	(no longer active)
	Comment 126 • 10 years ago

Can we just remember when we're manipulating these elements and ignore the reflow in that case?

There is a new CSS property <http://dev.w3.org/csswg/css-containment/> that lets us tell the browser that a reflow in one subset of the DOM tree doesn't affect anything outside of it, but we don't implement it yet, and I'm not actually sure if it does what you want.

But thinking about this a bit more, do we also need to worry about the case where something else in the frame tree is dirty and we also change the dimensions of one of these highlighter elements and they both get handled in the same reflow operation the next time that we paint?  In that case my idea above wouldn't really work, and we'd need to explicitly track whether a reflow is caused by a change to these elements or by something else (or both).