Closed
Bug 568932
Opened 15 years ago
Closed 14 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)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
0.6
People
(Reporter: dietrich, Assigned: dietrich)
References
()
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
+++ 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/
Assignee | ||
Updated•15 years ago
|
Target Milestone: -- → 0.5
Assignee | ||
Comment 1•14 years ago
|
||
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 → --
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
* use the status bar instead of a custom container
Assignee: nobody → dietrich
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
* 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
Assignee | ||
Comment 6•14 years ago
|
||
* changed to toolbaritems, fixes styling
* widgets have proper ids now
Attachment #455572 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
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?
Assignee | ||
Comment 8•14 years ago
|
||
(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.
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #457962 -
Flags: review?(myk)
Assignee | ||
Updated•14 years ago
|
Attachment #455696 -
Attachment is obsolete: true
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #457975 -
Flags: review?(myk) → review?(adw)
Assignee | ||
Updated•14 years ago
|
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 12•14 years ago
|
||
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-
Comment 13•14 years ago
|
||
(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?
Assignee | ||
Comment 14•14 years ago
|
||
comments addressed
Attachment #457975 -
Attachment is obsolete: true
Attachment #458579 -
Flags: review?(adw)
Comment 15•14 years ago
|
||
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+
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/labs/jetpack-sdk/rev/31b3ae0dfd69
a=myk on post-freeze landing.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
No longer depends on: jetpack-panel-apps
Assignee | ||
Updated•14 years ago
|
Target Milestone: -- → 0.6
Assignee | ||
Comment 17•14 years ago
|
||
(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.
Comment 18•14 years ago
|
||
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?
Comment 19•14 years ago
|
||
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.
Description
•