624607 - Annotator example add-on

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect)

Description

[Will Bamberg [:wbamberg]]

Attachment: Annotator example

Here's an 'Annotator' example add-on: the idea is that it's simple enough to be easy to understand but complex enough to show how to build something application-like by fitting together various SDK modules.

If you think its reasonable I'd like to use it as the backbone of a tutorial for the developer guide.

It's not very polished: the styling of the panels isn't beautiful, and it's missing some things (like the ability to delete annotations) that would be essential in a real annotator. But I wanted to minimize the amount of complexity that's not related to the SDK exposition.

There are a few problems I know about, which if you have any ideas about I'd really appreciate some help.

1) right-clicking the widget gets captured by the add-on bar, and displays its own context menu as well as the annotation list, despite my best attempts to prevent it from doing so.

2) the annotations should be anchored to the annotation-anchors, and the annotation-list should be anchored to the widget. But I couldn't figure out how to get panels to be anchored. There doesn't even seem to be any unit test code for it. This certainly needs to be documented (but I  need to understand it before I can document it...).

3) the page-mods generate onAttach when they are attached to each page. In my onAttach I take a reference to the worker and keep it in an array, so I can later send it messages when things happen. However, as there is no onDetach event, I end up with orphaned workers in my array when I close pages. Is there any way around this, or should we have an onDetach event?

I've attached a patch, but the code's also at: https://github.com/wbamberg/jetpack-sdk/tree/annotator .

Thanks!

Comment 1

[Brian Warner [:warner :bwarner]]

overall concept sounds good. You might set the package.json's "id" to have an "anonid0-" prefix so that folks who try to build XPIs from it won't get "no private key" warnings. I agree that page-mods needs some kind of onDetach message, or something to let the main addon code manage a list of active tabs.

I'm not sure about the other issues.. we'll need to get someone else to help out with them.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #0)
> 1) right-clicking the widget gets captured by the add-on bar, and displays its
> own context menu as well as the annotation list, despite my best attempts to
> prevent it from doing so.

Hmm, I think someone else mentioned this as well recently.  We shouldn't show the add-on bar's context menu when you click on a widget (just like we don't show the context menu for the navigation toolbar when you click on an item in the toolbar; we only show it when you click on the navigation toolbar itself).

cc:ing Dietrich for confirmation.


> 2) the annotations should be anchored to the annotation-anchors, and the
> annotation-list should be anchored to the widget. But I couldn't figure out how
> to get panels to be anchored. There doesn't even seem to be any unit test code
> for it. This certainly needs to be documented (but I  need to understand it
> before I can document it...).

In theory, you anchor a panel by passing a handle to a DOM element into its show() method.  In practice, the only established anchors for panels are widgets whose panel properties have been set to panels that open automatically when you click the widget.

So there's more work to do here to make panel anchoring work for other kinds of anchors (and for there to be documentation on how to do so).


> 3) the page-mods generate onAttach when they are attached to each page. In my
> onAttach I take a reference to the worker and keep it in an array, so I can
> later send it messages when things happen. However, as there is no onDetach
> event, I end up with orphaned workers in my array when I close pages. Is there
> any way around this, or should we have an onDetach event?

Hmm, we should have an onDetach event.  In the meantime, you can work around the problem by having your content script listen for the "unload" event and send a message to your PageMod instance when it occurs.

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> (In reply to comment #0)
> > 1) right-clicking the widget gets captured by the add-on bar, and displays its
> > own context menu as well as the annotation list, despite my best attempts to
> > prevent it from doing so.
> 
> Hmm, I think someone else mentioned this as well recently.  We shouldn't show
> the add-on bar's context menu when you click on a widget (just like we don't
> show the context menu for the navigation toolbar when you click on an item in
> the toolbar; we only show it when you click on the navigation toolbar itself).
> 
> cc:ing Dietrich for confirmation.

That's a bug. Filed bug 626326.

Comment 4

[Will Bamberg [:wbamberg]]

Attachment: Updated patch for annotator

>  You might set the package.json's "id" to have an
> "anonid0-" prefix so that folks who try to build XPIs from it won't get "no
> private key" warnings. I agree that page-mods needs some kind of onDetach
> message, or something to let the main addon code manage a list of active tabs.

That's done.

