1010616 - PredicateContext does not set linkURL of parentNodes

Status: RESOLVED FIXED

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

Description

[Ben]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140508121358

Steps to reproduce:

Create a context menu with a PredicateContext that shows the linkURL when clicked on:

  var cm = require("sdk/context-menu");
  linkURLtest = cm.Item({
    label: "linkURL test",
    data: 'unset',
    context: cm.PredicateContext(function(data) {
        linkURLtest.data = data.linkURL;
        return true;
      }),
     contentScript: ('self.on("click", function(node, data) { alert("data: " + data); });')
  });


Actual results:

On a Node like:

  <a href="http://mylink.com">Link</a>

this works fine.

On a 'img'-node surrounded by an 'a'-node:

  <a href="http://myLink.com"><img src="myImage.png"/></a>

this does not work.


Expected results:

The PredicateContext should check parentNodes for an 'href'-attribute unitil one is found. A (untested) implementation would be to replace (lib/sdk/context-menu.js#L262):

  data.linkURL = node.href || null;

with:

  let currentNode = node;
  while (!data.linkURL && !(currentNode instanceof Ci.nsIDOMDocument)) {
    data.linkURL = currentNode.href || null;
    currentNode = currentNode.parentNode;
  }

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Would you be willing to take your untested implementation and turn it into a patch?

Comment 2

[Tomislav Jovanovic :zombie]

i can handle this one..

Comment 3

[Ben]

@Wes: Of course I could turn it into a patch. But I do not know how to test/debug changes in the sdk. I have just started to develop addons for mozilla.

@Tomislav: You are free to take my implementation and turn it into a patch and test it if you want to. Or what do you mean with handle?

Comment 4

[Tomislav Jovanovic :zombie]

hey Ben, sorry that wasn't clear, i was speaking to Dave there.

i meant i can handle the mentoring (if needed) and review on this one. here is a starting point:

https://github.com/mozilla/addon-sdk/wiki/contribute

in short: you should check out the master branch from github, make your changes, add some unit tests, test with Nightly version of firefox, open a Pull Request on github, add a link to it as an attachment to this bug, and flag me for review?

Comment 5

[Tomislav Jovanovic :zombie]

and forgot to say, feel free to ask if something is not clear or you get stuck. and if you prefer a more immediate way of interaction, join us in #jetpack channel on irc.mozilla.org.

Comment 6

[Ben]

Attachment: Link to pull request

Comment 7

[Tomislav Jovanovic :zombie]

good work Ben, thanks!

i left a few smaller nits in the PR, so if you could address them, and then squash your commits into one (using git rebase), that would be great.

just post a comment here when you are done, so i can merge this.

Comment 8

[Ben]

Attachment: Link to pull request of revised patch

I am not that familiar with git, so I got really confused about the rebase thing and didn't get it to work. That's why I created a new pull request with the revised patch. Hope that's all right.

Comment 9

[Tomislav Jovanovic :zombie]

(In reply to Ben from comment #8)
> I am not that familiar with git, so I got really confused about the rebase
> thing and didn't get it to work. That's why I created a new pull request
> with the revised patch. Hope that's all right.

that's ok, but now there is another thing (typo?) you should fix, so you might as well rebase. for example, if there are two commits in your branch, you can try with this:

  git rebase -i HEAD~2

which will open the two commits in an editor. you need to edit the first word on the second commit from "pick" to "f", and then do:

  git -f push


you can check out this page for more info, or again, feel free to ask.

http://git-scm.com/book/en/Git-Tools-Rewriting-History

Comment 10

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d4ed66f92a6fe765912c0bf7897be7491dc6bd7a
Bug 1010616 - PredicateContext does not set linkURL of parentNodes

https://github.com/mozilla/addon-sdk/commit/c63c6c1f345c9fb6b367479d2c7bed0d28ca7ef8
Merge pull request #1523 from spammail495/master

Bug 1010616 - PredicateContext does not set linkURL of parentNodes, r=@zombie

Comment 11

[Tomislav Jovanovic :zombie]

thank you Ben for this, nice work!

if you are interested in contributing some more, here is a list of good first bugs:

https://bugzilla.mozilla.org/buglist.cgi?status_whiteboard=[good+first+bug]&product=Add-on+SDK