contextMenus.OnClickData should have the frameId (can be used with tabs.sendMessage)

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P5
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: kernp25, Assigned: kernp25, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [contextMenus], triaged)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170214030231

Steps to reproduce:

Chrome also has frameId in info
https://developer.chrome.com/extensions/contextMenus#event-onClicked

Updated

3 months ago
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: [contextMenus], triaged
any work been done on this? can I start on it?
Flags: needinfo?(kernp25)
(Assignee)

Comment 2

2 months ago
Created attachment 8847177 [details] [diff] [review]
Bug 1339416 - contextMenus.OnClickData should have the frameId (can be used with tabs.sendMessage)
Flags: needinfo?(kernp25)
Attachment #8847177 - Flags: feedback?(hrishikeshbman)
(Assignee)

Comment 3

2 months ago
(In reply to hrishikeshbman from comment #1)
> any work been done on this?
Yes! See the patch.

> can I start on it?
Yes!
(Assignee)

Comment 4

2 months ago
Created attachment 8850063 [details] [diff] [review]
bug-1339416-fix.patch
Attachment #8847177 - Attachment is obsolete: true
Attachment #8847177 - Flags: feedback?(hrishikeshbman)
Attachment #8850063 - Flags: review?(mixedpuppy)
Comment on attachment 8850063 [details] [diff] [review]
bug-1339416-fix.patch

This looks fine, I'd like to see a test along with this.  Maybe add something in browser_ext_contextMenus_onclick.js.  There is a frame now in the context.html test file that could be used as part of that.

Also, if the context menu is on chrome, what does frameId end up as?  I'm expecting it should be -1.  You could probably add a simple check for that in some test in browser_ext_contextMenus_chrome.js
Attachment #8850063 - Flags: review?(mixedpuppy) → feedback+
(Assignee)

Comment 6

2 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Comment on attachment 8850063 [details] [diff] [review]
> bug-1339416-fix.patch
> 
> This looks fine, I'd like to see a test along with this.  Maybe add
> something in browser_ext_contextMenus_onclick.js.  There is a frame now in
> the context.html test file that could be used as part of that.
> 
> Also, if the context menu is on chrome, what does frameId end up as?  I'm
> expecting it should be -1.  You could probably add a simple check for that
> in some test in browser_ext_contextMenus_chrome.js

How to write and run those tests?
(Assignee)

Comment 7

2 months ago
Created attachment 8851064 [details] [diff] [review]
bug-1339416-fix-v2.patch
Attachment #8850063 - Attachment is obsolete: true
Attachment #8851064 - Flags: review?(kmaglione+bmo)
Comment on attachment 8851064 [details] [diff] [review]
bug-1339416-fix-v2.patch

Still needs tests I requested
Attachment #8851064 - Flags: review?(kmaglione+bmo) → review-
(Assignee)

Comment 9

2 months ago
Created attachment 8851341 [details] [diff] [review]
bug-1339416-fix-v3.patch
Attachment #8851341 - Flags: feedback?(scaraveo)
(Assignee)

Comment 10

2 months ago
113 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_contextMenus_chrome.js | click in frame - Got 4294967304, expected 1

Can you tell me what's the problem?
(Assignee)

Updated

2 months ago
Attachment #8851341 - Flags: feedback?(scaraveo) → feedback?(mixedpuppy)
Mentor: mixedpuppy@gmail.com
kern25: fyi we're in a work week, I've looked at this and reproduced the problem but haven't looked at why yet.  My responses will be slow this week.
(In reply to kernp25 from comment #10)
> 113 INFO TEST-UNEXPECTED-FAIL |
> browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_contextMenus_chrome.js | click in frame - Got 4294967304,
> expected 1
> 
> Can you tell me what's the problem?

Ok, looking into the code, we don't handle frameId like we do for tabs and windows.  frameId is actually the value of outerWindowId unless it is the top frame (it is zero).  Change the test to something like this:

isnot(info.frameId, undefined, "frame click, frameId is not undefined");
isnot(info.frameId, 0, "frame click, frameId probably okay");

The patch is a little bit rotted due to changes yesterday, so you'll need to hg pull.
(Assignee)

Comment 13

2 months ago
Created attachment 8854375 [details] [diff] [review]
bug-1339416-fix-v4.patch
Attachment #8851064 - Attachment is obsolete: true
Attachment #8851341 - Attachment is obsolete: true
Attachment #8851341 - Flags: feedback?(mixedpuppy)
Attachment #8854375 - Flags: review?(mixedpuppy)
Comment on attachment 8854375 [details] [diff] [review]
bug-1339416-fix-v4.patch

I'd like to run this through tryserver before landing, if you don't have access to push to that, let me know I'll do it.
Flags: needinfo?(kernp25)
Attachment #8854375 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → kernp25
(Assignee)

Comment 15

2 months ago
I don't have access to the tryserver :/
Flags: needinfo?(kernp25)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e275ab87d0bada5fd2c05bfc31ea3c7ce678a44
Created attachment 8855837 [details] [diff] [review]
bug-1339416-fix-v4.patch

Just updating the commit message in the patch.  This is ready to go.
Attachment #8854375 - Attachment is obsolete: true
Attachment #8855837 - Flags: review+
Keywords: checkin-needed
kernp25: (no idea what your name is) on it's way to landing \o/  thank you!

Comment 19

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebebeede8340
contextMenus.OnClickData should have the frameId. r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ebebeede8340
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.