Closed Bug 1290965 Opened 8 years ago Closed 8 years ago

XUL elements fire command event twice via DOM on click()

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

Details

Attachments

(1 file)

Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.
58 bytes, text/x-review-board-request
enndeakin
: review+
Details 
It appears as if the command event fires twice when using element.click() in JavaScript on a toolbarbutton.

The following code creates the new event:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#362

 let cmdEvent = document.createEvent("xulcommandevent");

However this only applies on the user click once but the click() happens twice.
Adding debug into the oncommand on the elements causes the output etc.

Changing #back-button https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#671: 
-command="Browser:BackOrBackDuplicate"
+oncommand="alert(1)"

This could be related or a dupe of Bug 1257617
I don't see this in general for toolbarbuttons, but without a testcase, patch or steps to reproduce it is hard to tell. The code you linked to suggests a something specific to the back/forward button though.
Component: XUL → Toolbars and Customization
Product: Core → Firefox
I use the XUL preview editor from: https://addons.mozilla.org/en-US/firefox/addon/extension-developer/?src=collection&collection_id=402bc83a-e15f-75ff-bdfc-cbc731c58135


<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window id="yourwindow" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:html="http://www.w3.org/1999/xhtml" xmlns:h="http://www.w3.org/1999/xhtml">
<toolbarbutton oncommand="alert(1)" id="a">A</toolbarbutton>
<toolbarbutton oncommand="alert(2)" id="b">B</toolbarbutton>
<toolbarbutton id="c">Trigger B</toolbarbutton>
<script type="text/javascript">
<![CDATA[
 var bEl = document.getElementById('b');
var cEl = document.getElementById('c');
bEl.addEventListener('click', function (aEvent) {
      let cmdEvent = document.createEvent("xulcommandevent");
      cmdEvent.initCommandEvent("command", true, true, window, 0,
                                aEvent.ctrlKey, aEvent.altKey, aEvent.shiftKey,
                                aEvent.metaKey, null);
      aEvent.currentTarget.dispatchEvent(cmdEvent);
});
  cEl.addEventListener('click', function () {
    bEl.click();
  });
]]>
</script>
</window>


Clicking button A or B with the pointer gets 1 alert.
Clicking button C triggers the click event on the B element, however it also calls oncommand itself, this results in two alert(2) being fired.
Flags: needinfo?(enndeakin)
This behavior isn't specific to toolbarbutton either, changing those elements to <el> works just the same.
OK, so a toolbarbutton when clicked should already fire a command element itself, so writing code to fire another one is redundant which is why you get two alerts.

For other elements, this is because nsXULElement::ClickWithInputSource doesn't check what kind of element is being clicked. The disabled check is also not doing this.
Flags: needinfo?(enndeakin)
But .click() and a physical click should behave the same right or am I missing something?
Flags: needinfo?(enndeakin)
No, it just fires a click event at the element. Much of the behaviour that would happen when clicking the mouse is not performed. This is the same for HTMLElement.click()
Flags: needinfo?(enndeakin)
But in all cases click() fires the onclick and oncommand yet the physical when there is an onclick event only triggers onclick and not oncommand (without the retriggering code).

Is this a bug? Is there any way to make click() behave the same etc? Is there a different implementation that would work similar?
Flags: needinfo?(enndeakin)
Ideally, nsXULElement::click() shouldn't fire the command event at all, but we've had the current behaviour for a long time and there is likely code and/or addons relying on this.

There are bugs here but short of not firing the command event, I'm not sure what the right fix would be.

But if you explain what you're actually trying to accomplish in detail I can offer an alternative.
Flags: needinfo?(enndeakin)
I'm trying to add a drop down menu on hold to the add new tab button the same as how the back button is working.

I need a onclick, onmousedown and command attribute from what I can tell:
- onmousedown to trigger the menu to show after a period of time
- onclick because command doesn't seem to fire when the mousedown is applied
- command for keyboard
- The button has onmouseenter and others too but I don't think they play any part.

All this code works fine however the tests fail as it creates two tabs.

Could nsXULElement::click() check for the onclick event and prevent the command from firing instead like it seems the normal click does? In fact I don't actually care if we have to add another attribute either to prevent addon breakage etc. I know this would be a breaking change but it seems the back button tests work purely based on the event triggering a page change (so users don't go twice into history in the tests).
Flags: needinfo?(enndeakin)
Are you using SetClickAndHoldHandlers() from browser.js? It would be a good idea to just adjust that function? (I'm assuming you're writing code to be added to Firefox and not an addon) That code doesn't call click() at any time.

It sounds like you might be either calling click() directly in a test or using EventUtils::sendMouseEvent. You should use synthesizeMouseAtCenter instead which synthesizes a real mouse event.
Flags: needinfo?(enndeakin)
I modified that for the non anonymous add button and that was when I realised the code https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#372 adds click handlers and also triggers a command.

This test for example does call back-button.click() https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/puppeteer/test_toolbars.py#61 there are similar failures I get for click() of new-tab

This is the patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf880317fab0&selectedJob=24229401
https://hg.mozilla.org/try/rev/cf880317fab0acb57bd475c420018945b35b1ad8

