JEP 102 - Single UI Mechanism for exposing add-ons to users

RESOLVED FIXED in 0.4

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dbuc, Assigned: dietrich)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 17 obsolete attachments)

5.37 KB, image/png
Details
37.79 KB, patch
myk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Create a single UI mechanism that jetpacks can register an element for users to interact with.  This will likely take the place of the statusbar and other random areas where UI is typically stuck in the browser.
(Reporter)

Updated

8 years ago
Priority: P1 → P2
(Reporter)

Updated

8 years ago
Duplicate of this bug: 537197
(Reporter)

Updated

8 years ago
Duplicate of this bug: 535482
(Reporter)

Updated

8 years ago
Duplicate of this bug: 499809
Atul: should this actually be assigned to you, given that you're mostly focused on the core platform stuff, while this is one of the high-level APIs?
Severity: blocker → normal
Priority: P2 → P1
Target Milestone: -- → 0.2

Comment 5

8 years ago
Yep, this should probably be mine.

Updated

8 years ago
Assignee: avarma → aza
Aza's the champion for the proposal, but this bug is about the implementation, so it should ideally be assigned to the person implementing the proposal.  I've recently agreed to take a crack at that, so reassigning to myself for implementation.
Assignee: aza → myk
Status: NEW → ASSIGNED
Target Milestone: 0.2 → 0.3
This isn't going to make 0.3, as it is on hold pending more UX work from boriss, and Marco/Dietrich are going to take over implementation.  Moving to 0.4, which is when Dietrich thinks this is going to happen, and reassigning to Dietrich.
Assignee: myk → dietrich
Status: ASSIGNED → NEW
Target Milestone: 0.3 → 0.4
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Version: 0.1 → Trunk
(Assignee)

Updated

8 years ago
Summary: Single UI Mechanism for exposing jetpacks to users → JEP 102 - Single UI Mechanism for exposing add-ons to users
Can i suggest taking a look at the Windows 7 notification area?
The problems are similar:
- apps can register to it and provide options and notifications
- if too many apps register, it becomes an hell both to see and manage

The solution Windows 7 implemented is interesting, apps register to it, but they are hidden unless they have to notify something to the user. But the user can also choice to make an app always visible opening the palette and saying "Always show" (it could actually be better, with a context menu item and drag and drop between the bar and the palette). The choice is up to the user, not to the app developer (correctly). This way the bar would take a small space, and in future could eventually be movable in place of the search bar (if it would ever be merged to the locationbar).
Marco: that's useful information, but I just noticed that boriss, who is designing the UX for this API, isn't cc:ed to this bug.  cc:ing her so she too can read your comment.
(Assignee)

Comment 10

8 years ago
Created attachment 441931 [details] [diff] [review]
wip

WIP patch that implements some of the core infrastructure for this. no work on UI really. this allows us to build basic widgets, and get a feel for where we want the API to go. the api implemented so far is almost directly matching the pseudocode in the JEP.
(Assignee)

Comment 11

8 years ago
Created attachment 441932 [details]
screenshot

Comment 12

8 years ago
Sweet.

One thing I like about Apple's approach to these kinds of UIs is that they offer platform-level services that reduce the amount of effort developers need to make art that matches the UI's look-and-feel. For instance, all the notifications on a Mac's menubar are black-and-white, and my understanding of iPhone apps is that developers only need to submit a bitmask for their buttons, which the platform then "snazzifies" to look shiny and blue when selected. Would be way cool to see something like this for the Jetpack icons--though we should still certainly provide the means for developers to be able to have full visual control over their app's appearance if they want it.
(Assignee)

Comment 13

8 years ago
Created attachment 441986 [details] [diff] [review]
wip2

some minor changes, adds a couple of examples to the docs.
Attachment #441931 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
(In reply to comment #12)

Absolutely! We should probably keep a list of UX thoughts on the JEP, for when Boriss starts the design work for this next month.
(Assignee)

Updated

8 years ago
Depends on: 494238
(Assignee)

Comment 15

8 years ago
Created attachment 443011 [details] [diff] [review]
wip3

- add accel-shift-U to toggle UI (hidden by default)
- change default widget size to 24px
Attachment #441986 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
Created attachment 443015 [details] [diff] [review]
wip4

fix ze leak.
Attachment #443011 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
Created attachment 443232 [details] [diff] [review]
wip5

try/catch/log all caller-passed properties.
Attachment #443015 - Attachment is obsolete: true
(Assignee)

Comment 18

8 years ago
Created attachment 443560 [details] [diff] [review]
wip6

more cleanup. asking for initial feedback pass.

i'm punting on content-frame for now, since that JEP needs a lot of work. we can swap out iframe for content-frame later.
Attachment #443232 - Attachment is obsolete: true
Attachment #443560 - Flags: feedback?(adw)
(Assignee)

Comment 19

8 years ago
note: since this will be landing in multiple phases, i'm tracking TODOs and open issues in the docs.

Comment 20

8 years ago
Comment on attachment 443560 [details] [diff] [review]
wip6

Sorry for the delay.

>diff --git a/packages/jetpack-core/lib/widget.js b/packages/jetpack-core/lib/widget.js

>+ * Contributor(s):
>+ *   Drew Willcoxon <adw@mozilla.com> (Original Author)

Huh? :)

