Closed Bug 568932 Opened 10 years ago Closed 10 years ago

update widget implementation to be ready to support the Firefox addon bar and toolbar customization

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: dietrich)

References

()

Details

Attachments

(1 file, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #543585 +++

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.

The first part of this, in bug 543585, added the SDK API for building widgets, and an experimental UI above the status bar to host them.

This second part is for finalizing where and how the widgets are displayed in the browser.

Boriss is designing the UX for this. See the initial blog post here:

http://jboriss.wordpress.com/2010/04/29/removing-firefoxs-status-bar-and-rehousing-add-on-icons-part-1-of-2/
Target Milestone: -- → 0.5
This is going to mostly Firefox changes, not Jetpack. Boriss' latest design:

http://jboriss.wordpress.com/2010/06/16/removing-firefox%E2%80%99s-status-bar-and-rehousing-add-on-icons-part-3-of-2-wut/
Target Milestone: 0.5 → --
Spec here for the moment, with Boriss' latest-latest design:

https://wiki.mozilla.org/User:Dietrich/Scratchpad

See bug 574688 for removing the status bar by default.
Depends on: 574688
Attached patch wip (obsolete) — Splinter Review
* use the status bar instead of a custom container
Assignee: nobody → dietrich
Blocks: 574688
No longer depends on: 574688
spec is here: https://wiki.mozilla.org/Firefox/Projects/AddonUI
Summary: Firefox support for add-on visibility in primary UI → update widget implementation to support the Firefox addon bar and toolbar customization
Attached patch wip2 (obsolete) — Splinter Review
* change id to addonbar
* change widgets to toolbar buttons in the browser palette
* change bar to a toolbar
* populate bar through the palette

todo:
* widgets need proper ids
* widgets need styling fixed
* widgets-as-toolbar-buttons don't show up in the palette UI
* figure out how to get toolbars not in navigator-toolbox into the View menu
* figure out how to get toolbar items to be drag/droppable onto the addon bar
* how to persist order?
Attachment #454047 - Attachment is obsolete: true
Attached patch wip3 (obsolete) — Splinter Review
* changed to toolbaritems, fixes styling
* widgets have proper ids now
Attachment #455572 - Attachment is obsolete: true
These need to be fixed in Firefox in separate bugs. They don't block this bug.

* widgets-as-toolbar-buttons don't show up in the palette UI
* figure out how to get toolbars not in navigator-toolbox into the View menu
* figure out how to get toolbar items to be drag/droppable onto the addon bar
* how to persist order?
(In reply to comment #7)
> These need to be fixed in Firefox in separate bugs. They don't block this bug.
> 
> * widgets-as-toolbar-buttons don't show up in the palette UI

filed bug 579500

> * figure out how to get toolbars not in navigator-toolbox into the View menu

filed bug 579506

> * figure out how to get toolbar items to be drag/droppable onto the addon bar

talked with UX, not going to do this. i'll update the spec with the outcome of the discussions at the summit.

> * how to persist order?

not sure how to move forward with this, but it's not blocking forward movement. filed bug 579505 to keep it on the radar.
Attached patch wip4 (obsolete) — Splinter Review
Attachment #457962 - Flags: review?(myk)
Attachment #455696 - Attachment is obsolete: true
Attached patch wip4.1 (obsolete) — Splinter Review
minor change, forgot to update the test that checks the element widgets are hosted in.
Attachment #457962 - Attachment is obsolete: true
Attachment #457975 - Flags: review?(myk)
Attachment #457962 - Flags: review?(myk)
(In reply to comment #8)
> (In reply to comment #7)
> > These need to be fixed in Firefox in separate bugs. They don't block this bug.
> > 
> > * widgets-as-toolbar-buttons don't show up in the palette UI
> 
> filed bug 579500

Resolved RTFM: As of Firefox 3, items in-use on toolbars do not show up on the palette. If I do not add widgets to the add-on bar, they show up in the customization palette.

I'll handle the following pieces in other bugs:

1. integrating the add-on bar into customization so that widgets can be dragged on/off.

2. persisting customizations on a per-widget basis, so that widgets load (or not) and in the right places.
Attachment #457975 - Flags: review?(myk) → review?(adw)
Summary: update widget implementation to support the Firefox addon bar and toolbar customization → update widget implementation to be ready to support the Firefox addon bar and toolbar customization
Comment on attachment 457975 [details] [diff] [review]
wip4.1

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

>+      let container = this.doc.getElementById("addon-bar");
>+      if (!container) {

You might comment that the toolbox and addon-bar created here are probably only placeholders, point to bug(s).

>+        container.setAttribute("style", "height: 100px; padding: 0px; margin: 0px;");

Nit: 80 chars or less.  Also, could you just set each container.style.foo = "bar"?

>+        container.setAttribute("toolbarname", "Add-ons Toolbar");

Maybe a TODO about l10n'ing this name.

>+        // TODO: make part of toolbar infrastructure, so is controlled
>+        // via the View menu instead of pref. (bug 579506)
>+        container.hidden = require("preferences-service").
>+                           get("jetpack.jetpack-core.widget.barIsHidden",
>+                               PREF_DEFAULT_ADDON_BAR_HIDDEN);

Nit: The default value is a const, so why not the pref name too?

>   // Remove container
>   _removeContainer: function BW__removeContainer() {

This method needs to be updated.  Since this._container is the toolbar and not the toolbox, the toolbox will be left over.  Which also means that the next time the toolbox and toolbar are created, there will be two toolboxes, one of which is empty.

If there are any other places that still assume this._container.parentNode is the status bar parent, they need to be updated too.

>@@ -321,29 +340,43 @@ BrowserWindow.prototype = {

>   // Add a widget to this window.
>   _addItemToWindow: function BW__addItemToWindow(widget) {
>     // XUL element container for widget
>-    let node = this.doc.createElement("hbox");
>+    let node = this.doc.createElement("toolbaritem");
>+    let id = require("self").id + widget.label.replace(/[^a-z0-9]/gi, "");
>+    node.setAttribute("id", id);

I like not pestering the dev for IDs for all of his widgets.  But this particular impl has a couple of problems:

1) If my extension has two widgets with the same label, my toolbaritems will have the same ID, which is likely to cause problems.  I think it's OK to mandate that my widgets have unique labels, but the Widget ctor should prevent me from breaking that rule.

2) If my labels aren't ASCII (e.g., Chinese or whatever -- or actually with /[^a-z0-9]/ just all caps or punctuation), all my toolbaritems will have the same ID.

Assuming Gecko's XUL DOM impl follows the HTML 4.01 spec with regard to IDs (they "must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits, hyphens, underscores, colons, and periods" [1]), I'd recommend instead hashing the label or just generating a GUID.

[1] http://www.w3.org/TR/1999/REC-html401-19991224/types.html#type-name

>+    // Ensure container is visible
>+    // TODO: need to figure out when it's appropriate to open it.
>+    if (this.container.getAttribute("hidden"))
>+      this.container.setAttribute("hidden", "false");

You set the `hidden` property above in `get container()` -- to avoid issues where the property and attribute aren't properly synced up, I think you should be consistent and instead set the attribute there or set the property here.

>+    // Add to the customization palette
>+    // TODO: Why doesn't this actually show up on the palette? (bug 579500)

Does it need some magic class, "chromeclass-toolbar-additional" or "toolbarbutton-1" or something?

r- for _removeContainer, the ID issue, and the `hidden` property-vs.-attribute inconsistency.
Attachment #457975 - Flags: review?(adw) → review-
(In reply to comment #12)
> >+    // Add to the customization palette
> >+    // TODO: Why doesn't this actually show up on the palette? (bug 579500)
> 
> Does it need some magic class, "chromeclass-toolbar-additional" or
> "toolbarbutton-1" or something?

Oh, I just clicked through to that bug, and it's marked invalid.  So this comment should be removed, right?
Attached patch wip5Splinter Review
comments addressed
Attachment #457975 - Attachment is obsolete: true
Attachment #458579 - Flags: review?(adw)
Comment on attachment 458579 [details] [diff] [review]
wip5

>+      // Bug 574688 replaces the status bar with the add-on bar. This code
>+      // might be removed when that bug is resolved. It might stay, if we 
>+      // want to support version of Firefox that don't have the add-on bar.

Ubernit: version -> versions

>   // Add a widget to this window.
>   _addItemToWindow: function BW__addItemToWindow(widget) {

This method no longer unhides the addon-bar like the previous patch did.  Is that intentional?

>+    let id = "widget: " + guid;
>+    node.setAttribute("id", id);

guid contains a space and curly braces, which according to the (HTML) spec aren't allowed, but Firefox doesn't complain.  Cool.

r=adw
Attachment #458579 - Flags: review?(adw) → review+
http://hg.mozilla.org/labs/jetpack-sdk/rev/31b3ae0dfd69

a=myk on post-freeze landing.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
No longer depends on: jetpack-panel-apps
Target Milestone: -- → 0.6
(In reply to comment #15)
> This method no longer unhides the addon-bar like the previous patch did.  Is
> that intentional?

yes - for now we should respect the pref. once the add-on bar is in firefox, we'll remove the pref in favor of the usual xul persistence mechanisms.
Will we still be able to drag and drop the add-on icons on the title bar (top of the window) like in the mockup? Or does that belong in another bug?
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.