1139186 - Allow event listeners on CanvasFrameAnonymousContentHelper

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

We should allow event listeners to be added to elements inside the CanvasFrameAnonymousContentHelper to allow interaction with the highlighter content.  This is a follow up of Bug 1123851.

Comment 1

[Patrick Brosset <:pbro>]

I had attached a patch to bug 1123851 for this.
Brian has done a review of it in bug 1123851 comment 73. Here are his comments (I will attached a new patch soon):

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

This looks pretty good, although there is a problem with all the patches applied having to do with originalTarget getter returning null (see comment below).

Also, it'd be easier to read the diff and eventual blame if the changes to highlighter.js were split into two patches - the first just doing the getElement refactor and the second making the relevant changes for event listeners.  Finally, can we move this patch into Bug 113918?

::: toolkit/devtools/server/actors/highlighter.js
@@ +610,5 @@
> +    if (!listeners) {
> +      return;
> +    }
> +
> +    let targetID = event.originalTarget.id;

I'm seeing errors about event.originalTarget is null with all the patches applied and running this code.  Instead of setting the getter on original target, how about this?

    // Hiding the originalTarget property to avoid exposing references to
    // native anonymous elements. See addEventListenerForElement's comment.
    if (event.originalTarget) {
      event._originalTarget = event.originalTarget;
      delete event.originalTarget;
    }

    let targetID = event._originalTarget.id;

@@ +978,5 @@
>        parent: rootWrapper,
>        attributes: {
>          "id": "elements",
> +        "class": "elements",
> +        "width": this.win.innerWidth,

Is there a benefit to doing the sizing here or can we move the sizing back to CSS for this since we made the pointer-events change opt-in?  Seems like this change plus the one below dealing with zoom is asking for some extra work for all custom highlighters.

@@ +1151,5 @@
>     */
>    _update: function() {
>      setIgnoreLayoutChanges(true);
>  
> +    let svg = this.getElement("elements");

Same comment as above about the sizing possibly moving to css

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
@@ +36,5 @@
> +  function onMouseDown(e, id) {
> +    is(id, "child-element", "The mousedown event was triggered on the element");
> +    mouseDownHandled ++;
> +  }
> +  el.addEventListener("mousedown", onMouseDown);

Can you also add the same event listener on a parent element within the same helper to make sure it fires both times?  It would also be nice to check with a second CanvasFrameAnonymousContentHelper positioned in the same place to make sure they don't interfere with each other (I'm not sure what the intended behavior is here)

Comment 2

[Patrick Brosset <:pbro>]

Attachment: 1 - Refactor to the native anon nodes manipulation in highlighters

Comment 3

[Patrick Brosset <:pbro>]

> Also, it'd be easier to read the diff and eventual blame if the changes to
> highlighter.js were split into two patches - the first just doing the
> getElement refactor and the second making the relevant changes for event
> listeners.
Done. I've attached the refactor patch here, still working on the other one.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +610,5 @@
> > +    if (!listeners) {
> > +      return;
> > +    }
> > +
> > +    let targetID = event.originalTarget.id;
> 
> I'm seeing errors about event.originalTarget is null with all the patches
> applied and running this code.
Can you describe your STR please? I haven't seen this error yet. I'm thinking this might be because of the way the gcli command works. Were you using the command when this occurred?

> Instead of setting the getter on original
> target, how about this?
> 
>     // Hiding the originalTarget property to avoid exposing references to
>     // native anonymous elements. See addEventListenerForElement's comment.
>     if (event.originalTarget) {
>       event._originalTarget = event.originalTarget;
>       delete event.originalTarget;
>     }
> 
>     let targetID = event._originalTarget.id;
delete event.originalTarget doesn't remove the property for me, probably because it only has a getter, and no setter, so it can't be removed. However, it seems a new getter can be defined on top. So I'm sticking with this for now.

> @@ +978,5 @@
> >        parent: rootWrapper,
> >        attributes: {
> >          "id": "elements",
> > +        "class": "elements",
> > +        "width": this.win.innerWidth,
> 
> Is there a benefit to doing the sizing here or can we move the sizing back
> to CSS for this since we made the pointer-events change opt-in?  Seems like
> this change plus the one below dealing with zoom is asking for some extra
> work for all custom highlighters.
I will rework this part.

> ::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
> @@ +36,5 @@
> > +  function onMouseDown(e, id) {
> > +    is(id, "child-element", "The mousedown event was triggered on the element");
> > +    mouseDownHandled ++;
> > +  }
> > +  el.addEventListener("mousedown", onMouseDown);
> 
> Can you also add the same event listener on a parent element within the same
> helper to make sure it fires both times?
So, the problem here is that when you add an event listener on an inserted element, the helper really adds the event on the parent browser, and when that event is emitted, it just looks at event.originalTarget.id and checks if it has the id in its list of listeners. So if I add a listener on a parent element in the test, this listener won't be called, because event.originalTarget will always be the child.
Do you think we should change the way CanvasFrameAnonymousContentHelper events are dispatched?

> It would also be nice to check
> with a second CanvasFrameAnonymousContentHelper positioned in the same place
> to make sure they don't interfere with each other (I'm not sure what the
> intended behavior is here)
Same here, the one on top of the other is going to receive the event only.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> > Also, it'd be easier to read the diff and eventual blame if the changes to
> > highlighter.js were split into two patches - the first just doing the
> > getElement refactor and the second making the relevant changes for event
> > listeners.
> Done. I've attached the refactor patch here, still working on the other one.
> 
> > ::: toolkit/devtools/server/actors/highlighter.js
> > @@ +610,5 @@
> > > +    if (!listeners) {
> > > +      return;
> > > +    }
> > > +
> > > +    let targetID = event.originalTarget.id;
> > 
> > I'm seeing errors about event.originalTarget is null with all the patches
> > applied and running this code.
> Can you describe your STR please? I haven't seen this error yet. I'm
> thinking this might be because of the way the gcli command works. Were you
> using the command when this occurred?
> 

I was using the gcli command with all of the patches from 1123851 applied, and I would run something like 'highlight --geometry .absolute-width-margin'.  Then when I tried to drag the element around I'd see the error.  I can test without using the gcli command if you have a patch that allows it, which I assume will require coming up with some kind of UI to lock the highlighter.  But it could be a very simple UI, just enough to narrow down if the issue had to do with the command.

> > ::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
> > @@ +36,5 @@
> > > +  function onMouseDown(e, id) {
> > > +    is(id, "child-element", "The mousedown event was triggered on the element");
> > > +    mouseDownHandled ++;
> > > +  }
> > > +  el.addEventListener("mousedown", onMouseDown);
> > 
> > Can you also add the same event listener on a parent element within the same
> > helper to make sure it fires both times?
> So, the problem here is that when you add an event listener on an inserted
> element, the helper really adds the event on the parent browser, and when
> that event is emitted, it just looks at event.originalTarget.id and checks
> if it has the id in its list of listeners. So if I add a listener on a
> parent element in the test, this listener won't be called, because
> event.originalTarget will always be the child.
> Do you think we should change the way CanvasFrameAnonymousContentHelper
> events are dispatched?
> 
> > It would also be nice to check
> > with a second CanvasFrameAnonymousContentHelper positioned in the same place
> > to make sure they don't interfere with each other (I'm not sure what the
> > intended behavior is here)
> Same here, the one on top of the other is going to receive the event only.

