1274809 - Some properties are missing from contextMenus.OnClickData

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[Will Bamberg [:wbamberg]]

Attachment: 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.

Comment 1

[Tomislav Jovanovic :zombie]

> "checked" is never present

We have tests for "checked", so this is odd -- I'll investigate.

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: bug 1274809 - Add missing properties to OnClicked data,

Review commit: https://reviewboard.mozilla.org/r/79902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79902/

Comment 3

[Tomislav Jovanovic :zombie]

> We have tests for "checked", so this is odd -- I'll investigate.

this was wrong, but now we do.

Comment 4

[Andrew Swan [:aswan]]

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.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8793122 [details]
bug 1274809 - Add missing properties to OnClicked data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79902/diff/1-2/

Comment 6

[Tomislav Jovanovic :zombie]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17ba1f41eb5f
Add missing properties to OnClicked data, r=aswan

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17ba1f41eb5f

Comment 9

[Vasilica Tamas, QA [:vtamas]]

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.