We should talk about event listeners in content script

RESOLVED FIXED

Status

Add-on SDK
Documentation
P2
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ochameau, Assigned: wbamberg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cherry-pick-1.3])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
I haven't found any explicit documentation/example about how to register a classic event listener, like onclick. 
I think it will be the first or second kind of common failure when using content scripts. (after or before having access to JS values)

We should find a place in documentation to mention this limitation:
  We can't do: 
    domElement.setAttribute("onclick", "alert('foo')");
  Do this instead:
    domElement.addEventListener("click", function (event) {
      alert('foo');
    }, false);
  (Or:)
    domElement.onclick = function (event) {
      alert('foo');
    }

The following page may be the right location for this?
/dev-guide/addon-development/content-scripts/access.html

Updated

7 years ago
Priority: -- → P2
(Assignee)

Updated

7 years ago
Assignee: nobody → wbamberg
(Assignee)

Comment 1

7 years ago
Hey Alex

A couple of questions.

1) It appears to me as if the "element.onclick = function(){alert('foo');} doesn't work either, see https://builder.addons.mozilla.org/addon/1022722/latest/. What am I doing wrong? :/

2) Is this an effect of using XRayWrapper, and if so, should we link to https://developer.mozilla.org/en/XPCNativeWrapper#Limitations_of_XPCNativeWrapper as well?

> The following page may be the right location for this?
> /dev-guide/addon-development/content-scripts/access.html

Yes, although it seems to be a bit well-hidden there. I feel as if the content script docs don't work that well to get people to understand. I wonder if it would be worth having a tutorial built around a content script example, that could exercise message-passing and these 'common failure' you're talking about.
(Reporter)

Comment 2

7 years ago
(In reply to Will Bamberg [:wbamberg] from comment #1)
> Hey Alex
> 
> A couple of questions.
> 
> 1) It appears to me as if the "element.onclick = function(){alert('foo');}
> doesn't work either, see
> https://builder.addons.mozilla.org/addon/1022722/latest/. What am I doing
> wrong? :/
> 

There is two issues here.
 - on* attributes doesn't work on window object, we should open a bug for that!
 - clickPage was defined *after* you try to assign it.

Here is a working example with on* attribute:
https://builder.addons.mozilla.org/addon/1022783/latest/

> 2) Is this an effect of using XRayWrapper, and if so, should we link to
> https://developer.mozilla.org/en/
> XPCNativeWrapper#Limitations_of_XPCNativeWrapper as well?
> 

We should not speak about XPCNativeWrapper or XrayWrapper.
I mentioned them in content proxy documentation, but this firefox wrapper story is really for us, core developers and low level API developers.
Addon kit users should only know about our custom content Proxies which have a really specific behavior (close to XrayWrappers!)

But you are right, this limitation is implemented by XrayWrappers, here is three equivalent usecases:
  window.setAttribute("onclick", "alert('foo')");
  window..onclick = "alert('foo')";
  window.addEventListener("click", "alert('foo')", false);
These usecases are problematic, because you need to evaluate this JS string "alert('foo')" and it is currently evaluated in the page scope only for the first example! It is very close to our "contentScript" attribute. It is a string of javascript code executed in another context.
So setAttribute works, but two others onclick and addEventListener fails because it only accept functions in order to avoid implementing this hard-to-understand feature.

