Closed Bug 1274809 Opened 3 years ago Closed 3 years ago

Some properties are missing from contextMenus.OnClickData

Categories

(WebExtensions :: Frontend, defect, P2)

defect

Tracking

(firefox52 verified)

VERIFIED FIXED
mozilla52
Tracking Status
firefox52 --- verified

People

(Reporter: wbamberg, Assigned: zombie)

Details

(Whiteboard: [contextMenu] triaged)

Attachments

(2 files)

Attached file context-menu.zip
I've attached a WebExtension that adds a few different context menu items, and logs some information from OnClickData when they are clicked.

It seems that a few are missing:

"editable" is only present when the element is editable. That is, it's never false.
"checked" is never present
"wasChecked" is never present

Especially given the last two, this seems to make it impossible to handle checkboxes and radio items.

Tested in Dev Edition and Nightly.
Priority: -- → P2
Whiteboard: [contextMenu] triaged
> "checked" is never present

We have tests for "checked", so this is odd -- I'll investigate.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Component: WebExtensions: Untriaged → WebExtensions: Frontend
> We have tests for "checked", so this is odd -- I'll investigate.

this was wrong, but now we do.
Comment on attachment 8793122 [details]
bug 1274809 - Add missing properties to OnClicked data,

https://reviewboard.mozilla.org/r/79902/#review79138

Nice, thanks for doing this!

::: browser/components/extensions/ext-contextMenus.js:421
(Diff revision 1)
>        menuItemId: this.id,
> +      editable: contextData.onEditableArea,
>      };
>  
>      function setIfDefined(argName, value) {
>        if (value) {

Your fix is fine but can you change this to check `value !== undefined` just to remove the trap for falsy values.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_checkboxes.js:62
(Diff revision 1)
>  
>      return extensionMenuRoot.getElementsByAttribute("type", "checkbox");
>    }
>  
> +  function confirmOnClickData(onClickData, id, was, checked) {
> +    is(onClickData.wasChecked, was, `checkbox item ${id} was ${was ? "" : "not"} checked before the click`);

nit: when `was` is true, your message has a double space between "was" and "checked".  same for the message below and the same code in the radio test.
Attachment #8793122 - Flags: review?(aswan) → review+
> > +    is(onClickData.wasChecked, was, `checkbox item ${id} was ${was ? "" : "not"} checked before
> 
> nit: when `was` is true, your message has a double space between "was" and "checked".

I thought "slightly easier to read code" > "test log message with double space"..
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/17ba1f41eb5f
Add missing properties to OnClicked data, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/17ba1f41eb5f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
I was able to reproduce the initial issue on Firefox 50.0b1 (20160920155715) under Windows 10 64-bit.

Verified fixed on Firefox 52.0a1 (2016-09-25) under Windows 10 64-bit. I confirm that "editable", "checked" and "wasChecked" are always displayed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.