Hmm, yes, I see the problem.  My main concern would be this scenario:

	child.addEventListener("mouseup", childMouseUp);
	parent.addEventListener("mousedown", () => {
		parent.addEventListener("mouseup", stopDragging);
		parent.addEventListener("mousemove", doDrag);
	});

In this case, the drag would 'stick' (stopDragging wouldn't be called in the case of mouseup) if the mouseup happened on the child.  I think if we call this function `addEventListener`, then people will expect that the handler should be called at all relevant times.  We could alternatively change the name of the function to make the difference clear.

It seems even other event target properties (currentTarget, target) will not be helpful given the scenario.  We could loop up through the parents of the originalTarget and call any handlers if needed...  In that case we might also want to check for calls to preventDefault(), stopPropagation(), etc until we've reimplemented the whole event system :).  Obviously we don't want to go too far with this, but will want to cover what seems like common use-cases for the highlighter API.  

Either way, we should specify both of these cases with *some* behavior in test coverage (even if it's just sticking with the current functionality):
1) Bind listener on a parent and child, and the event happens that outside of the child
2) Bind listener on a parent and child, and the event happens on the child

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> I was using the gcli command with all of the patches from 1123851 applied,
> and I would run something like 'highlight --geometry
> .absolute-width-margin'.  Then when I tried to drag the element around I'd
> see the error.  I can test without using the gcli command if you have a
> patch that allows it, which I assume will require coming up with some kind
> of UI to lock the highlighter.  But it could be a very simple UI, just
> enough to narrow down if the issue had to do with the command.
I've recently been using a script in scratchpad instead. Here are my STR:
- Open the inspector
- Select the node you want to highlight
- Run the following script in a browser-level scratchpad:
> let t = [...gDevTools._toolboxes][0][1];
> let i = t.getPanel("inspector");
> let u = t.highlighterUtils;
> let h;
> u.getHighlighterByType("GeometryEditorHighlighter").then(hi => h = hi);
- Then run this to show the highlighter:
> h.show(i.selection.nodeFront);
- Then run this to hide it again:
> h.hide();

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> > So, the problem here is that when you add an event listener on an inserted
> > element, the helper really adds the event on the parent browser, and when
> > that event is emitted, it just looks at event.originalTarget.id and checks
> > if it has the id in its list of listeners. So if I add a listener on a
> > parent element in the test, this listener won't be called, because
> > event.originalTarget will always be the child.
> > Do you think we should change the way CanvasFrameAnonymousContentHelper
> > events are dispatched?
> Hmm, yes, I see the problem.  My main concern would be this scenario:
> 
> 	child.addEventListener("mouseup", childMouseUp);
> 	parent.addEventListener("mousedown", () => {
> 		parent.addEventListener("mouseup", stopDragging);
> 		parent.addEventListener("mousemove", doDrag);
> 	});
> 
> In this case, the drag would 'stick' (stopDragging wouldn't be called in the
> case of mouseup) if the mouseup happened on the child.  I think if we call
> this function `addEventListener`, then people will expect that the handler
> should be called at all relevant times.  We could alternatively change the
> name of the function to make the difference clear.
> 
> It seems even other event target properties (currentTarget, target) will not
> be helpful given the scenario.  We could loop up through the parents of the
> originalTarget and call any handlers if needed...  In that case we might
> also want to check for calls to preventDefault(), stopPropagation(), etc
> until we've reimplemented the whole event system :).  Obviously we don't
> want to go too far with this, but will want to cover what seems like common
> use-cases for the highlighter API.  
> 
> Either way, we should specify both of these cases with *some* behavior in
> test coverage (even if it's just sticking with the current functionality):
> 1) Bind listener on a parent and child, and the event happens that outside
> of the child
> 2) Bind listener on a parent and child, and the event happens on the child
Re-implementing an event system sounds scary, but you're right, people will be expecting this to behave like the normal addEventListener. I don't think we should be making this thing too complex to start with, especially that it's possible that the right answer is to add addEventListener/removeEventListener at AnonymousContent.cpp level instead.
So I'd rather keep the behavior as it is right now, it's still sufficient for many use cases to be implemented anyway, and change the function name to reflect this behavior.
How does that sound?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Re-implementing an event system sounds scary, but you're right, people will
> be expecting this to behave like the normal addEventListener. I don't think
> we should be making this thing too complex to start with, especially that
> it's possible that the right answer is to add
> addEventListener/removeEventListener at AnonymousContent.cpp level instead.
> So I'd rather keep the behavior as it is right now, it's still sufficient
> for many use cases to be implemented anyway, and change the function name to
> reflect this behavior.
> How does that sound?