>+function Widget(options) {
>+  options = validateOptions(options, {

You can specify types by using the `is` property now.  Might be easier here.  It even gives you nice error messages by default.  (bug 563555)

>+    onClick: {
>+      ok: function (v) !v || typeof(v) === "function",
>+      msg: "The item's onClick must be a function."
>+    },
>+    onMouseover: {
>+      ok: function (v) !v || typeof(v) === "function",
>+      msg: "The item's onMouseover must be a function."
>+    },
>+    onMouseout: {
>+      ok: function (v) !v || typeof(v) === "function",
>+      msg: "The item's onMouseout must be a function."
>+    },
>+    onLoad: {
>+      ok: function (v) !v || typeof(v) === "function",
>+      msg: "The item's onLoad must be a function."
>+    }

Usually event listeners like this are Collection properties.

>+  // TODO: wtf, there's probably a better way to write this.
>+  ["label", "content", "onClick", "onLoad", "onMouseover", "onMouseout"].forEach(function(property) {
>+    this.__defineGetter__(property, require("errors").catchAndLog(function Widget_propertyGetter() {
>+      return options[property];
>+    }));
>+  }, this);

Hmm, looking below at _fillItemBox I see why you did this, but I kind of don't like it, because if I'm a client, the functions returned from these getters are not the functions I passed in.  And there's no reason to try-catch-log label and content.  But most importantly, I think (not 100% sure) |this| inside the callbacks will not be the Widget object, even though you pass |this| to forEach, because catchAndLog doesn't set it properly.  More generally you're (subtly) modifying the interface because of implementation needs.  So I think the try-catch-logs should be moved to the call sites.

>+  this.destroy = function Widget_destroy() {
>+    browserManager.removeItem(this);

Remember not to use |this| inside the methods of public objects.

>+/*
>+TODO: Much of the code below is from the context-menu module. Need to
>+figure out // how much of this can be pushed out, instead of each per-window module
>+duplicating it.
>+
>+Two problems:
>+
>+1. Duplicating browserManager and BrowserWindow across a lot of modules.

browserManager is just a specialized WindowTracker delegate.  It's specialized because 1) it keeps track of a list of items that it adds to and removes from new and closed windows, and 2) it tracks browser windows.  So if it does get factored out, it should be split into these two pieces.  For part 2, Atul's approach in bug 560716 should work...

BrowserWindow isn't general enough to be factored out, because what it does depends on your specific use case.  It's not a general "browser window" class.  browserManager should be modified so that you can plug in a class or function for onTrack to call.

>+2. Branching codepaths for different host applications. Only a few small bits
>+   of BrowserWindow have been modified for use here. Eg, it could be Window
>+   instead, and extend it for each supported app. Shouldn't require core
>+   support though - should use a registration system, so that XULRunner apps
>+   can easily plug-in without modifying the Jetpack core.

I think this is what bug 560716 and the 0.5 goal of app-specific implementations are about?

>+  _addKeyCommands: function BW__addKeyCommands() {
>+    let key = this.doc.getElementById("JetpackWidgetKey");

We should standardize the format of IDs and class names.  The context menu module uses the form "jetpack-context-menu-", so I'd recommend the form "jetpack-PACKAGE_NAME-foo".

>+      cmd.setAttribute("oncommand", "window.toggleJetpackWidgets();");

Do you plan on adding a menu item or some other UI element for toggling?  A key only seems a bit too advanced-userish.

>+  _addItemToWindow: function BW__addItemToWindow(widget) {

>+    widget.node = node;

This exposes node to clients, which shouldn't happen.  context-menu got around this by keeping a list of "item wrappers" in BrowserWindow's items array.  I see that's what you've done too, but you still expose node.

>+  _getItemContentType: function BW__getItemContentType(content) {
>+    try {
>+      require("url").parse(content);
>+      return CONTENT_TYPE_URI;
>+    } catch(e) {}
>+    return CONTENT_TYPE_HTML;
>+  },

I think Myk has decided that we should require clients to use a URL constructor to create URL objects, rather than trying this.  Part of content-frame (bug 564059 comment 3)?  But even if we punt on that, we should do the URL object part so the API doesn't change.

>+  _fillItemBox: function BS__fillItemBox(widget, node) {

>+    // Event handlers
>+    // Note: load is handled differently depending on content, is set later.
>+    if (widget.onClick) {
>+      iframe.addEventListener("click",
>+                              function(e) widget.onClick(e),
>+                              false);

If all these event listeners are Collection properties as I mentioned earlier, I guess you would need a way to listen for when items are added to and removed from the Collection so you can add and remove them from the iframe...

Also, do these listeners need to be removed when the widget is removed, or does the iframe take care of that when it's removed?

>+    // Get widget's content type
>+    let type = this._getItemContentType(widget.content);
>+    switch(type) {

This big switch should be broken into two helper methods.

>+      case CONTENT_TYPE_URI:
>+        iframe.setAttribute("src", widget.content);
>+        iframe.contentWindow.addEventListener("load", function() {
>+          if (iframe.contentDocument.body.firstElementChild.tagName == "IMG") {

Hmm, can we just tell people to give us an image instead of special-casing an HTML file with an image as its first element?

>+      case CONTENT_TYPE_XML:
>+        throw("XML is not yet supported as a widget content type.");
>+        break;
>+      case CONTENT_TYPE_WINDOW:
>+        throw("Window is not yet supported as a widget content type.");
>+        break;
>+      default:
>+        throw("Widget's content is not a supported type.");

You should always throw new Error()s.  Atul updated the JEP guide to explain why:

https://wiki.mozilla.org/Labs/Jetpack/So_You're_Implementing_a_JEP

Other thoughts:

It would be a little bit better to set up the widget container and command only if there's a widget, and remove them when there are no widgets.  Most extensions will require() the module, add a widget, and leave it there.  But I'm thinking of cases where people require() it and for whatever reason botch the add(), or add an item and later remove (destroy()) it.  In the latter case, the container should go away, and if someone later adds a widget, it should come back.  (FWIW, that's how the context-menu module works, with its several ensure*() functions.)

If I install X extensions that use this, their widgets should always appear in the same order across browser restarts.  Will they?
Attachment #443560 - Flags: feedback?(adw) → feedback+
(Assignee)

Updated

8 years ago
Depends on: 564524
(Assignee)

Comment 21

8 years ago
> >+ * Contributor(s):
> >+ *   Drew Willcoxon <adw@mozilla.com> (Original Author)
> 
> Huh? :)

i copied your context menu module wholesale, and started from there.

> You can specify types by using the `is` property now.  Might be easier here. 
> It even gives you nice error messages by default.  (bug 563555)

awesome, fixed.

> Usually event listeners like this are Collection properties.

i'm leaving it as-is for now. will file a bug to improve Collection in a way that supports what we need to do here.

> >+  this.destroy = function Widget_destroy() {
> >+    browserManager.removeItem(this);
> 
> Remember not to use |this| inside the methods of public objects.

fixed

> We should standardize the format of IDs and class names.  The context menu
> module uses the form "jetpack-context-menu-", so I'd recommend the form
> "jetpack-PACKAGE_NAME-foo".

fixed

> Do you plan on adding a menu item or some other UI element for toggling?  A key
> only seems a bit too advanced-userish.

this is intentional. this is phase 1, for jetpack devs to experiment with the widget definition and the ergonomics. we don't want people trying to ship this yet.

once we get real UI, that UI will likely get put in the core, where widgets can hook into it.

> >+    widget.node = node;
> 
> This exposes node to clients, which shouldn't happen.  context-menu got around
> this by keeping a list of "item wrappers" in BrowserWindow's items array.  I
> see that's what you've done too, but you still expose node.

was vestigial, fixed.

> I think Myk has decided that we should require clients to use a URL constructor
> to create URL objects, rather than trying this.  Part of content-frame (bug
> 564059 comment 3)?  But even if we punt on that, we should do the URL object
> part so the API doesn't change.

filed and patched bug 564524.

> If all these event listeners are Collection properties as I mentioned earlier,
> I guess you would need a way to listen for when items are added to and removed
> from the Collection so you can add and remove them from the iframe...

see earlier comment.

> Also, do these listeners need to be removed when the widget is removed, or does
> the iframe take care of that when it's removed?

iirc, event listeners to don't have references to the dom are able to be cleaned up when the dom is destroyed. if they do, it's leak city if the listener is not removed.

will figure out something before final review.

> Hmm, can we just tell people to give us an image instead of special-casing an
> HTML file with an image as its first element?

yeah, that'd be much better. maybe a require("url").IMG("") or an image module?

> You should always throw new Error()s.  Atul updated the JEP guide to explain

fixed

> the container should go away, and if someone later adds a widget, it should
> come back.  (FWIW, that's how the context-menu module works, with its several
> ensure*() functions.)

I don't think this disappearing UI approach is desirable. Once UX does the design work (see boriss and daniel's posts) we'll probably put this in the core. And the Jetpack API will just be about the widgets themselves.

> If I install X extensions that use this, their widgets should always appear in
> the same order across browser restarts.  Will they?

Not necessarily. See the JEP and the module docs for a host of things like this which are undetermined at this point.
(Assignee)

Comment 22

8 years ago
Created attachment 444480 [details] [diff] [review]
wip7

most of the fixes from drew's review
Attachment #443560 - Attachment is obsolete: true
(Assignee)

Comment 23

8 years ago
Created attachment 445150 [details] [diff] [review]
wip8

- converted to XUL iframes, properly marking content as untrusted, and handling events safely
- more catchAndLog()s
Attachment #444480 - Attachment is obsolete: true
(Assignee)

Comment 24

8 years ago
Created attachment 445407 [details] [diff] [review]
wip9

- added content setter and accompanying tests (enabling badging, etc)
- test multiple widgets and variable destroy sequence
Attachment #445150 - Attachment is obsolete: true
(Assignee)

Comment 25

8 years ago
Created attachment 445566 [details] [diff] [review]
wip10

- fixed multi-window updating
- storing ids instead of nodes now
Attachment #445407 - Attachment is obsolete: true
(Assignee)

Comment 26

8 years ago
Comment on attachment 445566 [details] [diff] [review]
wip10

i want to write a test for the keyboard shortcut and ui persistence pref. otherwise this is about where i'm comfortable shipping for 0.4, in terms of satisfying basic use-cases. i'm going to update the docs again, so only more code review needed at this point.
Attachment #445566 - Flags: review?(myk)
Attachment #445566 - Flags: feedback?(adw)
Comment on attachment 445566 [details] [diff] [review]
wip10

>diff --git a/packages/jetpack-core/docs/widget.md b/packages/jetpack-core/docs/widget.md

>+The Panels JEP has not yet been implemented. Currently that property is
>+ignored. You may hook up extended UI via the supported events, however
>+keep in mind that direct access to the browser's XUL window DOM may
>+break in the future.

Let's plan to break this in 0.5!  In particular, that is going to mean changes
to the way we expose event objects.  My current thinking is that we only expose
event objects for events that take place in the content frame, not for those
that originate in the chrome browser window.

In any case, we can figure this out in another bug...


>+widgets.add(widgets.Widget({
>+  label: "Widget with some text",
>+  // Note the CSS hurdles just to make text nicely centered. Ugh.
>+  content: "<div style='text-align: center; display: table-cell; vertical-align: middle; width: 24px; height: 24px; font-size: small; font-weight: bold; padding: 0px;'>HI.</div>"
>+}));

Nit: it'd be nice to wrap this to make it more readable, i.e.:

  widgets.add(widgets.Widget({
    label: "Widget with some text",
    // Note the CSS hurdles just to make text nicely centered. Ugh.
    content: "<div style='text-align: center; display: table-cell; " +
             "vertical-align: middle; width: 24px; height: 24px; " +
             "font-size: small; font-weight: bold; padding: 0px;'>HI.</div>"
  }));


>diff --git a/packages/jetpack-core/lib/widget.js b/packages/jetpack-core/lib/widget.js

>+  throw new Error([
>+    "The widget module currently supports only Firefox.  In the future ",
>+    "it will support other applications, possibly even your mom. Please see ",
>+    "https://bugzilla.mozilla.org/show_bug.cgi?id=560716 for more information."
>+  ].join(""));
>+}

You talkin' 'bout my mother?


>+  /**
>+   * Removes the widget from the UI.
>+   * @method
>+   */
>+  this.destroy = function Widget_destroy() {
>+    browserManager.removeItem(self);
>+  };

Rather than providing both [singleton].remove and [instance].destroy, both of which do the same thing, let's just provide [singleton].remove, which is consistent with what we're doing in other APIs.


>+      var bottombox = this.doc.getElementById("browser-bottombox");
>+      bottombox.insertBefore(container, bottombox.firstChild);

Instead of inserting this before the first child, which will make it appear above the find bar when the find bar is visible <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#755>, it would be better to insert it before the status bar (i.e. between the first and second children).


>+  _toggleUI: function BW__toggleUI() {
>+    this.container.hidden = !this.container.hidden;
>+    require("preferences-service").set("jetpack.jetpack-core.widget.barIsHidden", this.container.hidden);
>+  },

Shouldn't the code also observe changes to the preference, so hiding the bar hides it in all windows (rather than just the current one)?


>+    let node = this.doc.createElement("hbox");
>+    node.id = Math.floor(Math.random()*1001);
...
>+    this._items.push({widget: widget, nodeId: node.id});

It doesn't seem that hard for node IDs to collide when being chosen randomly from 1000 options.  It'd be better to use something like a GUID.  Alternately, if you cache a reference to the item rather than its ID, you don't need node IDs at all, i.e.:

  let node = this.doc.createElement("hbox");
...
  this._items.push({widget: widget, node: node});


>+      iframe.addEventListener("click", wrappedHandler, false);
...
>+      iframe.addEventListener("mouseover", wrappedHandler, false);
...
>+      iframe.addEventListener("mouseout", wrappedHandler, false);

Shouldn't these event listeners be removed when the widget is removed?


>+        if (doc.body.firstElementChild &&
>+            doc.body.firstElementChild.tagName == "IMG") {

This should also check that there are no other children of the body, since otherwise it'll mistake HTML pages whose first element happens to be an image for the images it is trying to detect.


>+    function configureIframeForHTML(iframe, widget) {
>+      // Allow content to expand to the edge of the box by default.
>+      iframe.addEventListener("DOMContentLoaded", function(e) {
>+        e.target.body.style.margin = "0";
>+      }, false);
>+
>+      if (widget.onLoad) {
>+        let wrappedHandler = require("errors").catchAndLog(function(e) widget.onLoad(e));
>+        iframe.addEventListener("DOMContentLoaded", wrappedHandler, false);
>+      }
>+    }

Shouldn't these event listeners be removed when the widget is removed?

Also, in the future, when the callback properties are collections, it'll be possible for an onLoad callback to be added between the time the iframe is configured and the time the load actually happens.  This code currently doesn't take that into account, which is ok, but it'd be useful to add a comment here noting the change that will be required.

Alternatively, you could make this code forward-compatible by just having a single DOMContentLoaded listener that always gets registered and wrapping/calling the onLoad callback, if any, there.

Finally, there's some inconsistency here between when we call the onLoad callback, since for URLs we call it on "load", while for HTML we call it on DOMContentLoaded.

We should be as consistent as we can here, and it seems like DOMContentLoaded is the more useful event, as it gives addons earlier access to the DOMs of their widgets.  However, if we standardize on DOMContenttLoaded, we should call the callback property onReady rather than onLoad, to distinguish it from a callback property handling the actual "load" event, and for consistency with other APIs that use "onReady" as the name of the callback property for the DOMContentLoaded event.

If DOMContentLoaded doesn't work for images, then I'm ok with also providing an onLoad callback property that handles "load" events (and documenting the need to use onLoad rather than onReady if you are loading an image into your widget and need to manipulate it after it is loaded).  I just don't want to conflate the two.


>+  // Removes an array of items from the window.
>+  removeItems: function BW_removeItems(items) {
>+    this._items.forEach(function(item, index) {
>+      for (var i in items) {
>+        if (items[i] == item.widget) {
>+          this.container.removeChild(this.doc.getElementById(item.nodeId));
>+          delete this._items[index];
>+          break;
>+        }
>+      }
>+    }, this);
>+  },

Deleting the value of an index of an array via |delete| leaves the index around with an undefined value, creating a "hole" in the array.  |for each.. in| and Array.forEach skip those holes, so this code works, but it's a bit dangerous, since Array.length counts the hole, and |for (let i = 0; i < ary.length; i++)| includes it, so if anyone ever uses those in new code, they'll get incorrect data.

Thus I think it would be better to splice the item out of the array, although that requires reversing the nesting of the iterations to avoid continuing to iterate an array that has been mutated, i.e.:

  for each (let widget in items) {
    for each (let i in this._items) {
      if (this._items[i].widget == widget) {
        this.container.removeChild(this.doc.getElementById(this._items[i].nodeId));
        this._items.splice(i, 1);
        break;
      }
    }
  }

Alternately, you could do something crazy, like:

  this._items = this._items.filter(function(v) items.indexOf(v.widget) != -1 ?
      this.container.removeChild(this.doc.getElementById(v.nodeId)) && false :
      true, this);
Attachment #445566 - Flags: review?(myk) → review-

Comment 28

8 years ago
Comment on attachment 445566 [details] [diff] [review]
wip10

Trying not to cover the same ground that Myk did...

>diff --git a/packages/jetpack-core/docs/widget.md b/packages/jetpack-core/docs/widget.md

>+The 'widget' module provides a consistent, unified way for extensions to

'widget' -> `widget` (backticks)

>+## Extended UI
>+
>+The Panels JEP has not yet been implemented. Currently that property is
>+ignored. You may hook up extended UI via the supported events, however
>+keep in mind that direct access to the browser's XUL window DOM may
>+break in the future.

>+## TODO

>+    <td><tt>content</tt></td>
>+    <td>
>+     @prop content {uri|html|xml|window}
>+           This is the small view of the feature.
>+           uri: A link to the URL of either an icon, or a webpage.
>+           html: A html string that will be shown in the small view.
>+           xml: E4X version of the above. NOT SUPPORTED.
>+           window: a reference to a DOM window object. NOT SUPPORTED.

>+    <td><tt>panel</tt></td>
>+    <td>
>+           NOT SUPPORTED.

I know you said the doc would be changing, but in case you weren't planning to change these things:  Let's keep the docs concise and relevant at all times, most especially for releases.  If it's not supported or not done yet, let's not mention it.  We have other channels that are better suited for discussion of TODOs.

>+     @prop content {uri|html|xml|window}

Don't you have to wrap the javadoc syntax in <api> tags for it to be parsed?

>+widgets.add(widgets.Widget({
>+  label: "Widget with some text",
>+  // Note the CSS hurdles just to make text nicely centered. Ugh.
>+  content: "<div style='text-align: center; display: table-cell; vertical-align: middle; width: 24px; height: 24px; font-size: small; font-weight: bold; padding: 0px;'>HI.</div>"

OMG.  Are we really shipping this while it requires these kinds of things to make your widget look nice?  "display: table-cell"?  That's not acceptable.  It's random hacks and junk like this that we're telling people Jetpack will smooth over.  I'm all for iteration, but in public releases let's iterate starting from a point of sanity.

>diff --git a/packages/jetpack-core/lib/widget.js b/packages/jetpack-core/lib/widget.js

>+ * Contributor(s):
>+ *   Drew Willcoxon <adw@mozilla.com> (Original Author)
>+ *   Dietrich Ayala <dietrich@mozilla.com>

List me as a second contributor if you want, but you're the original author! :)

>+function Widget(options) {
>+  options = apiutils.validateOptions(options, {
>+    label: {
>+      is: ["string"],
>+      ok: function (v) v.length > 0,
>+      msg: "The item must have a non-empty label property."

s/item/widget/, here and elsewhere.

>+    content: {
>+      // could be url obj or string
>+      map: function (v) v ? v.toString() : v,
>+      is: ["string", "object"],
>+      ok: function (v) v.length > 0,
>+      msg: "The item must have a non-empty content property."

map is applied before |is| is checked.  So this says content can be an object, but before that you're stringifying it.  I looked at the javadoc for this func, but it conflicts with the Markdown doc, so I'm not sure what types this does accept.  If content really can be an object, you probably shouldn't stringify it first.  And msg, if you define it, should mention what kinds of types are expected.

>+    onClick: {
>+      is: ["function", "array", "null", "undefined"],
>+      msg: "The item's onClick must be a function, or array of functions."

When you define |is|, you don't need to define msg, since by default it's "The option 'onClick' must be one of the following types: <types>".  (If you also define ok, though, you should define msg so that it's as specific as possible.)

>+BrowserWindow.prototype = {
>+
>+  _init: function BW__init() {

>+    // Hook up a window-scope function for toggling the UI
>+    // TODO: unhack this.

???

>+  updateItem: function BW_updateItem(updatedItem, content) {
>+    let item = this._items.filter(function(item) item.widget == updatedItem).shift();
>+    if (item) {
>+      let widgetNode = this.doc.getElementById(item.nodeId);
>+      if (widgetNode)
>+        this.setIframeContent(widgetNode.firstElementChild, content);
>+    }
>+  },
>+  

Don't you need to call addIframeEventHandlers again at some point here, like you do in _fillItemBox?  I can change the content type when I update it, right?  Also, despite its name, addIframeEventHandlers actually does more than that -- setting style and the image hack.  Which is evidence that it should be broken up into smaller, more focused functions.

>+  _addItemToWindow: function BW__addItemToWindow(widget) {
>+    // XUL element container for widget
>+    let node = this.doc.createElement("hbox");
>+    node.id = Math.floor(Math.random()*1001);

Echoing what Myk said here, this is brittle.  Why not keep a reference to the node like you were before?  (The comment in _init for _items is out of date with this change.)

>+    function configureIframeForURI(iframe, widget) {
>+      // TODO: special-casing of images will be replaced, probably by an
>+      // image-specific extension of the URI object.
>+      iframe.addEventListener("load", function(e) {
>+        // ignore about:blank load event
>+        var ios = Cc['@mozilla.org/network/io-service;1'].getService(Ci.nsIIOService);
>+        let targetURI = ios.newURI(e.target.location, null, null);
>+        let widgetURI = ios.newURI(widget.content, null, null);
>+        if (!targetURI.equals(widgetURI))
>+          return;
>+
>+        let doc = e.target;
>+        if (doc.body.firstElementChild &&
>+            doc.body.firstElementChild.tagName == "IMG") {

Is it really difficult to properly support images from the get-go?  If not, I'd rather leave out hacks like this altogether; TODOs don't absolve them.

We have bundled resources now (bug 557663).  I should be able to stick an icon bundled with my extension into my widget.  That's the common case, and as such we shouldn't release this API until that very basic use case is possible.
Attachment #445566 - Flags: feedback?(adw) → feedback-
(Assignee)

Comment 29

8 years ago
(In reply to comment #27)

most comments addressed, and noted below otherwise.

> Alternatively, you could make this code forward-compatible by just having a
> single DOMContentLoaded listener that always gets registered and
> wrapping/calling the onLoad callback, if any, there.

this needs to happen anyway in order to reasonably remove all the event handlers, so going with this approach.

> Finally, there's some inconsistency here between when we call the onLoad
...
> and need to manipulate it after it is loaded).  I just don't want to conflate
> the two.

Yeah iirc loading images directly didn't send DOMContentReady.. but i'm not totally sure. If that's the case i'll keep onLoad, with docs making clear which to use when.

Comment 30

8 years ago
(In reply to comment #28)
> >+    <td><tt>content</tt></td>
> >+    <td>
> >+     @prop content {uri|html|xml|window}
> >+           This is the small view of the feature.
> >+           uri: A link to the URL of either an icon, or a webpage.
> >+           html: A html string that will be shown in the small view.
> >+           xml: E4X version of the above. NOT SUPPORTED.
> >+           window: a reference to a DOM window object. NOT SUPPORTED.
> 
> >+    <td><tt>panel</tt></td>
> >+    <td>
> >+           NOT SUPPORTED.
> 
> I know you said the doc would be changing, but in case you weren't planning to
> change these things:  Let's keep the docs concise and relevant at all times,
> most especially for releases.  If it's not supported or not done yet, let's not
> mention it.  We have other channels that are better suited for discussion of
> TODOs.

I generally agree with this sentiment, but at the same time, I want to make sure that developers aren't confused by the functionality of an early release of an API. For instance, my intuition of a "tabs" API tells me that I should trivially be able to access the DOM of the document that's in the tab through it--if I don't see some API to access it from there, I'll scratch my head and look elsewhere for other APIs that might expose it. That time spent being confused and looking elsewhere could be obviated if there were a blurb on the page explaining why a very obvious feature that *should* be there--and eventually will--isn't present.

Obviously this is a slippery slope, though.

Comment 31

8 years ago
Whoa, I totally thought this was the bug for the "tabs" API. My bad... too early in the morning!
(Assignee)

Comment 32

8 years ago
> OMG.  Are we really shipping this while it requires these kinds of things to
> make your widget look nice?  "display: table-cell"?  That's not acceptable. 
> It's random hacks and junk like this that we're telling people Jetpack will
> smooth over.  I'm all for iteration, but in public releases let's iterate
> starting from a point of sanity.

Save your outrage for the CSS working group. I'm a-ok with not centering this text properly in order to keep the example simple. (side bet: try to achieve the same thing in fewer characters!)

See Atul's thoughts about this in comment #12. We'll probably provide some ways for making these look consistent and nice...  where we'll likely be doing these types of hacks behind the scenes, giving you another opportunity to be outraged ;)

Part of the feature is that people can stick web content in these widgets. Stock tickers, SVG clocks, snowglobes. Expect real-life widgets to be far scarier than this piddly example.

> >+    // Hook up a window-scope function for toggling the UI
> >+    // TODO: unhack this.
> 
> ???

There might be a better way to get window-global commands to call back out into our module context. Or there might not, in which case this isn't a hack.

> We have bundled resources now (bug 557663).  I should be able to stick an icon
> bundled with my extension into my widget.  That's the common case, and as such
> we shouldn't release this API until that very basic use case is possible.

Yes! I'll add this.

Comment 33

8 years ago
I took a moment to calm my raging fires, and it turns out this still sucks.

(In reply to comment #32)
> Save your outrage for the CSS working group. I'm a-ok with not centering this
> text properly in order to keep the example simple. (side bet: try to achieve
> the same thing in fewer characters!)
> 
> See Atul's thoughts about this in comment #12. We'll probably provide some ways
> for making these look consistent and nice...  where we'll likely be doing these
> types of hacks behind the scenes, giving you another opportunity to be outraged
> ;)

Behind-the-scenes hacks suck too, but in-front-of-the-scenes hacks fucking suck.  Too bad.

> Part of the feature is that people can stick web content in these widgets.
> Stock tickers, SVG clocks, snowglobes. Expect real-life widgets to be far
> scarier than this piddly example.

I don't know why we're making these little 24x24 or whatever squares web pages.  Making people jump through hoops to make the word "HI" look nice sounds like a great idea.  If I'm a developer coming to Jetpack, just ask me for my icon, let me hook up some events to it, give me some APIs for doing things like badges and notifications, and make it look nice.

Comment 34

8 years ago
(In reply to comment #33)
> I took a moment to calm my raging fires, and it turns out this still sucks.
> 
> (In reply to comment #32)
> > Save your outrage for the CSS working group. I'm a-ok with not centering this
> > text properly in order to keep the example simple. (side bet: try to achieve
> > the same thing in fewer characters!)
> > 
> > See Atul's thoughts about this in comment #12. We'll probably provide some ways
> > for making these look consistent and nice...  where we'll likely be doing these
> > types of hacks behind the scenes, giving you another opportunity to be outraged
> > ;)
> 
> Behind-the-scenes hacks suck too, but in-front-of-the-scenes hacks ****
> suck.  Too bad.
> 
> > Part of the feature is that people can stick web content in these widgets.
> > Stock tickers, SVG clocks, snowglobes. Expect real-life widgets to be far
> > scarier than this piddly example.
> 
> I don't know why we're making these little 24x24 or whatever squares web pages.
>  Making people jump through hoops to make the word "HI" look nice sounds like a
> great idea.  If I'm a developer coming to Jetpack, just ask me for my icon, let
> me hook up some events to it, give me some APIs for doing things like badges
> and notifications, and make it look nice.

Playing with the prototype, that was my gripe! I didn't care about adding headers, footers, CSS and other ****. I just wanted an icon in there that I/users could click and have perform a function. I understand that in rare cases users may want to overlay icon with text (e.g. tickers/email count), but in most cases, I simply wanted to stick in an icon and have it clickable. There were a few cases where I knocked up something to have text next to it and in order to get it perfectly aligned, it took far too much work. I felt I should've simply been table to stick in the location of the icon and the text I want displayed and have Jetpack gracefully do the rest.
(Assignee)

Comment 35

8 years ago
(In reply to comment #33)
> Behind-the-scenes hacks suck too, but in-front-of-the-scenes hacks fucking
> suck.  Too bad.

I do not know what you're asking for here.

> I don't know why we're making these little 24x24 or whatever squares web pages.

That's a qood question. Well, it's a statement, but a question was implied!

IMO we cannot imagine all the uses that developers will put these spaces to. So we offer them the same freedoms they have with the web.

> If I'm a developer coming to Jetpack, just ask me for my icon, let
> me hook up some events to it, give me some APIs for doing things like badges
> and notifications, and make it look nice.

This is a useful suggestion. As I already said, we'll support loading images from the packages. WRT to notifications and badges - those are great ideas for once we get the basic infrastructure in place, and are examples of the things we expect to put in the next iteration, depending on the feedback we get about what developers *actually* want to do here.
(Assignee)

Comment 36

8 years ago
(In reply to comment #34)
> Playing with the prototype, that was my gripe! I didn't care about adding
> headers, footers, CSS and other crap. I just wanted an icon in there that
> I/users could click and have perform a function. I understand that in rare
> cases users may want to overlay icon with text (e.g. tickers/email count), but
> in most cases, I simply wanted to stick in an icon and have it clickable. There
> were a few cases where I knocked up something to have text next to it and in
> order to get it perfectly aligned, it took far too much work. I felt I
> should've simply been table to stick in the location of the icon and the text I
> want displayed and have Jetpack gracefully do the rest.

That's exactly what the Google example does. You just put in a URL to an image, and onClick handler. How were you not able to do that?

Also, see the second half of comment #35. This is exactly the feedback we want, so thanks! Practical use-cases are much more useful than conjecture.
(Assignee)

Comment 37

8 years ago
Paul, also note that the change Drew suggested to use local images from the package will make it so your icon can be in the extension itself, which should make things even easier for your use-case.

Comment 38

8 years ago
Personally, I'm hoping for something as simple as:

	call({
		icon: location.gif,
		text:,
		width: 50,
		leftclick:, // essentially onReady
		rightclick:, // populous of context menu
	});

I honestly don't see the need for anything else. Text in the status bar is annoying and so I'm guessing my thoughts and feelings would be carried over. That said, it should be a freedom afforded to developers. You can currently edit the context menu with JS, so I don't see why the option wouldn't be there for users. And left click is well, onClick. You don't need more than that and besides, in terms of standardisation, anything else shouldn't be encouraged. The above example, doesn't make it so easy to create a dynamic icon. I've had thoughts on how to achieve that within the current model, but I believe that better ones are already flying about. But essentially. I believe, at least for me, the beauty of Jetpack was the promised simplicity. I was utterly disappointed when I had to start playing with fonts and padding just to get something to display at I wanted in the status bar, I'm hoping that won't happen again, though I may be too late.
(Assignee)

Comment 39

8 years ago
Thanks Paul. Right now that'd look like:

widget.add(widget.Widget({
  label: "my widget",
  content: "http://site/location.gif",
  onClick: function() {...}
});

We don't have right-click support yet, but great idea, i'll add it to the spec. 

I'd like to have the icon support be as simple as your example. right now with the local datastore it'd look something like:

var self = require("self");
widget.add(widget.Widget({
  label: "my widget",
  content: self.data.url("mystuff/location.gif"),
  onClick: function() {...}
});

Or if we make a separate property for icons, maybe we could support only local data, and do the url cruft behind the scenes.
(Assignee)

Comment 40

8 years ago
(In reply to comment #38)
> I honestly don't see the need for anything else. Text in the status bar is
> annoying and so I'm guessing my thoughts and feelings would be carried over.
> That said, it should be a freedom afforded to developers. 

Bingo, simplicity *and* freedom is the sweet spot.

Simple use-cases like a static image and an onClick handler are easy now, and will continue to be. Not-simple use-cases that you and I can't imagine will be possible, but not the common case.

Comment 41

8 years ago
Maybe I'm oversimplifying things here but:

var store = require("store");
widget.add(widget.Widget({
  label: "my widget", // optional
  icon: store.jetpackname.icon, // alternatively, simply "http://site/location.gif"
  alt: "my widget", // by default, it's the same as label
  onClick: function() {...},
  altClick: function() {...} // I know this isn't currently available. But I think it's critical to what you're trying to achieve here.
});

For me, that'd be perfect and the scope covers all uses I can think of.

Initially I forgot the alt/hover value. That can be important.

Comment 42

8 years ago
I forgot to mention that altClick should be as simple as:

altClick: ({[{
  type: option, // or you could have an 'openpreferences' and other stuff
  label: "my right click option"
  bold: yes,
  onClick: function() {...}
},{
  type: openpreferences,
  label: "my right click option 2"
  bold: no,
  onClick: function() openprefs(store.jetpackname.preferences)
}]});

Comment 43

8 years ago
(In reply to comment #41)
> Maybe I'm oversimplifying things here but:
> 
> var store = require("store");
> widget.add(widget.Widget({
>   label: "my widget", // optional
>   icon: store.jetpackname.icon, // alternatively, simply

A tangent, but the idea of a bundled file -> object mapping like this is interesting.  Drop your file in data/icons/hover.png for example and access it from code as require("data").icons.hover.  You don't have to worry about whether the "icon" property is a URL string or URL object or whatever.
(Assignee)

Comment 44

8 years ago
I talked with Drew about the content property - he suggested limiting it to an image in this release, and getting feedback as to whether this satisfies developer requirements, before putting an entire web environment in the widget view.

I'm on the fence. I feel that this doesn't leave much room for experimentation. However, it also might get us better feedback, as developers will feel constrained and ask for more specific features. Myk, Atul, Aza, any input on how best to go about this?

I think it's also not clear to me what the lesson learned from the Jetpack prototype is. Was there any conclusion about the usefulness of web content in these size-constrained spaces? These widgets are taller than the statusbar, but they're also static in size, and have a larger view via the panel (or will).

Comment 45

8 years ago
For what it's worth, we did actually *want* to include optional icon/label keywords in the status bar API for jetpack that would provide for such simple no-brainer functionality--and would also consume less system resources--but we just never got around to it. So I think that just supporting an image in this release is fine, and we can scale up to more functionality from there.

The main case in which the full power of HTML was useful was for statusbar widgets that wanted to offer particularly slick-looking or unusual UIs, like the animated sliding motion of Aza's original ad-blocking jetpack (which was launched with the prototype).
(In reply to comment #28)
> >+  // Note the CSS hurdles just to make text nicely centered. Ugh.
> >+  content: "<div style='text-align: center; display: table-cell; vertical-align: middle; width: 24px; height: 24px; font-size: small; font-weight: bold; padding: 0px;'>HI.</div>"
> 
> OMG.  Are we really shipping this while it requires these kinds of things to
> make your widget look nice?  "display: table-cell"?  That's not acceptable.

Some CSS (like |margin: 0| and centering) should be applied by the module to all widgets, just as Firefox applies a default stylesheet to all web pages, to provide sensible defaults for the widget context (which the widget developer can then choose to override).

Other CSS is primarily the responsibility of the widget developer, though.  After all, the whole point of supporting iframes is to enable developers to customize the appearance and behavior of their widgets.


(In reply to comment #44)
> I think it's also not clear to me what the lesson learned from the Jetpack
> prototype is. Was there any conclusion about the usefulness of web content in
> these size-constrained spaces?

Yes, the lesson learned was that developers can do a variety of interesting and useful things within an iframe that they can't do with an icon, and we should absolutely support that generative capability in any successor to the prototype's statusbarpanel API.


(In reply to comment #40)
> (In reply to comment #38)
> > I honestly don't see the need for anything else. Text in the status bar is
> > annoying and so I'm guessing my thoughts and feelings would be carried over.
> > That said, it should be a freedom afforded to developers. 
> 
> Bingo, simplicity *and* freedom is the sweet spot.
> 
> Simple use-cases like a static image and an onClick handler are easy now, and
> will continue to be. Not-simple use-cases that you and I can't imagine will be
> possible, but not the common case.

Exactly.
(Assignee)

Comment 47

8 years ago
Created attachment 445989 [details] [diff] [review]
wip11

- most of myk and drew's fixes
- adds an image property that takes a string filename of a local data file
Attachment #445566 - Attachment is obsolete: true
(Assignee)

Comment 48

8 years ago
Created attachment 446119 [details] [diff] [review]
wip12

- unification of all event handling
- support onLoad for images, onReady for content
- proper removal of all events
- reworked BrowserWindow for code clarity
Attachment #445989 - Attachment is obsolete: true
(Assignee)

Comment 49

8 years ago
The last bug I've been fighting turns out to be an interesting case. In order to support a simpler style for local images like:

{
  image: "myfile.jpg"
}

I need to generate the URL via require("self").data.url(widget.image) in the widget module.

However, the self module generates the URLs based on the calling module's package data... which means that the base URL for the image is the *widget* module's package and not the addon-author's package.

Core modules need a way to access caller's package context maybe? Or we punt on supporting the simpler syntax for now, instead shipping 0.4 with:

{
  image: require("self").data.url("myfile.jpg")
}
(Assignee)

Comment 50

8 years ago
Created attachment 446121 [details] [diff] [review]
wip13

doc updates in progress, more after dinner. code is ready for review, outside of the image url issue in the previous comment.
Attachment #446119 - Attachment is obsolete: true
Attachment #446121 - Flags: review?(myk)
(Assignee)

Comment 51

8 years ago
Created attachment 446134 [details] [diff] [review]
wip14

- docs updated
- examples updated
Attachment #446121 - Attachment is obsolete: true
Attachment #446134 - Flags: review?(myk)
Attachment #446121 - Flags: review?(myk)
(Assignee)

Comment 52

8 years ago
Created attachment 446138 [details] [diff] [review]
wip15

- add a random-flickr-photo example to the docs
Attachment #446134 - Attachment is obsolete: true
Attachment #446134 - Flags: review?(myk)
(Assignee)

Updated

8 years ago
Attachment #446138 - Flags: review?(myk)
(Assignee)

Comment 53

8 years ago
Created attachment 446143 [details] [diff] [review]
wip16

- adding the static files used in the tests
Attachment #446138 - Attachment is obsolete: true
Attachment #446143 - Flags: review?(myk)
Attachment #446138 - Flags: review?(myk)
Comment on attachment 446143 [details] [diff] [review]
wip16

>diff --git a/packages/jetpack-core/docs/widget.md b/packages/jetpack-core/docs/widget.md

>+Currently only the small widget view in the bar is supported.
>+Subsequent releases will allow authors to specify a larger panel
>+for displaying rich content. You may hook up extended UI via the
>+supported events, however keep in mind that direct access to the
>+browser's XUL window DOM may break in the very near future, likely
>+in the 0.5 release.

Nit: it would be useful to mention that access to events will probably change as well (at least for chrome events).


>+    <td><tt>onLoad</tt></td>
>+    <td>
>+      An optional function to be called when <tt>Widget</tt> content
>+      that is an image is loaded. If the <tt>Widget</tt>'s content is HTML
>+      then use the <tt>onReady</tt> event instead.

Nit: onLoad should actually work fine for HTML content.  It just fires later than onReady, during the load event (i.e. after all associated resources, like scripts, stylesheets, and images, have loaded).  So rather than telling people this gets called when an image is loaded, I would tell them that it gets called when the content is loaded (no matter what it is), but continue to recommend that addons use onReady when their content is HTML.


>diff --git a/packages/jetpack-core/lib/widget.js b/packages/jetpack-core/lib/widget.js

>+    this.windows.forEach(function (w, i) w.updateItem(item, property, value));

Nit: argument "i" isn't used so doesn't need to be specified.


>+        let imageURL = require("self").data.url(item.widget.image);
>+        iframe.setAttribute("src", imageURL);

Since this doesn't seem to work, as we discussed on IRC, let's go with requiring the addon to make the call to require("self").data for now and figure out a better solution in the next phase.


>+      // TODO: special-casing of images will be replaced, probably by an

It looks like this comment got cut off.


>+      if (doc.body.childNodes.length == 1 &&
>+          doc.body.firstElementChild &&
>+          doc.body.firstElementChild.tagName == "IMG") {
>+        // Force image content to size.
>+        // Add-on authors must size their images correctly.
>+        doc.body.firstElementChild.style.width = "24px";
>+        doc.body.firstElementChild.style.height = "24px";
>+
>+      }
>+
>+      // Allow all content to fill the box by default.
>+      doc.body.style.margin = "0px"; // Or just 0?

0px works fine, but simply 0 is more conventional in Mozilla code (since 0 means the same thing regardless of the measurement unit).


>+        if(!loadURIIsMatch(e.target.location, item.widget))
>+          return

Nits: if( -> if (    and    return -> return;


>diff --git a/packages/jetpack-core/tests/test-widget.js b/packages/jetpack-core/tests/test-widget.js

>+      widgets.remove(this)

Nit: semi-colon at end of line (here and at the end of similar lines elsewhere).


Otherwise, this looks great.  However, while tests pass on 3.6, they fail on trunk with a stacktrace that ends:

  File "resource://jetpack-core-jetpack-core-tests/test-widget.js", line 12, in 
    let doc = browserWindow.document;
TypeError: browserWindow is null

Also, when I try the examples in the docs, I don't see the addon bar (neither in 3.6 nor on trunk).  Is there something special I need to do to make it appear?
Attachment #446143 - Flags: review?(myk) → review-
(Assignee)

Comment 55

8 years ago
Created attachment 446153 [details] [diff] [review]
wip17

- all review comments fixed
- flip UI on by default
Attachment #446143 - Attachment is obsolete: true
Attachment #446153 - Flags: review?(myk)
(Assignee)

Comment 56

8 years ago
Created attachment 446156 [details] [diff] [review]
wip18

- only change is a new test for multiple window support
Attachment #446153 - Attachment is obsolete: true
Attachment #446156 - Flags: review?(myk)
Attachment #446153 - Flags: review?(myk)
Comment on attachment 446156 [details] [diff] [review]
wip18

Peaches! r=myk
Attachment #446156 - Flags: review?(myk) → review+
(Assignee)

Comment 58

8 years ago
thanks! pushed: https://hg.mozilla.org/labs/jetpack-sdk/rev/c7aac052fdd3
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 568932

Comment 59

8 years ago
The rest of modules uses |const apiUtils = require("api-utils");| (note camelCase), perhaps this code should be changed to match that too?
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.