Having said that I think we should not explain this complex story but just tell that we do not accept strings as callback of on* and addEventListener. And may be implement the same restriction for setAttribute? (but it won't be easy :/)

Am I clear? What's your thoughts on this?

> > The following page may be the right location for this?
> > /dev-guide/addon-development/content-scripts/access.html
> 
> Yes, although it seems to be a bit well-hidden there. I feel as if the
> content script docs don't work that well to get people to understand. I
> wonder if it would be worth having a tutorial built around a content script
> example, that could exercise message-passing and these 'common failure'
> you're talking about.

Your last content script doc improvement was really a huge win! But it was only release on 1.3, right? so it may not have been seen by many people.
Then I agree on avoid a content script tutorial. The widget that inject a content script with tabs.activeTab.attach sounds like a popular usecase and many people keep using pageMod for that :/
(Assignee)

Comment 3

7 years ago
> 
> Here is a working example with on* attribute:
> https://builder.addons.mozilla.org/addon/1022783/latest/
> 

That example seems to be empty(?).

> > 2) Is this an effect of using XRayWrapper, and if so, should we link to
> > https://developer.mozilla.org/en/
> > XPCNativeWrapper#Limitations_of_XPCNativeWrapper as well?
> > 
> 
> We should not speak about XPCNativeWrapper or XrayWrapper.
> I mentioned them in content proxy documentation, but this firefox wrapper
> story is really for us, core developers and low level API developers.
> Addon kit users should only know about our custom content Proxies which have
> a really specific behavior (close to XrayWrappers!)
> 
> But you are right, this limitation is implemented by XrayWrappers, here is
> three equivalent usecases:
>   window.setAttribute("onclick", "alert('foo')");
>   window..onclick = "alert('foo')";
>   window.addEventListener("click", "alert('foo')", false);
> These usecases are problematic, because you need to evaluate this JS string
> "alert('foo')" and it is currently evaluated in the page scope only for the
> first example! It is very close to our "contentScript" attribute. It is a
> string of javascript code executed in another context.
> So setAttribute works, but two others onclick and addEventListener fails
> because it only accept functions in order to avoid implementing this
> hard-to-understand feature.
> 
> Having said that I think we should not explain this complex story but just
> tell that we do not accept strings as callback of on* and addEventListener.
> And may be implement the same restriction for setAttribute? (but it won't be
> easy :/)
> 
> Am I clear? What's your thoughts on this?
> 

I'll think some more about that :).

> > > The following page may be the right location for this?
> > > /dev-guide/addon-development/content-scripts/access.html
> > 
> > Yes, although it seems to be a bit well-hidden there. I feel as if the
> > content script docs don't work that well to get people to understand. I
> > wonder if it would be worth having a tutorial built around a content script
> > example, that could exercise message-passing and these 'common failure'
> > you're talking about.
> 
> Your last content script doc improvement was really a huge win! But it was
> only release on 1.3, right? so it may not have been seen by many people.
> Then I agree on avoid a content script tutorial. The widget that inject a
> content script with tabs.activeTab.attach sounds like a popular usecase and
> many people keep using pageMod for that :/

No, it made it into 1.2: https://addons.mozilla.org/en-US/developers/docs/sdk/1.2/dev-guide/addon-development/web-content.html. It's more that there's a lot of detail in the content script guide, and it might be helpful to lift some of the most basic concepts out into a tutorial, which might be easier for people to navigate. Just a thought.
(Assignee)

Comment 4

7 years ago
Sorry, I"m having trouble understanding this. In the description you say:

>   We can't do: 
>    domElement.setAttribute("onclick", "alert('foo')");
>  Do this instead:
> ...

... but in c2 above you say:

>  window.setAttribute("onclick", "alert('foo')");
>  window..onclick = "alert('foo')";
>  window.addEventListener("click", "alert('foo')", false);
>  setAttribute works, but two others onclick and addEventListener fails 

These statements seem to be contradictory?

Here's an example: https://builder.addons.mozilla.org/addon/1022722/latest/. These work:

1)  document.getElementById("setAttribute").setAttribute("onclick", "alert('set by setAttribute')");

2)  document.getElementById("onclick").onclick = function() {
    alert("set by onclick assignment");
  };

3)  document.getElementById("addEventListener").addEventListener("click", function() {
    alert("set by addEventListener");
  });
(Reporter)

Comment 5