> > 1) right-clicking the widget gets captured by the add-on bar, and displays its
> > own context menu as well as the annotation list, despite my best attempts to
> > prevent it from doing so.
> 
> Hmm, I think someone else mentioned this as well recently. 

Yeah, that was probably me :-). You did suggest, as did KWierso, that if I use the right arguments to addEventListener this should stop the context menu being shown, but something like the below is not suppressing the add-on bar's context menu for me:

const widgets = require("widget");

var widget = widgets.Widget({
  label: "Mozilla website",
  contentScriptWhen: 'ready',
  contentScript: 'this.addEventListener("click", function(e) {' +
                        '  postMessage("click");' +
                        '  e.preventDefault();' +
                        '}, true);',
  contentURL: "http://www.mozilla.org/favicon.ico",
  onMessage: function(message) {
    console.log(message);
  }
});

> Hmm, we should have an onDetach event.  In the meantime, you can work around
> the problem by having your content script listen for the "unload" event and
> send a message to your PageMod instance when it occurs.

Works perfectly, thanks.

I think even with these limitations it would be worth having this example in the 1.0b2 release, as these are certainly things people are going to want to do with the SDK and having an example of it might save them some grief.

Comment 5

[Brian Warner [:warner :bwarner]]

I'd like to get this into 1.0b2 . I tried building an XPI and installing it
(in FF4b9), and I couldn't get it to run. Two things jump out as likely
typos:

 * annotation.html uses getElementById('note-box') but only includes a <div>
   with id="annotation" .. seems like these need to match
 * it starts with "const storage = require('simple-storage')" and then does
   "storage.array = []", but Myk and I read the docs to suggest that it ought
   to be "const storage = require('simple-storage').storage" instead.

After changing those, loading a page causes the following error to appear in
the Error Console:

  Error: uncaught exception: [Exception... "Not enough arguments [nsIDOMWindowInternal.postMessage]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: /Users/warner/Library/Application Support/Firefox/Profiles/n9kdrtm6.b9/extensions/anonid0-annotator@jetpack/resources/anonid0-annotator-annotator-data/selector.js :: <TOP_LEVEL> :: line 37"  data: no]

which points at:

     window.addEventListener('unload', function() {
-->      postMessage(['detach']);
       },
       false);

I don't understand that at all.. that call to postMessage() looks ok to me.

What version of Firefox are you testing against? When I try it against FF4b8,
I don't get the errors above, but when I click on the add-on-bar button (does
it need a simple icon?), nothing happens except for this error appearing in
the console:

  Error: parent.getAttribute is not a function
  Source File: resource://gre/modules/WindowDraggingUtils.jsm
  Line: 63

If we can make this work with b9, we can land it before the freeze.

Comment 6

[Will Bamberg [:wbamberg]]

I'm testing it with b9, running on OS X. 

'cfx run' works for me, but when I build an XPI and try to install it Firefox tells me that my XPI is not compatible with b9. But I get that same error with the other examples...

>  * annotation.html uses getElementById('note-box') but only includes a <div>
>    with id="annotation" .. seems like these need to match
>  * it starts with "const storage = require('simple-storage')" and then does
>    "storage.array = []", but Myk and I read the docs to suggest that it ought
>    to be "const storage = require('simple-storage').storage" instead.

Yeah, they both look like problems. Although I'm rather surprised it works at all given the second problem.