Sounds good, as long as we have tests covering the quirks of the current behavior.

Comment 8

[Patrick Brosset <:pbro>]

I noticed something odd while working on tests, maybe Olli would know about this.
The steps are:
- insert DOM nodes into the canvasFrame native anonymous container
- add an event listener on the browser's chromeEventHandler
- simulate an event where the inserted DOM nodes are
- in the event handler, change event.originalTarget
=> If I simulate the event synchronously, right after having added the listener, then e.originalTarget is the documentElement.
=> If instead I wait ~100ms to simulate the event, then e.originalTarget is the expected DOM node inserted into the native anonymous element in the canvasFrame.

This pastebin should explain it better: https://pastebin.mozilla.org/8825272
What you can do to test is:
- open a new tab
- open scratchpad (shift-F4)
- switch it to browser environment in the menu bar
- also open the browser console (ctrl/cmd-shift-J)
- paste the code and execute it.

Thanks for your help.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1139186/pbrosset

/r/5103 - Bug 1139186 - 1 - Refactor to the native anon nodes manipulation in highlighters; r=bgrins
/r/5105 - Bug 1139186 - 2 - Add event handling support to CanvasFrameAnonymousContentHelper; r=bgrins

Pull down these commits:

hg pull review -r 87b8783718e2990ead150f39c9d4429b33796316

Comment 10

[Patrick Brosset <:pbro>]