7 years ago
(In reply to Will Bamberg [:wbamberg] from comment #4)

Ok ... If we don't explicitely click "Save" button ... it is not saved :(
But your new example contains what I tried to show you.

> Sorry, I"m having trouble understanding this. In the description you say:
> 
> >   We can't do: 
> >    domElement.setAttribute("onclick", "alert('foo')");
> >  Do this instead:
> > ...
> 
> ... but in c2 above you say:
> 
> >  window.setAttribute("onclick", "alert('foo')");
> >  window..onclick = "alert('foo')";
> >  window.addEventListener("click", "alert('foo')", false);
> >  setAttribute works, but two others onclick and addEventListener fails 
> 
> These statements seem to be contradictory?

You are right, I was wrong. The first case work: setAttribute with second argument being a string.
But developers may not understand that this JS script will be executed in page context instead of content script context.
So if you do:
  window.setAttribute("onclick", "alert('foo')");
"alert('foo')" is executed in page context and won't have access to anything defined in content script.

> 
> Here's an example: https://builder.addons.mozilla.org/addon/1022722/latest/.
> These work:
> 
> 1)  document.getElementById("setAttribute").setAttribute("onclick",
> "alert('set by setAttribute')");

So yes, I was wrong. It works but it is executed in page context.

> 
> 2)  document.getElementById("onclick").onclick = function() {
>     alert("set by onclick assignment");
>   };

There is a major difference between my example:
  window.onclick = "alert('foo')";
I'm using a string as right value. And string won't work.

> 
> 3)  document.getElementById("addEventListener").addEventListener("click",
> function() {
>     alert("set by addEventListener");
>   });

Same than previous example, string vs function. String won't work.

Doesn't hesitate to ping me on irc if that still unclear.
(Assignee)

Comment 6

7 years ago
Created attachment 571666 [details] [diff] [review]
Added section on event listeners
Attachment #571666 - Flags: review?(poirot.alex)
(Reporter)

Comment 7

7 years ago
Comment on attachment 571666 [details] [diff] [review]
Added section on event listeners

To be 100% precise, I think that strings are allowed for:
  onclick = "alert(theMsg)", and,
  addEventListener("click", "alert(theMsg)", false)
But it doesn't work in our content scripts. 
We are entering a level of details that is not really usefull!
Attachment #571666 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 8

7 years ago
(In reply to Alexandre Poirot (:ochameau) from comment #7)
> Comment on attachment 571666 [details] [diff] [review] [diff] [details] [review]
> Added section on event listeners
> 
> To be 100% precise, I think that strings are allowed for:
>   onclick = "alert(theMsg)", and,
>   addEventListener("click", "alert(theMsg)", false)
> But it doesn't work in our content scripts. 

I could not get this to work in page scripts either, see my mad example: https://builder.addons.mozilla.org/addon/1023637/latest/. :( 

> We are entering a level of details that is not really usefull!

Do you think? If it's true that you can do it in page scripts, then the fact that you can't do it in content scripts should be mentioned here I think.
(Reporter)

Comment 9

7 years ago
Oh I was really convinced that it was possible. A guy in the mailing list try to use this pattern...
Regarding MDC documentation it only accept function.
https://developer.mozilla.org/en/DOM/element.onclick
https://developer.mozilla.org/en/DOM/element.addEventListener
I'm really sorry for this waste of time :/
I verified using webconsole and I get the same behavior than your test!
(Assignee)

Comment 10

7 years ago
(In reply to Alexandre Poirot (:ochameau) from comment #9)
> Oh I was really convinced that it was possible. A guy in the mailing list
> try to use this pattern...
> Regarding MDC documentation it only accept function.
> https://developer.mozilla.org/en/DOM/element.onclick
> https://developer.mozilla.org/en/DOM/element.addEventListener
> I'm really sorry for this waste of time :/
> I verified using webconsole and I get the same behavior than your test!

No problem Alex, you're always very generous with your own time. Since it's obviously a source of confusion I'll add a note about it in the patch. I wondered if perhaps other browsers allowed it, but it seems that IE and Chrome, at least, do not.
(Assignee)

Comment 11

7 years ago
Landed as: https://github.com/mozilla/addon-sdk/commit/ab78d7eb73f3dda1ae2605f7980e4ad7932b8abb
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.