jetpacks without settings shouldn't appear to have them

RESOLVED FIXED in 0.7

Status

Mozilla Labs
Jetpack Prototype
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

unspecified

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Jetpacks that don't have settings appear to have them (an enabled setting button appears next to their entry in the list of installed jetpacks).  That shouldn't be the case, as it's confusing for users.

Two options for fixing this bug are to remove the button and to disable it.
(Assignee)

Updated

9 years ago
Assignee: nobody → myk
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: -- → 0.7
(Assignee)

Comment 1

9 years ago
Created attachment 417824 [details] [diff] [review]
patch v1: disables settings buttons for jetpacks without settings
Attachment #417824 - Flags: review?(avarma)
(Assignee)

Updated

9 years ago
Attachment #417824 - Flags: review?(avarma) → review?(edilee)

Comment 2

9 years ago
Comment on attachment 417824 [details] [diff] [review]
patch v1: disables settings buttons for jetpacks without settings

>+++ b/extension/content/css/index.css
>@@ -15,16 +15,30 @@ body {
>+.buttony[disabled] {
>+    background-color: #eeeeee;
>+span.buttony[disabled]:hover {
>+    background-color: #eeeeee;
>+span.buttony[disabled]:active {
>+    background-color: #eeeeee;
Are these to override the normal span:hover/active color changes? If you make the first coloring target span.buttony[disabled] instead of .buttony[disabled], it'll have the same "preciseness" as the earlier non-disabled matches, and because it's further down, it'll win. (Pretty sure..)

>+++ b/extension/content/js/app.js
>@@ -1,19 +1,26 @@
>+  _addButton: function _addButton(div, name, label, enabled) {
>+    if (typeof enabled == "undefined")
>+      enabled = true;
Any particular reason for converting missing/undefined to true and not just leave falsy?

>@@ -227,17 +234,20 @@ var App = {
>+    var context = JetpackRuntime.getJetpack(url);
>+    var enabled = context && context.manifest && context.manifest.settings;
>+    this._addButton(div, "open-settings", "settings", enabled);
Is it possible for context to be not null? manifest should always exist on a context with the change from bug 532724. So then you could just pass in context.manifest.settings...

Except if there's no settings property on the manifest, it'll get passed in as |undefined| and with the current logic, it'll be set to true.

Comment 3

9 years ago
Er. re: last part "possible for context to be not null" -- I meant the opposite :p
(Assignee)

Comment 4

9 years ago
Created attachment 418082 [details] [diff] [review]
patch v2: addresses review comments

(In reply to comment #2)
> (From update of attachment 417824 [details] [diff] [review])
> >+++ b/extension/content/css/index.css
> >@@ -15,16 +15,30 @@ body {
> >+.buttony[disabled] {
> >+    background-color: #eeeeee;
> >+span.buttony[disabled]:hover {
> >+    background-color: #eeeeee;
> >+span.buttony[disabled]:active {
> >+    background-color: #eeeeee;
> Are these to override the normal span:hover/active color changes? If you make
> the first coloring target span.buttony[disabled] instead of .buttony[disabled],
> it'll have the same "preciseness" as the earlier non-disabled matches, and
> because it's further down, it'll win. (Pretty sure..)

Ah, indeed, that works great.


> >+++ b/extension/content/js/app.js
> >@@ -1,19 +1,26 @@
> >+  _addButton: function _addButton(div, name, label, enabled) {
> >+    if (typeof enabled == "undefined")
> >+      enabled = true;
> Any particular reason for converting missing/undefined to true and not just
> leave falsy?

That was for backwards compatibility with older callers that don't specify a value for that parameter, and also so I didn't have to require them to do so, since label is an optional parameter, and it's annoying to add a required parameter after an optional one, because it makes the optional parameter no longer optional!

Perhaps a better way to express this is to reverse the meaning of the parameter, so it specifies the disabled state rather than the enabled one.  Then undefined can be treated as false, which is backwards compatible with existing buttons.

This patch does that.


> >@@ -227,17 +234,20 @@ var App = {
> >+    var context = JetpackRuntime.getJetpack(url);
> >+    var enabled = context && context.manifest && context.manifest.settings;
> >+    this._addButton(div, "open-settings", "settings", enabled);
> Is it possible for context to be not null? manifest should always exist on a
> context with the change from bug 532724. So then you could just pass in
> context.manifest.settings...

Yup, that's what I do now.

> Except if there's no settings property on the manifest, it'll get passed in as
> |undefined| and with the current logic, it'll be set to true.

With the previous change, this is no longer the case, and it works fine!
Attachment #417824 - Attachment is obsolete: true
Attachment #418082 - Flags: review?(edilee)
Attachment #417824 - Flags: review?(edilee)

Updated

9 years ago
Attachment #418082 - Flags: review?(edilee) → review+
(Assignee)

Comment 5

9 years ago
Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/57f2e3e7b23f.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 6

9 years ago
Myk, I'm getting an error with this changeset:

  Error: context is null
  Source File: chrome://jetpack/content/js/app.js
  Line: 243

243 is the _addButton call:

    var context = JetpackRuntime.getJetpack(url);
    this._addButton(div, "open-settings", "settings",
                    !context.manifest.settings);

It happens when I open about:jetpack and makes it impossible to uninstall features.  At least, nothing happens when I click "uninstall", but when I refresh the feature is no longer listed at all, and there's no list of purged items either.

Comment 7

9 years ago
OK, I just tried with a fresh Jetpack annotations DB.  The problem starts when you uninstall a feature.  You get the error above.

When about:jetpack is reloaded, the annotation for the feature still exists in the DB, so we call addLinkForJetpack() eventually with its feed, but the feed is not in JetpackRuntime.contexts, and you get the error again.  (I forgot to mention in my previous comment that I first got the error on uninstall.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 8

9 years ago
(In reply to comment #2)
> >@@ -227,17 +234,20 @@ var App = {
> >+    var context = JetpackRuntime.getJetpack(url);
> >+    var enabled = context && context.manifest && context.manifest.settings;
> >+    this._addButton(div, "open-settings", "settings", enabled);
> Is it possible for context to be not null?

If I change

    this._addButton(div, "open-settings", "settings",
                    !context.manifest.settings);

to

    this._addButton(div, "open-settings", "settings",
                    !context || !context.manifest.settings);

things seem to work again.

Comment 9

9 years ago
Aw. :( !(context && context.manifest.settings)

When is this item without a context showing up exactly? Are we trying to show it or is something processing it without showing?
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> Aw. :( !(context && context.manifest.settings)

I just checked this in as a bustage fix.


> When is this item without a context showing up exactly? Are we trying to show
> it or is something processing it without showing?

I just remembered that it's the "recently uninstalled" features that don't have a context.  Disabling the button is a bit wonky for those, since it implies that they too don't have settings, whereas the reality is that they may have them, but a technical limitation prevents us from allowing users to modify their settings.  Hopefully this'll get addressed by the redesign of this interface.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.