https://reviewboard.mozilla.org/r/5105/#review4157

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
(Diff revision 1)
> +  // want it to be handled by the native anonymous content.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1139186#c8

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_04.js
(Diff revision 1)
> +  // want it to be handled by the native anonymous content.

https://bugzilla.mozilla.org/show_bug.cgi?id=1139186#c8

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_05.js
(Diff revision 1)
> +  // want it to be handled by the native anonymous content.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1139186#c8

Comment 11

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/5103/#review4163

Ship it (for part 1 only)

::: toolkit/devtools/server/actors/highlighter.js
(Diff revision 1)
> +    return this.markup.getElement(this.ID_CLASS_PREFIX + id);

Maybe define getElement with this implementation on AutoRefreshHighlighter instead of separately on CssTransformHighlighter, GeometryEditorHighlighter, and BoxModelHighlighter?

Comment 12

[Brian Grinstead [:bgrins]]

Can you please attach relevant patches to Bug 1139187 so I can test this patch with them applied?  When I tried grabbing the obsolete ones from 1123851 (plus the gcli patch) I'm getting errors like:

ReferenceError: svg is not defined
Stack trace:
GeometryEditorHighlighter.prototype<._update@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:2338:5

Comment 13

[Patrick Brosset <:pbro>]

Attachment: 1 - Refactor to the native anon nodes manipulation in highlighters

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> Created attachment 8576775 [details] [diff] [review]
> 1 - Refactor to the native anon nodes manipulation in highlighters
You should be able to qimport this into MQ, just on top of fx-team, no need to apply any other patches. This contains both patches in this bug + both patches in bug 1139187 + the gcli highlight command changes.

As discussed on IRC, I've cleaned up the left-over svg error, and I'm not seeing the originalTarget is undefined error you mentioned, nor the target is undefined error.
Try with this patch and let me know if everything's working ok or if you're still having the errors.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> > Created attachment 8576775 [details] [diff] [review]
> > 1 - Refactor to the native anon nodes manipulation in highlighters
> You should be able to qimport this into MQ, just on top of fx-team, no need
> to apply any other patches. This contains both patches in this bug + both
> patches in bug 1139187 + the gcli highlight command changes.
> 
> As discussed on IRC, I've cleaned up the left-over svg error, and I'm not
> seeing the originalTarget is undefined error you mentioned, nor the target
> is undefined error.
> Try with this patch and let me know if everything's working ok or if you're
> still having the errors.

Thanks, everything is working OK with that patch so it must have been that one of the patches in my queue was out of date.