> After changing those, loading a page causes the following error to appear in
> the Error Console:
> 
>   Error: uncaught exception: [Exception... "Not enough arguments
> [nsIDOMWindowInternal.postMessage]"  nsresult: "0x80570001
> (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame ::
> /Users/warner/Library/Application
> Support/Firefox/Profiles/n9kdrtm6.b9/extensions/anonid0-annotator@jetpack/resources/anonid0-annotator-annotator-data/selector.js
> :: <TOP_LEVEL> :: line 37"  data: no]
> 
> which points at:
> 
>      window.addEventListener('unload', function() {
> -->      postMessage(['detach']);
>        },
>        false);
> 
> I don't understand that at all.. that call to postMessage() looks ok to me.

Nor do I. I'll see if I can make sense of it.

> What version of Firefox are you testing against? When I try it against FF4b8,
> I don't get the errors above, but when I click on the add-on-bar button (does
> it need a simple icon?)

Not sure I understand: there should be an icon (supposed to be a pencil) for the widget. Are you not seeing that, or do you mean something else?

Comment 7

[Will Bamberg [:wbamberg]]

(In reply to comment #6)
> when I build an XPI and try to install it Firefox
> tells me that my XPI is not compatible with b9. But I get that same error with
> the other examples...
> 

Sorry, scratch that: cfx xpi produces an installable XPI, for all the examples I tried.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Yeah, they both look like problems. Although I'm rather surprised it works at
> all given the second problem.

I would expect it to work within a browsing session, since the value require("simple-storage") returns is an object, so you can set and retrieve its properties within a browsing session just like you can on any object.  But only properties set on that object's storage property persist, and thus I expect the annotations not to persist across a restart.

Comment 9

[Brian Warner [:warner :bwarner]]

I saw an empty box, not a pencil icon. I'll try it again in a moment (on a different machine) and capture a screenshot if it's still a problem. Maybe there's something weird with my FF install...

Comment 10

[Brian Warner [:warner :bwarner]]

Attachment: broken icon in lower-right corner

Yeah, OS-X, FF4b9, clean profile, 'cfx xpi' then install the resulting annotator.xpi . I get the attached not-actually-an-icon in the corner, and clicking on certain parts of it provokes the "parent.getAttribute is not a function" error coming from WindowDraggingUtils.jsm . I wonder if the handful of pixels I see is actually something else, like a panel or something, mostly clipped to fit the tiny space.

If it's just my setup that's broken, I'm willing to accept this one, because it'd be nice to have this example in the release. Could a third person give this one a test?

Comment 11

[Drew Willcoxon :adw]

(In reply to comment #10)
> clicking on certain parts of it provokes the "parent.getAttribute is not a
> function" error coming from WindowDraggingUtils.jsm .

This is bug 615152 and unrelated to Will's add-on FWIW.

Comment 12

[Brian Warner [:warner :bwarner]]

more notes: sometimes it works (maybe those errors are harmless). Well, at least one of my tabs got into a state where I get the yellow highlight and clicking causes an annotation box to pop up. It seems that clicking on the not-actually-an-icon while tab 1 is up causes tab 2 to change states, so I suspect a problem with the way postMessage is being routed.

Comment 13

[Will Bamberg [:wbamberg]]

(In reply to comment #10)
> Created attachment 505114 [details]
> broken icon in lower-right corner
> 
> Yeah, OS-X, FF4b9, clean profile, 'cfx xpi' then install the resulting
> annotator.xpi . I get the attached not-actually-an-icon in the corner, and
> clicking on certain parts of it provokes the "parent.getAttribute is not a
> function" error coming from WindowDraggingUtils.jsm . I wonder if the handful
> of pixels I see is actually something else, like a panel or something, mostly
> clipped to fit the tiny space.
> 

That's the same handful of pixels I see if I rename the icons, so the add-on
can't find them. But even then I don't get these errors. 

> If it's just my setup that's broken, I'm willing to accept this one, because
> it'd be nice to have this example in the release. Could a third person give
> this one a test?

It would be really nice to have it, but I don't have much confidence in it
working reliably across multiple platforms if it doesn't even work reliably
across multiple computers running the same SW... I'd really like to understand
the cause of the problems you are seeing.

Comment 14

[Will Bamberg [:wbamberg]]

(In reply to comment #12)
> more notes: sometimes it works (maybe those errors are harmless). Well, at
> least one of my tabs got into a state where I get the yellow highlight and
> clicking causes an annotation box to pop up. It seems that clicking on the
> not-actually-an-icon while tab 1 is up causes tab 2 to change states, so I
> suspect a problem with the way postMessage is being routed.

Clicking on the not-an-icon (i.e. toggling the selector) is supposed to affect all tabs.

Comment 15

[Brian Warner [:warner :bwarner]]

ok, a couple of pilot-error problems fixed: I applied the patch with /usr/bin/patch, which isn't smart enough to handle the binary (encoded) .jpg files. I reapplied the patch with 'git apply', and now I get the pencil icon.

Second one: the icon is overlapped by the OS-X resize-window handle (the three diagonal lines in that same lower-right corner). There's a thin edge on the left and top sides of the icon that are reachable: the portion that's overlapped is not. Maybe our addon-bar shouldn't run all the way to the right margin, at least on OS-X?

Clicking on the now-yes-an-icon toggles the icon's background. I'm still not seeing the yellow highlight effect on the rest of the page, though. I'll keep fussing with it.

Comment 16

[Will Bamberg [:wbamberg]]

(In reply to comment #15)
> ok, a couple of pilot-error problems fixed: I applied the patch with
> /usr/bin/patch, which isn't smart enough to handle the binary (encoded) .jpg
> files. I reapplied the patch with 'git apply', and now I get the pencil icon.
> 

Great! I should have thought of that.

> Second one: the icon is overlapped by the OS-X resize-window handle (the three
> diagonal lines in that same lower-right corner). There's a thin edge on the
> left and top sides of the icon that are reachable: the portion that's
> overlapped is not. Maybe our addon-bar shouldn't run all the way to the right
> margin, at least on OS-X?
> 

Agreed. I think all the add-ons do this, it's annoying.

> Clicking on the now-yes-an-icon toggles the icon's background. I'm still not
> seeing the yellow highlight effect on the rest of the page, though. I'll keep
> fussing with it.

Not all page elements are annotatable: they have to be <p> elements which have an ancestor with an id attribute: this is to make it easier to find elements later and to make it more likely that annotations will attach to elements that are easy to highlight. Technically this is the trickiest part of the add-on, and I haven't really engaged with that, because it would add a lot of complexity that's not related to using the SDK APIs. 

So: many elements won't be highlighted, and some pages might not contain any highlightable elements at all. But, for instance, the paragraph "Easy ways you can help spread Firefox" at http://www.mozilla.com/en-US/about/ should be annotatable...

Comment 17

[Will Bamberg [:wbamberg]]

Attachment: Updated patch, again

Fixed storage, and made the code to identify annotation points more reliable.

Comment 18

[Drew Willcoxon :adw]

Will, you're a sharp guy, so if simple-storage confused you, that's a bad sign, and other people are definitely in the same boat.  If you have ideas on improving that API or its doc, please do, or let me know!  (Maybe more examples of how not to use it?)

Comment 19

[Myk Melez [:myk] [@mykmelez]]

I wonder if we could use a proxy to throw an exception when a property is set on the exports object (as opposed to its storage property).

Alternately, since functions can't be stored in simple storage, we could make the exports object itself be the storage while retaining the ability to define functions like "on" for registering an event listener.  But that would be a breaking change.

Comment 20

[Will Bamberg [:wbamberg]]

(In reply to comment #19)
> I wonder if we could use a proxy to throw an exception when a property is set
> on the exports object (as opposed to its storage property).

I think that would help a great deal: one problem is that you can't (as far as I know) test it using 'cfx run'. Of course, I should have tested the installation and proper functioning over restarts as well.

I think it is a bit unintuitive that you can't take a reference to the storage object and use that. So while it seems like a nice thing that you can just assign to a variable rather than having to call get/set or something, it's actually not such a nice thing because it doesn't fully support the contract that variable assignment implies, in my mind anyway. 

However it is very clearly documented and I did know that. What I missed initially was that 'simplestorage.*storage*.array part, then just blindly copied the change in comment 5... 

By the way... is it my add-on's responsibility to clean up simple-storage when it is uninstalled? So it should listen for onUnload('uninstall') and zero the storage? Because it looks to me that if I install, create some annotations, then uninstall and reinstall, the old annotations are still there.

Comment 21

[Drew Willcoxon :adw]

(In reply to comment #19)
> I wonder if we could use a proxy to throw an exception when a property is set
> on the exports object (as opposed to its storage property).

That sounds like a good idea.  I wonder too if the ability to set the `storage` object itself, rather than only properties on it, confuses things.  If so, the doc could just not mention it.

(In reply to comment #20)
> By the way... is it my add-on's responsibility to clean up simple-storage when
> it is uninstalled?

Sigh, simple-storage is supposed to remove the store when the add-on is uninstalled, but I see it's not.  It was working when I wrote it, so let me find out what's wrong.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #20)
> one problem is that you can't (as far as
> I know) test it using 'cfx run'. Of course, I should have tested the
> installation and proper functioning over restarts as well.

Yup, agreed.  Atul started working on a solution to that ("cfx develop" and then "cfx run --use-server"), for testing an addon including install/change/update and install/uninstall/reinstall within a single browser session), but he didn't get very far with it, and it's currently in the "experimental" bucket (and might not even work anymore).


> I think it is a bit unintuitive that you can't take a reference to the storage
> object and use that.

Hmm, not 100% sure what you're thinking, but it should be possible to do:

  let storage = require("simple-storage").storage;
  storage.whatever = ...;


(In reply to comment #21)
> I wonder too if the ability to set the `storage`
> object itself, rather than only properties on it, confuses things.  If so, 
> the doc could just not mention it.

That sounds like a good idea.

Comment 23

[Drew Willcoxon :adw]

(In reply to comment #22)
> Hmm, not 100% sure what you're thinking, but it should be possible to do:
> 
>   let storage = require("simple-storage").storage;
>   storage.whatever = ...;

Yes, this should work.

I filed bug 627432 for the uninstall issue (it's a Firefox problem) and bug 627470 for the simple-storage improvements suggested here.

Comment 24

[Brian Warner [:warner :bwarner]]

Comment on attachment 505434 [details] [diff] [review]
Updated patch, again

Yay! This one works for me! That's a cool add-on!

The yellow outlines interact weirdly with hrefs: when I turn off annotation
mode and mouseover a <p> with a link near the border, sometimes it blinks
back and forth between the note-bearing widget and the follow-a-link cursor.
Within the yellow box, each time the mouse transitions between the link and
the surrounding text, the widget disappears and then reappears. Weird but not
a big deal.

The limit on only updating <p> elements was a bit hard to discover: the first
couple of pages that I tried it against had no real <p>s and nothing
happened. It might be a good idea to pop up a panel when the feature is first
activated with a brief description, and to offer a link that causes a tab to
open up on some mozilla.com page that's known to have the right elements to
play with. Or, just put a hint in the package.json "description" field, and
hope that experimentors will read that before wondering why it doesn't appear
to work.

Looks great!

Comment 25

[Brian Warner [:warner :bwarner]]

Oh, RE being able to do require("simple-storage").storeme=42 , the recent patch for bug 577782 (which landed before the freeze) allows modules to export arbitrary things, not merely add properties to a pre-existing object. So our loader now makes it possible for simple-storage.js to export a proxy that could intercept the handler.set() call and stash the resulting data. Whether or not it's a good idea to change the simple-storage API to use this new ability is a separate question...

Comment 26

[Will Bamberg [:wbamberg]]

(In reply to comment #24)
 
> Yay!

I'm glad it's working now! I just pushed some changes to /annotator, and that's what I will land when the tree thaws. 

> The yellow outlines interact weirdly with hrefs: when I turn off annotation
> mode and mouseover a <p> with a link near the border, sometimes it blinks
> back and forth between the note-bearing widget and the follow-a-link cursor.
> Within the yellow box, each time the mouse transitions between the link and
> the surrounding text, the widget disappears and then reappears. Weird but not
> a big deal.

Yeah, that's quite nasty. Should be fixed now. There is flickering in some conditions but it doesn't seem to be so common now.
 
> The limit on only updating <p> elements was a bit hard to discover: the first
> couple of pages that I tried it against had no real <p>s and nothing
> happened. It might be a good idea to pop up a panel when the feature is first
> activated with a brief description, and to offer a link that causes a tab to
> open up on some mozilla.com page that's known to have the right elements to
> play with. Or, just put a hint in the package.json "description" field, and
> hope that experimentors will read that before wondering why it doesn't appear
> to work.

I've thrown that limitation away now, so you can annotate any element. The reason for the limitation was that it seemed like quite often, with some elements, it had trouble finding the annotation again (or at least decorating it), which is annoying. But it seems to do somewhat better at that now.

I also updated it to listen to OverQuota events.

Comment 27

[Will Bamberg [:wbamberg]]

https://github.com/mozilla/addon-sdk/commit/d72f29bcf72baada9f9f57f05cc90ca272edc8af

Comment 28

[Nickolay_Ponomarev]

https://github.com/mozilla/addon-sdk/pull/104

This new example has no tests, which makes cfx testex and cfx testall fail when attempting to test it. The pull requests adds an empty test to make cfx happy.