688142 - Improve to check Action object in PopupNotifications.show.

Status: RESOLVED INVALID

(Toolkit :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110920 Firefox/9.0a1
Build ID: 20110920085517

Steps to reproduce:

PopupNotifications.show() method's 3rd parameter is JavaScript object literal to describe notification action. And its show() method checks notification action object.
But I seem that it checking process is not perfect.


Actual results:

If I set empty string to notification action object's lablel/accessKey, it will occur an error and stop PopupNotifications.show().

For example:
PopupNotifications.show(
  gBrowser.selectedBrowser,
  "test",
  "test",
  null,
  { label:"", accessKey:"", callback:function(){} },
  null,
  null
);


Expected results:

I think it should be possible to pass empty strings to them.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Amendment of fix isInvalidAction

I attach the amendment to fix isInvalidAction() inside PopupNotifications_show().

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Please review my patch.

Comment 4

[Josh Matthews [:jdm]]

Saneyuki, I think typeof(foo) != bar checks are better than !(typeof(foo) == bar). Would you mind making that change?

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Josh Matthews [:jdm] from comment #4)
> Saneyuki, I think typeof(foo) != bar checks are better than !(typeof(foo) ==
> bar). Would you mind making that change?

When I wrote this patch, I thought that I should follow the original code and should modify to the minimum.

However I agree your suggestion is more simple.
I mind to modify this patch as below:

- return !a || !(typeof(a.callback) == "function") || !a.label || !a.accessKey;
+ return !a || typeof(a.callback) != "function" || 
+        typeof(a.label) != "string" || typeof(a.accessKey) != "string";

Is this change all right?

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 592412 [details] [diff] [review]
proposed patch

Let's see what the code owners say.

Comment 7

[Dão Gottwald [:dao]]

(In reply to OHZEKI Tetsuharu (a.k.a saneyuki_s) from comment #5)
> However I agree your suggestion is more simple.
> I mind to modify this patch as below:
> 
> - return !a || !(typeof(a.callback) == "function") || !a.label ||
> !a.accessKey;
> + return !a || typeof(a.callback) != "function" || 
> +        typeof(a.label) != "string" || typeof(a.accessKey) != "string";
> 
> Is this change all right?

Yes, !(a == b) doesn't make sense. Another nit: 'typeof foo' instead of 'typeof(foo)'. typeof is not a function.

(In reply to OHZEKI Tetsuharu (a.k.a saneyuki_s) from comment #0)
> I think it should be possible to pass empty strings to them.

Why would you want to pass an empty string?

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Dão Gottwald [:dao] from comment #7) 
> Yes, !(a == b) doesn't make sense. Another nit: 'typeof foo' instead of
> 'typeof(foo)'. typeof is not a function.

Thank you. I rewrite the patch.

> Why would you want to pass an empty string?

In some case of showing popup notification from add-ons, I (an add-on developer) want to set no accesskey because the set accesskey conflicts with other one. (e.g. Firefox default keyset)
No setting accesskey is bad on usability. But conflict of accesskeys is worse than it.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch v1.1

I rewrote the patch with new style.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to OHZEKI Tetsuharu (a.k.a saneyuki_s) from comment #8)

> > Why would you want to pass an empty string?
> 
> In some case of showing popup notification from add-ons, I (an add-on
> developer) want to set no accesskey because the set accesskey conflicts with
> other one. (e.g. Firefox default keyset)
> No setting accesskey is bad on usability. But conflict of accesskeys is
> worse than it.

How do they conflict?

The accesskey for an action is only in effect when the popup is showing, in which it is correctly overriding any existing accesskey. (And when the popup is hidden, the access key performs the expected action).

E.G.: I changed the accesskey for the geolocation popup to "f", to conflict with the accesskey for the File menu. When the prompt is shown, alt-f focuses the button (as alt-a would normally do, not sure why it's just a focus and not command, but anyway). When the prompt is hidden (such as after clicking on the page instead of a button on the prompt), alt-f again brings up the File menu.

This seems expected to me?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

A key is handled only once as accesskey.

Firstly, focused ESM looks for a content which has the pressed acesskey from current focused element. If it reaches to the end of the content, it goes back to the start of the document and looks for the target until the focused content. Secondary, ESM looks for a target in its subdocument. Finally, ESM looks for the target in the chrome if the ESM is content.

If ESM finds a target, it executes the content and quits from the accesskey handling.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Thanks to explain accesskey.
I misunderstood the behavior of accesskey.

However the current implementation is bad because it can pass not only string but also function, array, or other type JS object.
isInvalidAction() should check strictly the passed Notification Action object.

Comment 13

[Justin Dolske [:Dolske]]

I'm going to close this as invalid, since previous comments indicate that allowing an empty access key isn't something that's actually a problem or needed.

The last part of comment 12 should be handled as a separate bug, since it's a separate issue. (Bugs are cheap, multiple issues in a bug makes things confusing) However, the issue you describe is pretty normal, we often don't do strict argument type checking in JS. It's the programmer's responsibility to use APIs correctly. [There are exceptions, usually for things like guarding against common mistakes, checking for null when it's unclear if it's allowed or not, etc. I don't think any of these kinds of situations apply here, but am open to the possibility. :)]