Comment 16

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/5105/#review4335

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_01.js
(Diff revision 2)
> +function getTabActor(win) {

getTabActor seems like something that could be shared in head.js, probably renamed to something like getMockTabActor.  Or we could use an actual TabActor instead of mocking it so we don't have to simulate the 'navigate' event later on, but I'm fine with using a mocked one.

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_05.js
(Diff revision 2)
> +  is(mouseDownHandled.length, 1, "The mousedown event was handled only once");

I'm glad there is now test coverage for this, and this behavior would be fine for v1, but I have a feeling we are going to bump into issues with it quickly (as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1139186#c4).  When I have the rollup patch applied, for instance, I see issues when I start a drag operation on one of the dimension bubbles (nothing happens) and then try to start a drag operation on the highlighted elements (it sticks and doesn't move until I let go of my mouse at which point I'm dragging with my mouse up).  I'm guessing these are related problems - and I wonder how hard it would be to traverse up parent nodes to at least make sure that we call the event when it should happen (ignoring calls to stopPropagation / stopImmediatePropagation).

My main concern with landing now and then following up with changes is if we will have to support old server's behavior.  So if we end up needing to special case some code for old actors in the highlighters it would be awkward, especially for extensions.  I believe that since the CanvasFrameAnonymousHelper is used by the HighlighterActor this would be a potential issue - is that correct?

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_04.js
(Diff revision 2)
> +  events.emit(tabActor, "navigate", {window: doc.defaultView, isTopLevel: true});

Nit: I think you could pass tabActor in as the third param to this emit

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
(Diff revision 2)
> +  setTimeout(() => {

I know you have a ni? out for this, but I'd like to not need a random timeout here.  Would a timeout of 0 work?

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8576055 [details]
MozReview Request: bz://1139186/pbrosset

https://reviewboard.mozilla.org/r/5101/#review4337

Just a couple of notes on part 2

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> https://reviewboard.mozilla.org/r/5105/#review4335
> 
> ::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_01.js
> (Diff revision 2)
> > +function getTabActor(win) {
> 
> getTabActor seems like something that could be shared in head.js, probably
> renamed to something like getMockTabActor.  Or we could use an actual
> TabActor instead of mocking it so we don't have to simulate the 'navigate'
> event later on, but I'm fine with using a mocked one.
Well there's no debugger server running when this test runs, so I'd have to instantiate a TabActor myself. That can be done I guess, but I thought it was easier to just mock the few properties I was using.

> ::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_05.js
> (Diff revision 2)
> > +  is(mouseDownHandled.length, 1, "The mousedown event was handled only once");
> 
> I'm glad there is now test coverage for this, and this behavior would be
> fine for v1, but I have a feeling we are going to bump into issues with it
> quickly (as mentioned in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1139186#c4).  When I have the
> rollup patch applied, for instance, I see issues when I start a drag
> operation on one of the dimension bubbles (nothing happens)
I could fix this one easily even with the current API.
> and then try to
> start a drag operation on the highlighted elements (it sticks and doesn't
> move until I let go of my mouse at which point I'm dragging with my mouse
> up).  I'm guessing these are related problems - and I wonder how hard it
> would be to traverse up parent nodes to at least make sure that we call the
> event when it should happen (ignoring calls to stopPropagation /
> stopImmediatePropagation).
> 
> My main concern with landing now and then following up with changes is if we
> will have to support old server's behavior.  So if we end up needing to
> special case some code for old actors in the highlighters it would be
> awkward, especially for extensions.  I believe that since the
> CanvasFrameAnonymousHelper is used by the HighlighterActor this would be a
> potential issue - is that correct?
Hmm, I'm not sure backward compatibility is a concern here. The client-side never really need to know how the Geometry Highlighter's drag/drop is implemented. Right now the client-side doesn't even know if the highlighter has drag/drop support.
Having said that, I understand and share your concern about this fake event handling mechanism. It's not great. In fact, I've filed bug 1143649 to implement it properly at webidl/C++ level. Just exposing addEventListener/removeEventListener there is really simple, but it may get really complicated if we want to avoid exposing DOM nodes (via event.target for instance).

So I think we have at least 2 choices:
- we can continue with this patch, making sure the add/remove event methods have names and comments that suggest they aren't the real thing, and that a better solution may come in the future (I've changed the name to add/removeNoPropagationEventListener already), knowing that when we implement something better later, compabitility with older servers shouldn't be a problem,
- or we depend on bug 1143649 instead,

I suggest we at least wait until Ehsan answers the NI? in bug 1143649.

> ::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
> (Diff revision 2)
> > +  setTimeout(() => {
> 
> I know you have a ni? out for this, but I'd like to not need a random
> timeout here.  Would a timeout of 0 work?
Yeah, me too, I know how this usually leads to intermittent. The thing is, on my machine at least, it only starts to work after a timeout of about 50~100 ms. And I can't seem to find what's taking time here.

Comment 19

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> I noticed something odd while working on tests, maybe Olli would know about
> this.
> The steps are:
> - insert DOM nodes into the canvasFrame native anonymous container
> - add an event listener on the browser's chromeEventHandler
> - simulate an event where the inserted DOM nodes are
> - in the event handler, change event.originalTarget
I don't understand what this means. You can't change event.originalTarget from a script.


> => If I simulate the event synchronously, right after having added the
> listener, then e.originalTarget is the documentElement.
simulate which way? Perhaps the layout object hasn't been created for the
target

> => If instead I wait ~100ms to simulate the event, then e.originalTarget is
> the expected DOM node inserted into the native anonymous element in the
> canvasFrame.
Assuming the previous is right, layout object would be created at this point

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Olli Pettay [:smaug] from comment #19)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> > I noticed something odd while working on tests, maybe Olli would know about
> > this.
> > The steps are:
> > - insert DOM nodes into the canvasFrame native anonymous container
> > - add an event listener on the browser's chromeEventHandler
> > - simulate an event where the inserted DOM nodes are
> > - in the event handler, change event.originalTarget
> I don't understand what this means. You can't change event.originalTarget
> from a script.
Oh I'm sorry, that's a typo, I meant "log event.originalTarget", not "change".

> > => If I simulate the event synchronously, right after having added the
> > listener, then e.originalTarget is the documentElement.
> simulate which way? Perhaps the layout object hasn't been created for the
> target
Simulate, as in:
EventUtils.synthesizeMouseAtPoint(100, 100, {type: "mousedown"}, win);
which does something like:
var event = aWindow.document.createEvent('MouseEvent');
event.initMouseEvent(...);

> > => If instead I wait ~100ms to simulate the event, then e.originalTarget is
> > the expected DOM node inserted into the native anonymous element in the
> > canvasFrame.
> Assuming the previous is right, layout object would be created at this point
Ok thanks. It looks like I can "wait" for the layout object to be created by forcing a sync reflow from my test script. So I'll be able to remove the timeout.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8576055 [details]
MozReview Request: bz://1139186/pbrosset

/r/5103 - Bug 1139186 - 1 - Refactor to the native anon nodes manipulation in highlighters; r=bgrins
/r/5105 - Bug 1139186 - 2 - Add event handling support to CanvasFrameAnonymousContentHelper; r=bgrins

Pull down these commits:

hg pull review -r bbecd53652a38c51498dab294ea8a2a0d09180c1

Comment 22

[Patrick Brosset <:pbro>]

I think all comments have been addressed in this new patch, except the one related to the more important discussion about the event handling API. See comment 18.
I'll upload a new roll-out patch in a minute.

Comment 23

[Patrick Brosset <:pbro>]

Attachment: geo-highlighter-rollup-patch.diff

New patch for testing. Contains that both patches that are up for review + changes to the gcli highlight command + the move + resize features.

Comment 24

[Patrick Brosset <:pbro>]

To be honest I'm lost with the way mozreview interacts with bugzilla. But anyway, just so this doesn't get lost, I have updated the review request here https://reviewboard.mozilla.org/r/5105/ with a new part 2 patch that handles event bubbling. I think all review comments should now be addressed.
I'll upload a new test patch here.

Comment 25

[Patrick Brosset <:pbro>]

Attachment: geo-highlighter-rollup-patch.diff

Comment 26

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3e40e15c62

Comment 27

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/5105/#review4721

Looks good

::: toolkit/devtools/server/actors/highlighter.js
(Diff revision 4)
> +    // If noone is listening for this type of event yet, add one listener.

Nit: 'no one'

::: toolkit/devtools/server/actors/highlighter.js
(Diff revision 4)
> +    // If noone is listening for event type anymore, remove the listener.

Nit: 'no one'

::: toolkit/devtools/server/actors/highlighter.js
(Diff revision 4)
> +    let handler = listeners.get(targetID);

Nit: you could get rid of this duplicated logic and move it into the loop by making it a `do while`, or move the `node = node.parentNode` into the end of the loop, or whatever

::: toolkit/devtools/server/actors/highlighter.js
(Diff revision 4)
> +      let handler = listeners.get(node.id);

I think if !node.id we should continue -- or disallow passing non-strings into addEventListenerForElement so no one can do:

addEventListenerForElement(null, () => { /* called on any iteration with no id */ })

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8576055 [details]
MozReview Request: bz://1139186/pbrosset

https://reviewboard.mozilla.org/r/5101/#review4723

Ship It!

Comment 29

[Patrick Brosset <:pbro>]

https://reviewboard.mozilla.org/r/5105/#review4759

> I think if !node.id we should continue -- or disallow passing non-strings into addEventListenerForElement so no one can do:
> 
> addEventListenerForElement(null, () => { /* called on any iteration with no id */ })

I ended up adding a check in addEventListenerForElement for this.

Comment 30

[Patrick Brosset <:pbro>]

Thanks for the review Brian. I addressed all comments in the new patch.

Comment 31

[Patrick Brosset <:pbro>]

remote:   https://hg.mozilla.org/integration/fx-team/rev/fe3a966d78e2
remote:   https://hg.mozilla.org/integration/fx-team/rev/ad230a491176

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/fe3a966d78e2
https://hg.mozilla.org/mozilla-central/rev/ad230a491176

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> To be honest I'm lost with the way mozreview interacts with bugzilla. But
> anyway, just so this doesn't get lost, I have updated the review request
> here https://reviewboard.mozilla.org/r/5105/ with a new part 2 patch that
> handles event bubbling. I think all review comments should now be addressed.
> I'll upload a new test patch here.

In my experience, sending updated patches updates the existing review request you made before.  You re-publish it from the MozReview UI.  If any patches have reviewers set, the Bugzilla attachment that points to MozReview should get r? flags re-set on it again as needed.

If it's confusing, let the people in #mozreview know!

Comment 35

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1139186 - 2 - Add event handling support to CanvasFrameAnonymousContentHelper; r=bgrins

Comment 36

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1139186 - 1 - Refactor to the native anon nodes manipulation in highlighters; r=bgrins