So you think that tests should be changed? I'm not sure we have that ability in the ui tests.
Flags: needinfo?(enndeakin)
I'm not familiar with those tests so I don't know.
Flags: needinfo?(enndeakin)
Blocks: 1272256
Hi do either of you know if the behavior of DOM .click() could be changed to behave similar to the physical click?

The line here is what is causing the oncommand to fire:
https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp#1787

It seems like the physical click when there is an onclick on the element won't trigger this. Is there another way round this at all?
Flags: needinfo?(masayuki)
Flags: needinfo?(bobbyholley)
(In reply to Jonathan Kingston [:jkt] from comment #13)
> Hi do either of you know if the behavior of DOM .click() could be changed to
> behave similar to the physical click?

The spec is clear that the current behaviour for HTML elements is correct. Having it mimic a real click would open up potential spoofing and security issues.

If the issue is that the tests are using element.click() and expecting that it mimics a real click then the tests are wrong as element.click() does not mimic what would happen if the user clicks the element.

What we could do though is improve nsXULElement::ClickWithInputSource such that it doesn't call DoCommand() if the click event was cancelled, then the click event listener in comment 2 can just call preventDefault().
I don't know anything about this offhand.
Flags: needinfo?(bobbyholley)
Looks like Enn already answered it (although, I'm not familiar with that).
Flags: needinfo?(masayuki)
Summary: toolbarbutton fires command event twice via DOM → XUL elements fire command event twice via DOM on click()
Assignee: nobody → jkt
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

Hi Neil,

Is this like you were after?

Thanks
Attachment #8779367 - Flags: review?(enndeakin)
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

https://reviewboard.mozilla.org/r/70350/#review68022

::: dom/xul/nsXULElement.cpp:1783
(Diff revision 1)
>  
>              // send mouse click
>              status = nsEventStatus_eIgnore;  // reset status
>              EventDispatcher::Dispatch(static_cast<nsIContent*>(this),
>                                        context, &eventClick, nullptr, &status);
> +

Use can just compare 'status' here.

::: dom/xul/test/test_bug1290965.xul:10
(Diff revision 1)
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js" />
> +  <toolbarbutton oncommand="++counter;" id="a">B</toolbarbutton>
> +  <toolbarbutton oncommand="++counter;" id="b">B</toolbarbutton>
> +  <script type="text/javascript">
> +  <![CDATA[
> +    Components.utils.import("resource://testing-common/BrowserTestUtils.jsm");

It's a bit weird to use BrowserTestUtils in a non-browser test here especially as you don't save any typing by using it.

::: dom/xul/test/test_bug1290965.xul:12
(Diff revision 1)
> +  <toolbarbutton oncommand="++counter;" id="b">B</toolbarbutton>
> +  <script type="text/javascript">
> +  <![CDATA[
> +    Components.utils.import("resource://testing-common/BrowserTestUtils.jsm");
> +    SimpleTest.waitForExplicitFinish();
> +    SimpleTest.expectAssertions(0, 1);

What assertion are you expecting?
Attachment #8779367 - Flags: review?(enndeakin)
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

https://reviewboard.mozilla.org/r/70350/#review68022

> It's a bit weird to use BrowserTestUtils in a non-browser test here especially as you don't save any typing by using it.

There wasn't waitForEvent defined in this test, is there something else I should import instead?
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

Hi Neil,

See comment #22 for my update here.
Please let me know if there is another way to implement the waitForEvent.


Thanks
Jonathan
Attachment #8779367 - Flags: review?(enndeakin)
You can just use addEventListener as in the other code you have.
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

https://reviewboard.mozilla.org/r/70350/#review68742

::: dom/xul/test/test_bug1290965.xul:11
(Diff revision 4)
> +  <toolbarbutton oncommand="++counter;" id="a">A</toolbarbutton>
> +  <toolbarbutton oncommand="++counter;" id="b">B</toolbarbutton>
> +  <script type="text/javascript">
> +  <![CDATA[
> +    SimpleTest.waitForExplicitFinish();
> +    SimpleTest.expectAssertions(3);

Still not sure what assertions these are

::: dom/xul/test/test_bug1290965.xul:33
(Diff revision 4)
> +                                aEvent.ctrlKey, aEvent.altKey, aEvent.shiftKey,
> +                                aEvent.metaKey, null);
> +      aEvent.currentTarget.dispatchEvent(cmdEvent);
> +    });
> +
> +    aEl.addEventListener('command', function (aEvent) {

This doesn't actually wait to see if a second command event occurred. It would be better to just click on each button in sequence, keep a separate counter for each button and verify the counters at the end.
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

Comments addressed.
Thanks
Attachment #8779367 - Flags: review?(enndeakin)
Comment on attachment 8779367 [details]
Bug 1290965 - Prevent command from firing when click has prevented default on the XUL element.

https://reviewboard.mozilla.org/r/70350/#review69086
Attachment #8779367 - Flags: review?(enndeakin) → review+
Thanks Neil for checking this over.
Keywords: checkin-needed
That fail was removed after updating in the latest patch: https://reviewboard.mozilla.org/r/70348/diff/5#index_header

Would you like me to push to try again?

Thanks!
Flags: needinfo?(jkt) → needinfo?(ryanvm)
No, thanks for clarifying.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f92d65fdbee
Prevent command from firing when click has prevented default on the XUL element. r=enn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f92d65fdbee
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: