547189 - element.addEventListener("command") handlers only work if the element already has an inline oncommand attribute

Status: RESOLVED DUPLICATE of bug 371900

(Core :: XUL, defect)

Description

[Tim Anderson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6) Gecko/20100214 Gentoo IceCat/3.5.6 (like Firefox/3.5.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100213 Minefield/3.7a2pre

a <key> element will only allow an event listener to be added to it if it already has the oncommand attribute set.

Reproducible: Always

Steps to Reproduce:
The following code should demonstrate the problem
<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window id="XULBug" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<label value="Press a to see a popup"/>
<keyset>
<key id="key1" key="a"/>
</keyset>
<script>
<![CDATA[
document.getElementById("key1").addEventListener('command',function(e){alert("Hello")},false)
]]>
</script>
</window>
Actual Results:  
nothing

Expected Results:  
When the 'a' key is pressed, an alert with the text "hello" should come up.

The bug can be worked around by adding an oncommand="void(0)" to the key tag in the XUL.

Comment 1

[Brian Grinstead [:bgrins]]

I've also seen this issue with command elements.  A command with an event listener added to the "command" event will fire the callback when the command is triggered.  Given this markup:

    <commandset>
      <command id="nodeSearchCommand"
        oncommand="console.log('1');"/>
    </commandset>

    <keyset>
      <key key="a" command="nodeSearchCommand"/>
    </keyset>

    <script type="application/javascript">
      document.querySelector("#nodeSearchCommand").addEventListener("command", () => {
        console.log("2");
      }, false);
    </script>

After pressing 'a', I see "1" and "2" logged, as expected.

However, if the markup is changed to remove the oncommand="" handler in the markup, like so:

    <commandset>
      <command id="nodeSearchCommand"/>
    </commandset>

    <keyset>
      <key key="a" command="nodeSearchCommand"/>
    </keyset>

    <script type="application/javascript">
      document.querySelector("#nodeSearchCommand").addEventListener("command", () => {
        console.log("2");
      }, false);
    </script>

After pressing 'a', I don't see anything logged.  I expect to see "2", since the callback is listening to the command event.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

CCing Neil who has been very helpful with XUL bugs in the past.

Comment 3

[Neil Deakin]

Attachment: xbloncommand

We could try removing the filter here, although maybe there's some side effects?

Comment 4

[[e-mail]]

Comment on attachment 8382236 [details] [diff] [review]
xbloncommand

This is necessary to stop the dummy <key> elements that the <menuitem> elements in the Edit menu get their accelerator text from from preventing the default behaviour for their keystrokes which is actually implemented by the platform HTML binding code (also via nsXBLWindowKeyHandler, confusingly).

I suggested adding disabled="true" to all the relevant elements but bryner gave me an r- at the time.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to [e-mail] from comment #4)
> Comment on attachment 8382236 [details] [diff] [review]
> xbloncommand
> 
> This is necessary to stop the dummy <key> elements that the <menuitem>
> elements in the Edit menu get their accelerator text from from preventing
> the default behaviour for their keystrokes which is actually implemented by
> the platform HTML binding code (also via nsXBLWindowKeyHandler, confusingly).
> 
> I suggested adding disabled="true" to all the relevant elements but bryner
> gave me an r- at the time.

We need a way to have a command fire without the inline handler in order to secure internal pages with Content Security Policy (see Bug 923920 for about: pages and Bug 960728 for DevTools pages).  This process is further explained here https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles.

Even adding a oncommand=";", which works around this particular issue prevents the CSP from being applied: https://bugzilla.mozilla.org/show_bug.cgi?id=961524#c30.

Any ideas about what could we do to work around this?  Adding disabled="true" on the few elements that want this undocumented behavior seems reasonable, though there could be addons or other content relying on the behavior.  Do you know the Bug # where you originally suggested this?

Comment 6

[[e-mail]]

(In reply to Brian Grinstead from comment #5)
> (In reply to comment #4)
> > This is necessary to stop the dummy <key> elements that the <menuitem>
> > elements in the Edit menu get their accelerator text from from preventing
> > the default behaviour for their keystrokes which is actually implemented by
> > the platform HTML binding code (also via nsXBLWindowKeyHandler, confusingly).
> > 
> > I suggested adding disabled="true" to all the relevant elements but bryner
> > gave me an r- at the time.
> 
> We need a way to have a command fire without the inline handler in order to
> secure internal pages with Content Security Policy (see Bug 923920 for
> about: pages and Bug 960728 for DevTools pages).  This process is further
> explained here https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles.
> 
> Even adding a oncommand=";", which works around this particular issue
> prevents the CSP from being applied:
> https://bugzilla.mozilla.org/show_bug.cgi?id=961524#c30.
> 
> Any ideas about what could we do to work around this?  Adding
> disabled="true" on the few elements that want this undocumented behavior
> seems reasonable, though there could be addons or other content relying on
> the behavior.  Do you know the Bug # where you originally suggested this?

Bug 336740 comment #40.

It turns out that this bug is a duplicate of bug 371900 which already has a suggestion, which is to only require the <key> itself to have either an oncommand or a command attribute, as opposed to the current code which also requires the command element to have an oncommand attribute.

Attributes:		Behaviour:
command	oncommand	current	suggested
no	no		no	no
no	yes		yes	yes
yes	no*		no	YES
yes	yes*		yes	yes
*on command element

The dummy keys e.g. <key id="key_copy" key="C" modifiers="accel"/> fall under row 1, so that's OK.
Existing users presumably fall under rows 2 or 4, so they're OK.
Row 3 doesn't make much sense with the current code, so I doubt it gets used. The suggestion from bug 371900 would therefore allow you to write <key id="focusSearch" key="K" modifiers="accel" command="cmd_focusSearch"/> <command id="focusSearch"/> and use an event listener on the command element.

Note that the test case in bug 371900 is broken because it tries to declare a top-level onload function which apparently doesn't work any more.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to [e-mail] from comment #6)
> (In reply to Brian Grinstead from comment #5)
> > (In reply to comment #4)
> > > This is necessary to stop the dummy <key> elements that the <menuitem>
> > > elements in the Edit menu get their accelerator text from from preventing
> > > the default behaviour for their keystrokes which is actually implemented by
> > > the platform HTML binding code (also via nsXBLWindowKeyHandler, confusingly).
> > > 
> > > I suggested adding disabled="true" to all the relevant elements but bryner
> > > gave me an r- at the time.
> > 
> > We need a way to have a command fire without the inline handler in order to
> > secure internal pages with Content Security Policy (see Bug 923920 for
> > about: pages and Bug 960728 for DevTools pages).  This process is further
> > explained here https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles.
> > 
> > Even adding a oncommand=";", which works around this particular issue
> > prevents the CSP from being applied:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=961524#c30.
> > 
> > Any ideas about what could we do to work around this?  Adding
> > disabled="true" on the few elements that want this undocumented behavior
> > seems reasonable, though there could be addons or other content relying on
> > the behavior.  Do you know the Bug # where you originally suggested this?
> 
> Bug 336740 comment #40.
> 
> It turns out that this bug is a duplicate of bug 371900 which already has a
> suggestion, which is to only require the <key> itself to have either an
> oncommand or a command attribute, as opposed to the current code which also
> requires the command element to have an oncommand attribute.
> 
> Attributes:		Behaviour:
> command	oncommand	current	suggested
> no	no		no	no
> no	yes		yes	yes
> yes	no*		no	YES
> yes	yes*		yes	yes
> *on command element
> 
> The dummy keys e.g. <key id="key_copy" key="C" modifiers="accel"/> fall
> under row 1, so that's OK.
> Existing users presumably fall under rows 2 or 4, so they're OK.
> Row 3 doesn't make much sense with the current code, so I doubt it gets
> used. The suggestion from bug 371900 would therefore allow you to write <key
> id="focusSearch" key="K" modifiers="accel" command="cmd_focusSearch"/>
> <command id="focusSearch"/> and use an event listener on the command element.
> 
> Note that the test case in bug 371900 is broken because it tries to declare
> a top-level onload function which apparently doesn't work any more.
> 
> *** This bug has been marked as a duplicate of bug 371900 ***

OK great, this should cover our use case.  I will follow Bug 371900.