561547 - Node passed to context menu item callbacks may not match given selector

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: patch

Myk, I'm requesting 0.3 approval for this.

Erik pointed this out today in IRC.  If you bind an item to a selector context, that item appears when the user clicks a node that either matches that selector or has an ancestor that matches.  However, the node passed to onClick is always the clicked node -- which might not match the selector.

For example, with this document and Item:

  <a><span>foo</span></a>
  Item({ context: "a", onClick: function (contextObj) {} });

contextObj.node is the span, not the anchor.  But since Erik specified the anchor, he was expecting to get the anchor, and I think that's reasonable.

Two ways to fix this inconsistency: for selector contexts 1) don't match ancestors, or 2) contextObj.node should be the node that matched.  Matching ancestors is useful, so this patch does 2.

Myk, do you agree with all that?  If so, I'd like to take this for 0.3, since context-menu hasn't been released yet and this changes its API subtly.  It's hard to claim an API change however subtle is low risk but here goes:  The module has lots of test coverage, I updated the test for this case, and the changes are fairly small.  Also, giving ourselves a brief opportunity to respond to feedback on new APIs is IMO one of the purposes of having release candidates.  Understand if you disagree though.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 441270 [details] [diff] [review]
patch

(In reply to comment #0)
> Myk, do you agree with all that?

Yes, that all makes perfect sense.  And the code looks good, r=myk.


> If so, I'd like to take this for 0.3, since
> context-menu hasn't been released yet and this changes its API subtly.  It's
> hard to claim an API change however subtle is low risk but here goes:  The
> module has lots of test coverage, I updated the test for this case, and the
> changes are fairly small.  Also, giving ourselves a brief opportunity to
> respond to feedback on new APIs is IMO one of the purposes of having release
> candidates.  Understand if you disagree though.

Indeed, it's a bit riskier than I would normally want to take at this stage in the cycle.  But API changes are good ones, it's easier to change the API now than later, and the code looks good and seems well-covered by its unit tests, so a=myk.

Comment 2

[Drew Willcoxon :adw]

Thanks Erik.  Fixed with this changeset:

http://hg.mozilla.org/labs/jetpack-sdk/rev/48e4e539e6ad

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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".