Closed Bug 1010616 Opened 10 years ago Closed 10 years ago

PredicateContext does not set linkURL of parentNodes

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: spammail495, Assigned: spammail495, Mentored)

References

Details

(Whiteboard: )

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
zombie
: review+
Details | Review
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;
  }
Would you be willing to take your untested implementation and turn it into a patch?
Flags: needinfo?(spammail495)
Priority: -- → P2
Flags: needinfo?(dtownsend+bugmail)
i can handle this one..
Flags: needinfo?(dtownsend+bugmail)
Whiteboard: [mentor=tomica+amo@gmail.com]
@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?
Flags: needinfo?(spammail495)
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?
Assignee: nobody → spammail495
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached file Link to pull request (obsolete) —
Attachment #8424272 - Flags: review?(tomica+amo)
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.
Status: NEW → ASSIGNED
Attachment #8424272 - Flags: review?(tomica+amo) → review+
Mentor: tomica+amo
Whiteboard: [mentor=tomica+amo@gmail.com] →
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.
Attachment #8424272 - Attachment is obsolete: true
Attachment #8443754 - Flags: review?(tomica+amo)
(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
Attachment #8443754 - Flags: review?(tomica+amo)
Attachment #8443754 - Flags: review+
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
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.