This will likely be superseded by better UI down the road.

Dashboard implementation, inspired by the mockup at http://people.mozilla.com/~faaborg/files/projects/apps/creation-i1/added.html

Updated patch, with support for a context menu on apps with a "Delete application" entry.

Comment on attachment 577993 [details] [diff] [review] wip patch This looks great for a temporary UI. I don't think you would even need the drag-here-to-delete part, right-clicking it to remove would also work. Generally, people rarely delete apps from e.g. their phones or computers, because they have perceived or real "value", and people are afraid of losing them or not finding them should they need them later. Therefore, prioritizing deletion as a very dominant action isn't really necessary.

Ok, I'll remove the drag to delete stuff, and just keep the right-click context menu action.

Updated patch addressing UI comments.

Comment on attachment 578319 [details] [diff] [review] patch Imma steal this from gavin, but feel free to steal it back!

(In reply to Andreas Gal :gal from comment #0) > This will likely be superseded by better UI down the road. How likely? If there's a chance that this will stay around, then browser/base/content/aboutApps.css should be replaced with files in browser/themes/. By the way, use two-space indentation in CSS and don't hardcode "background-color: white" (use -moz-field and -moz-fieldtext instead).

I put the css in browser/base/content/ since other about pages do this (about:home and about:sync-tabs) Thanks for the tips with -moz-field!

about:home had some special reasons, I think. about:sync-tabs uses chrome://browser/skin/aboutSyncTabs.css.

Comment on attachment 578319 [details] [diff] [review] patch I'm gonna update this patch.

Updated to address Dao's comments.

rebased patch

Comment on attachment 581669 [details] [diff] [review] patch Review of attachment 581669 [details] [diff] [review]: ----------------------------------------------------------------- Just a few comments, including some small changes for accessibility, and showing something useful when no webapps are installed. Also, did you guys sort out the dueling-APIs issue, such that the the stuff being blogged about on https://hacks.mozilla.org/2011/12/mozilla-labs-apps-preview/ is compatible with this patch? Other than these things, I think the patch should be fine with the noted changes since about:apps will be a temporary spot as web apps mature. ::: browser/base/content/aboutApps.js @@ +52,5 @@ > + > + navigator.mozApps.enumerateAll(function(aApps) { > + for (let i = 0; i < aApps.length; i++) > + addApplication(aApps[i]); > + }); It would be nice to show _something_ for when there are no web apps installed (which will be true for everyone initially!) I'd suggest adding a div to the .xhtml with a simple "No web apps installed. Get some from the <a>app store</a>.", and then hide/show it here based on the count. Sadface for this API being an enumeration, instead of an array with .length. @@ +57,5 @@ > +} > + > +function addApplication(aApp) { > + let list = document.getElementById("appgrid"); > + let manifest = new DOMApplicationManifest(aApp.manifest, aApp.origin); Hmm, kind of a weird API to create a manifest from aApp.manifest. :| @@ +63,5 @@ > + img.src = manifest.iconURLForSize("64"); > + let container = document.createElement("div"); > + container.className = "app"; > + container.setAttribute("id", "app-" + aApp.origin); > + container.setAttribute("contextmenu", "appmenu"); Please set |tabindex| to "0", for A11Y (so keyboard users can tab to an item and trigger the context menu on it.) @@ +92,5 @@ > +} > + > +function deleteApp(aEvent) { > + if (gCurrentApp) > + navigator.mozApps.uninstall(gCurrentApp); It would be better to use the target of the context menu to derive the app (ie, look at target.id) instead of using a mouseover to set gCurrentApp... The mouseover stuff is prone to getting out of sync. And without a mouseleave handler you might unexpectedly delete an app from a previous mouseover when you're, say, 1px shy of the app you think you're hovering over. ::: browser/base/content/aboutApps.xhtml @@ +68,5 @@ > + <div id="appgrid"/> > + <div class="spacer"> </div> > + </div> > + <menu type="context" id="appmenu"> > + <menuitem label="&aboutapps.ctxtDelete;" onclick="deleteApp(event)"></menuitem> Add an |id| and an |accesskey|.

Also, could you post a screenshot with a couple of apps installed? UX might want to do some visual tweaks, even if it is temporary. /cc limi.

(In reply to Justin Dolske [:Dolske] from comment #16) > Also, could you post a screenshot with a couple of apps installed? UX might > want to do some visual tweaks, even if it is temporary. /cc limi. I did in comment 2, and Limi said it was looking good in comment 4.

> Also, did you guys sort out the dueling-APIs issue, such that the the stuff being blogged about on https://hacks.mozilla.org/2011/12/mozilla-labs-apps-preview/ is compatible with this patch? Yes, we're working on it - on both sides. > @@ +57,5 @@ > > +} > > + > > +function addApplication(aApp) { > > + let list = document.getElementById("appgrid"); > > + let manifest = new DOMApplicationManifest(aApp.manifest, aApp.origin); > > Hmm, kind of a weird API to create a manifest from aApp.manifest. :| The naming may be misleading but DOMApplicationManifest is actually a helper that provides localization and a couple of other functions. > @@ +92,5 @@ > > +} > > + > > +function deleteApp(aEvent) { > > + if (gCurrentApp) > > + navigator.mozApps.uninstall(gCurrentApp); > It would be better to use the target of the context menu to derive the app (ie, look at target.id) instead of using a mouseover to set gCurrentApp... The mouseover stuff is prone to getting out of sync. And without a mouseleave handler you might unexpectedly delete an app from a previous mouseover when you're, say, 1px shy of the app you think you're hovering over. So, using the target (or originalTarget) of the context menu didn't work since it is always the menuitem. Maybe we should change the behavior of contextmenu? Setting gCurrentApp to null in a mouseleave handler also has issues since mouseleave is fired after the right click when we enter in the context menu. So we always end up with a null gCurrentApp

Comment on attachment 581880 [details] [diff] [review] updated patch Review of attachment 581880 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the L10N change. ::: browser/locales/en-US/chrome/browser/aboutApps.dtd @@ +1,4 @@ > +<!ENTITY aboutapps.pageTitle "Your Apps"> > +<!ENTITY aboutapps.ctxtDelete "Delete Application"> > +<!ENTITY aboutapps.ctxtDelete.accesskey "D"> > +<!ENTITY aboutapps.noApps "No web apps installed. Get some from the <a href='https://apps-preview.mozilla.org/'>app store</a>."> Localizers apparently have a tendency to mangle URLs, and so the preferred way to do this is: &foo.pre;<a href="xxx">&foo.middle;</a>&foo.post; See: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd?force=1#31 and http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblem.xml?force=1#72 the .pre/.middle/.post allows localizations to have alternate sentence structures where the link may occur anywhere.

(In reply to Fabrice Desré [:fabrice] from comment #17) > > Also, could you post a screenshot with a couple of apps installed? UX might > > want to do some visual tweaks, even if it is temporary. /cc limi. > > I did in comment 2, and Limi said it was looking good in comment 4. Well, how 'bout that! ;-) All good then. (In reply to Fabrice Desré [:fabrice] from comment #18) > > Also, did you guys sort out the dueling-APIs issue [...] > Yes, we're working on it - on both sides. Ok. My main concern is to make sure we're not confusing developers (and eventually users) by having Labs making noise about one implementation, and Firefox shipping an incompatible implementation. Everybody loses there. > So, using the target (or originalTarget) of the context menu didn't work > since it is always the menuitem. Maybe we should change the behavior of > contextmenu? Hmm, no, it would be for the <menu> I thought. But I was actually thinking of .popupNode, which is only on XUL documents. In any case, meh, this should work good enough as-is. Temporary feature, doesn't have to be finely crafted.

I think we are moving along nicely to harmonize things. By the time we ship this you will be able to look at the same registry from either perspective (builtin and web-hosted dashboard). https://bugzilla.mozilla.org/show_bug.cgi?id=711138

Wouldn't it be more worth it if the final way to manage web apps is to combine about:apps with about:addons. Add-ons and Web-apps should in my opinion be managed together in a single manager, since both are extension to Firefox.

Patch rebased and updated to use the new API.

Comment on attachment 603059 [details] [diff] [review] patch updated to current tree, and minor changes to make use of the new API from bug 720415

Driveby: Looks like this should use inContent.css

You should use MPLv2 headers for the files added by this patch, and the CSS files should get license headers too. The strings file could use a localization note that explains that aboutapps.noApps.middle is linked text, and mention where it points to. The URL that's hardcoded in that patch (https://apps-preview.mozilla.org) doesn't seem to work, is that going to change before this lands?

I don't understand what is exactly the purpose of this page. From what I heard it's intended only for people working on the OWA project, not for users or app developers. Is that correct? If that's the case, I'm not sure that we would want to expose this via the about:apps URL, as once it's out there users will start using it to manage apps and we will have to support it forever. If it's targeted to users and this is just a barebones UI that will be improved later, then I believe it's fine.

For now we provide a way to install apps but not to manage them. I don't see why this should be limited to people of the OWA project, as a user I want that! For sure this is a very simple UI, and must be enhanced or moved somewhere else in the future (part of the new home page for instance).

Comment on attachment 603059 [details] [diff] [review] patch Review of attachment 603059 [details] [diff] [review]: ----------------------------------------------------------------- Some small changes in the patch, and please note the other comments from Gavin and Blair. Using inContentUI.css you can get much of the same look of about:addons, about:permissions for free ::: browser/base/content/aboutApps.js @@ +42,5 @@ > + navigator.mozApps.mgmt.oninstall = onInstall; > + navigator.mozApps.mgmt.onuninstall = onUninstall; > + updateList(); > +} > + It's better to set up a listener with this function for DOMContentLoaded instead of using <body onload=""> @@ +54,5 @@ > + request.onsuccess = function() { > + for (let i = 0; i < request.result.length; i++) > + addApplication(request.result[i]); > + if (!request.result.length) > + document.getElementById("noapps").className = ""; To make it easier to add more class names in the future, use .classList.remove("hidden") and .classList.add("hidden") for this @@ +62,5 @@ > +function addApplication(aApp) { > + let list = document.getElementById("appgrid"); > + let manifest = new DOMApplicationManifest(aApp.manifest, aApp.origin); > + let img = document.createElement("img"); > + img.src = manifest.iconURLForSize("64"); Please set the alt text of the image to the app name for accessibility @@ +70,5 @@ > + container.setAttribute("contextmenu", "appmenu"); > + container.setAttribute("tabindex", "0"); > + container.addEventListener("mouseover", function(aEvent) { > + gCurrentApp = aApp; > + }, false); This is too much of a hack (and won't work if you use the accesskey). Please remove this and on deleteApp you can use the target of the event to get the "id" attribute and get a reference to the app again to uninstall it. @@ +87,5 @@ > + if (node) > + return; > + addApplication(aEvent.application); > + document.getElementById("noapps").className = "hidden"; > +} this is up to you, but you could simplify this by having the className removal unconditionally called inside addApplication. Then you can drop the other usage of this above (inside `onsuccess`).

