Make menulists usable and useful in addon manager inline settings

VERIFIED FIXED in mozilla7

Status

()

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({dev-doc-complete})

unspecified
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

8 years ago
In the addon manager (as of bug 653637) we load a file using XMLHttpRequest and copy nodes into the main document. Menulist nodes throw this error:

Error: val.setAttribute is not a function
Source File: chrome://global/content/bindings/menulist.xml
Line: 224

I think it's because in the source document the menulist doesn't have any binding applied to it, and it gains one in the destination document.
Assignee

Comment 1

8 years ago
Further discoveries:

1. The binding is applied if there's no whitespace between the <menulist> and <menupopup> tags. (This can be fixed by removing the includes attribute from the children node of the binding content, which isn't ideal but it does work.)

2. menulist.xml should be updated to use firstElementChild and friends.

Fixing both of these would solve the problem I'm having, so I'll probably do that and leave the underlying cause unresolved.
Assignee

Updated

8 years ago
Blocks: 653637
Assignee

Comment 2

8 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee

Updated

8 years ago
Summary: Menulist binding doesn't apply correctly when moved from one document to another → Make menulists usable and useful in addon manager inline settings
Assignee

Updated

8 years ago
Depends on: 661494
Assignee

Comment 3

8 years ago
Posted patch patch (obsolete) — Splinter Review
I've asked fligtar to find out roughly how many addons use the AddonOptionsLoad event in mobile (http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/extensions.xml#136). Depending on the answer I'll either replace it, or add the observer service notification there too.
Assignee: nobody → geoff
Attachment #535984 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #537307 - Flags: feedback?(dtownsend)
Comment on attachment 537307 [details] [diff] [review]
patch

>-      var desc = setting.textContent.trim();
Did you mean to lose the trim()? I don't see a replacement for it.
Assignee

Comment 5

8 years ago
(In reply to comment #4)
> Comment on attachment 537307 [details] [diff] [review] [review]
> patch
> 
> >-      var desc = setting.textContent.trim();
> Did you mean to lose the trim()? I don't see a replacement for it.

No, I didn't. I suppose the desc attribute should also be trimmed, since there's no point in creating an empty label.
Assignee

Comment 6

8 years ago
Posted patch patch (obsolete) — Splinter Review
I've decided we should keep the event (on mobile) and fire a notification as well.
Attachment #537307 - Attachment is obsolete: true
Attachment #537307 - Flags: feedback?(dtownsend)
Attachment #537702 - Flags: review?(dtownsend)
Comment on attachment 537702 [details] [diff] [review]
patch

Review of attachment 537702 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2834,5 @@
> +          text += stripTextNodes(aNode.childNodes[i]);
> +        }
> +      }
> +      return text;
> +    }

What if you only remove whitespace text nodes? Does that solve the problem equally well?

@@ +2870,5 @@
>          rows.appendChild(row);
>        }
>      }
> +
> +    Services.obs.notifyObservers(document, "AddonOptionsLoad", this._addon.id);

Why an observer notification rather than just a DOM event like on mobile?
Assignee

Comment 8

8 years ago
(In reply to comment #7)
> What if you only remove whitespace text nodes? Does that solve the problem
> equally well?

It should (non-whitespace text nodes inside the menulist make no sense anyway), but I need to capture the description text and make sure it doesn't get displayed in the wrong place, so I'm doing that here too.

> Why an observer notification rather than just a DOM event like on mobile?

Well I could be wrong, but isn't listening for an event a bit of a hassle when the addon manager could be in any tab in any window?
(In reply to comment #8)
> (In reply to comment #7)
> > What if you only remove whitespace text nodes? Does that solve the problem
> > equally well?
> 
> It should (non-whitespace text nodes inside the menulist make no sense
> anyway), but I need to capture the description text and make sure it doesn't
> get displayed in the wrong place, so I'm doing that here too.

I wonder if we're going to need to do this for other elements where we want to keep real text nodes in the future but I guess we can cross that bridge when we come to it.

> > Why an observer notification rather than just a DOM event like on mobile?
> 
> Well I could be wrong, but isn't listening for an event a bit of a hassle
> when the addon manager could be in any tab in any window?

A little more of a hassle but there is something to be said for keeping events properly scoped. I guess having both gives devs both options though. I'd prefer if you used "addon-options-displayed" for the observer notification though. Not only is it more consistent with other observer notifications but it can be confusing to have two different events with the same name.
Comment on attachment 537702 [details] [diff] [review]
patch

Review of attachment 537702 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took me a while to get through these
Attachment #537702 - Flags: review?(dtownsend) → review+
Assignee

Comment 11

8 years ago
Posted patch patchSplinter Review
Attachment #537702 - Attachment is obsolete: true
Assignee

Comment 12

8 years ago
I'll update the docs once this has landed. The relevant tests passed on try although I did strike bug 661784.
http://hg.mozilla.org/mozilla-central/rev/bfcc94f3fd0a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Patch looks ok and tests are passing. Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110703 Firefox/7.0a1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.