I see that you mentioned this in a previous comment: > So, using the target (or originalTarget) of the context menu didn't work since it is > always the menuitem. Maybe we should change the behavior of contextmenu? you should be able to get the intended target by following .parentNode as necessary

I addressed review comments, except the one related to the context menu. The event arget is always the menu itself, so followinf the parent chain only leads to menuitem -> menu -> div#main-content -> body

Note - A recent discussion said that about:apps is supposed to redirect to apps.persona.org. This bug shows there may be a disagreement on the approach. Please consult Jen Arguello before moving forward with the implementation.

about:apps is a builtin feature, apps.persona.org some web service that may or may not be down when the user is trying to run an app. Redirecting makes just about zero sense to me.

(In reply to Andreas Gal :gal from comment #33) > about:apps is a builtin feature, apps.persona.org some web service that may > or may not be down when the user is trying to run an app. Redirecting makes > just about zero sense to me. Okay. The underlying concern I have here is to answer this question - Are we targeting about:apps for public consumption? If not, and instead, this is more of a debugging tool for testing, then the implementation is okay to be done.

I don't think many users will navigate to about:apps, without primary UI guiding them there. Independently of that, I think that web-hosting the dashboard will come back and bite us in the ass in an epic fashion, but we can discuss that at some future I-told-you-so point in time.

(In reply to Andreas Gal :gal from comment #35) > I don't think many users will navigate to about:apps, without primary UI > guiding them there. Sounds good. > Independently of that, I think that web-hosting the dashboard will come back > and bite us in the ass in an epic fashion, but we can discuss that at some > future I-told-you-so point in time. Yeah I think there's been discussion on that significantly. Currently, it's hosted on myapps.mozillalabs.com. I believe the original reason was to support apps management and interaction if browsers outside of desktop/mobile firefox.

Jen - Could you provide insight here to give a product management perspective? Also FYI - This is out of scope for FF 14 until the UX flow is ironed out more.

Apps management UX is being worked out as was previously mentioned. Andreas point of having of the pitfalls of pointing to a hosted dashboard is definitely known and of course having a dashboard work offline is a requirement. I have no qualms with this bug. We won't be pointing to about:apps from the Marketplace for the April 26th release.

(In reply to Andreas Gal :gal from comment #35) > Independently of that, I think that web-hosting the dashboard will come back > and bite us in the ass in an epic fashion, but we can discuss that at some > future I-told-you-so point in time. Just to clarify, the bug summary seems to imply app management (organizing, uninstalling ?) as opposed to launching of apps. And assuming apps-in-the-cloud, wouldn't management require having a connection? If just launching apps, wouldn't it work with a hosted page that uses appcache? The data needed to fill the appcache-loaded page comes from the mozApps API which is available offline anyway.

(In reply to Edward Lee :Mardak from comment #39) > (In reply to Andreas Gal :gal from comment #35) > > Independently of that, I think that web-hosting the dashboard will come back > > and bite us in the ass in an epic fashion, but we can discuss that at some > > future I-told-you-so point in time. > Just to clarify, the bug summary seems to imply app management (organizing, > uninstalling ?) as opposed to launching of apps. And assuming > apps-in-the-cloud, wouldn't management require having a connection? I don't think that really actually matters. about:apps is intended as a just a temporary, bare-bones UI to help bootstrap things. It's ok if it's incomplete or doesn't work in some cases; it's intended to be replaced by something better.

(In reply to Justin Dolske [:Dolske] from comment #40) > (In reply to Edward Lee :Mardak from comment #39) > > (In reply to Andreas Gal :gal from comment #35) > > > Independently of that, I think that web-hosting the dashboard will come back > > > and bite us in the ass in an epic fashion, but we can discuss that at some > > > future I-told-you-so point in time. > > Just to clarify, the bug summary seems to imply app management (organizing, > > uninstalling ?) as opposed to launching of apps. And assuming > > apps-in-the-cloud, wouldn't management require having a connection? > > I don't think that really actually matters. about:apps is intended as a just > a temporary, bare-bones UI to help bootstrap things. It's ok if it's > incomplete or doesn't work in some cases; it's intended to be replaced by > something better. In the short term, why not just use https://myapps.mozillalabs.com/ then? It works as a temporary place for app management. When we flesh out more details, let's revisit the approach for where app management should take place (I believe there is a separate bug tracking this).

Comment on attachment 614861 [details] [diff] [review] updated patch Review of attachment 614861 [details] [diff] [review]: ----------------------------------------------------------------- Driveby review time again! Only looked at a few things. ::: browser/base/content/aboutApps.xhtml @@ +26,5 @@ > + <script type="text/javascript;version=1.8" src="chrome://browser/content/aboutApps.js"/> > + </head> > + > + <body dir="&locale.dir;"> > + <div id="main-content"> This should be class="main-content" - then inContentUI.css will style it appropriately for different OSes (at the moment, you have winstripe and pinstripe looking like gnomesripe). @@ +28,5 @@ > + > + <body dir="&locale.dir;"> > + <div id="main-content"> > + <div>&aboutapps.pageTitle;</div> > + <div id="noapps" class="hidden"> Why not just use the built-in hidden attribute, instead of a custom class? ::: browser/themes/gnomestripe/aboutApps.css @@ +25,5 @@ > + height: 85px; > + font-size: 10px; > +} > + > +.app img { Try to avoid the descendant selector (it's slow). Either use the child selector (.app > img), or just add a class to the <img> and use just that. (https://developer.mozilla.org/en/CSS/Writing_Efficient_CSS)

Should we keep this bug around? bug 748955, bug 748962, bug 748959 have all defined apps integration points into firefox for k9o. We might want to shift our focuses there, given that those features are the intended integration points for apps (e.g. new tab, awesome bar).

We could probably need this page to manage applications on Linux (on desktop environments that don't support jumplist actions [https://live.gnome.org/Design/Whiteboards/Jumplists])

(In reply to Marco Castelluccio from comment #44) > We could probably need this page to manage applications on Linux (on desktop > environments that don't support jumplist actions > [https://live.gnome.org/Design/Whiteboards/Jumplists]) I know what you are talking about. Why couldn't use the new tab or awesome bar features mentioned in comment 43?

(In reply to Jason Smith [:jsmith] from comment #45) > I know what you are talking about. Why couldn't use the new tab or awesome > bar features mentioned in comment 43? Well, we could use the new tab I think, but I'm not an UX expert. (For people that don't know what I'm talking about, the problem is the removal of applications on Linux)

Folks here are mostly in agreement that: 1. Firefox must acquire a user-friendly interface for managing your apps; 2. about:apps is not that interface; 3. it is nevertheless useful for developers and testers of the feature (analogous to about:config, about:crashes, and about:memory); 4. we should land it for that purpose in addition to the user-friendly interfaces that we do expose to users. So let's do it.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #47) > Folks here are mostly in agreement that: > > 1. Firefox must acquire a user-friendly interface for managing your apps; > 2. about:apps is not that interface; > 3. it is nevertheless useful for developers and testers of the feature > (analogous to about:config, about:crashes, and about:memory); > 4. we should land it for that purpose in addition to the user-friendly > interfaces that we do expose to users. > > So let's do it. We aren't going to be able to target this for firefox 15 - launching the native webRT shells aren't supported in Firefox 15. Another problem is distinguishing definition of the different levels of uninstall (uninstall from profile vs. uninstall from native app), but that could be resolved down the line.

(In reply to Jason Smith [:jsmith] from comment #48) > (In reply to Myk Melez [:myk] [@mykmelez] from comment #47) > > Folks here are mostly in agreement that: > > > > 1. Firefox must acquire a user-friendly interface for managing your apps; > > 2. about:apps is not that interface; > > 3. it is nevertheless useful for developers and testers of the feature > > (analogous to about:config, about:crashes, and about:memory); > > 4. we should land it for that purpose in addition to the user-friendly > > interfaces that we do expose to users. > > > > So let's do it. > > We aren't going to be able to target this for firefox 15 - launching the > native webRT shells aren't supported in Firefox 15. Another problem is > distinguishing definition of the different levels of uninstall (uninstall > from profile vs. uninstall from native app), but that could be resolved down > the line. Talked this over on IRC - we could override the app.launch approach to go against the native webRT down the line. This is alright for a first implementation. Moving back to Firefox 15.

Note this bug conflicts with the proposal in bug 744985 - flagging productwanted. One of these bugs needs to marked as invalid, as both of them conflict in the proposal specified.

Per anant this does not conflict anymore.

Sorry to ask this, but is there a very significant difference between a app and a add-on? I see combining Add-ons manager and about:apps as the most efficient way to manage Apps and add-ons. but that is just my 2 cents.

Note: bug 744985 just landed on services-central, so when we decide to land this, we'll need to make a few tweaks to the patch to make sure about:apps redirects to the local dashboard instead of the remote one. This mostly just involves removing the about:apps logic from here and setting the services.aitc.dashboard.url pref to a resource:// or chrome:// URI.

We'd need an UI to manage applications on some Linux desktop environments. The solution we're using now should be standard (http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s08.html), but isn't supported yet by most desktop environments (and it isn't a priority for them).

(In reply to Andreas Gal :gal from comment #0) > This will likely be superseded by better UI down the road. This has now been superseded by OS-specific UI for managing apps, so we no longer need to have this temporary way to manage apps inside the browser. (In reply to Marco Castelluccio [:marco] from comment #55) > We'd need an UI to manage applications on some Linux desktop environments. > The solution we're using now should be standard > (http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s08.html), > but isn't supported yet by most desktop environments (and it isn't a > priority for them). I'd like to improve the experience for such users, but the interface described in this bug is too general a solution to that problem, which is limited to certain distributions of Linux. Rather than overload this bug with that issue, I filed bug 899360 to figure out a specific solution to that particular problem.

Fabrice and I chatted today about this. The original need has been addressed by native integration, and we haven't (yet?) identified another use case for this feature that is significant enough to justify its implementation. So I'm going to close this until such a use case is identified.