Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#15097 closed defect (fixed)

Button: Cannot stop click event from bubbling

Reported by: Paul Christopher Owned by: Douglas Hays
Priority: undecided Milestone: 1.9
Component: Dijit - Form Version: 1.7.2
Keywords: Cc: Kitson Kelly, ben hockey
Blocked By: #16344 Blocking:

Description

Description

Calling stopPropagation e.g. in a (dijit) click handler should stop the propagation of a click event for regular (dom) click handlers, too.

Test Case

Have a look at the attached test case. It is a simple div containing a dijit button. You can select the div by clicking on it, but clicking the button should perform some action and not select the div. Therefore I tried to call stopPropagation in the button's click handler - without any effect.

Workaround

The workaround is to set up a separate dom-click-handler on the button's domNode and call stopPropagation there.

Discussion

This is not a real bug report nor a real feature request, but more a real life example(?? or a newbie's mistake???). I just place it here so as to make sure that this cross-dom-dijit-event case is considered in system design. Myself, I'm not absolutely sure, whether it is a good idea to cancel all dom-clicks when a dijit-click with stopPropagation is called. But calling both events the same ('click') could be sometimes confusing, too.

Attachments (3)

testStopPropagation.html (1.4 KB) - added by Paul Christopher 7 years ago.
15097.patch (2.5 KB) - added by Douglas Hays 7 years ago.
possible fix
15097.2.patch (15.2 KB) - added by Douglas Hays 7 years ago.
refreshed fix

Download all attachments as: .zip

Change History (32)

Changed 7 years ago by Paul Christopher

Attachment: testStopPropagation.html added

comment:1 Changed 7 years ago by bill

#15099 is a duplicate of this ticket.

comment:2 Changed 7 years ago by bill

Cc: Kitson Kelly added
Component: GeneralDijit
Milestone: tbd2.0
Owner: set to bill
Status: newassigned

I guess agree that your suggested behavior would be nicer than the current behavior, and I think it will be fixed in 2.0 when all widget event connections just get delegated to be connections events on the DOM. I don't see a practical way to fix it before then.

I'm referring to removing the "backwards-compatibility" section in the current definition of _WidgetBase.on():

on: function(/*String*/ type, /*Function*/ func){
        // summary:
        //              Call specified function when event occurs, ex: myWidget.on("click", function(){ ... }).
        // description:
        //              Call specified function when event `type` occurs, ex: `myWidget.on("click", function(){ ... })`.
        //              Note that the function is not run in any particular scope, so if (for example) you want it to run in the
        //              widget's scope you must do `myWidget.on("click", lang.hitch(myWidget, func))`.

        // For backwards compatibility, if there's an onType() method in the widget then connect to that.
        // Remove in 2.0.
        var widgetMethod = this._onMap(type);
        if(widgetMethod){
                return aspect.after(this, widgetMethod, func, true);
        }

        // Otherwise, just listen for the event on this.domNode.
        return this._adoptHandles(on(this.domNode, type, func))[0];
}

comment:3 Changed 7 years ago by Paul Christopher

This ticket might be related to http://bugs.dojotoolkit.org/ticket/14734, which aims at providing a a11y click for DOM elements. Strictly speaking: A simple DOM click is not really what I want for the div. The div should be selectable by keyboard, too (provided that it has the appropriate tabindex attribute).

comment:4 Changed 7 years ago by bill

Component: DijitDijit - Form
Milestone: 2.0tbd
Owner: changed from bill to Douglas Hays
Summary: Cannot stop click event from bubblingButton: Cannot stop click event from bubbling

Actually in this case I think the problem is in Button.js. The original click event is getting canceled, but then Button.js calls _onClick which creates another event which bubbles:

	_onClick: function(/*Event*/ e){
		// summary:
		//		Internal function to handle click actions
		var ok = this.inherited(arguments);
		if(ok){
			if(this.valueNode){
				this.valueNode.click();
				e.preventDefault(); // cancel BUTTON click and continue with hidden INPUT click
                e.stopPropagation();    // avoid two events bubbling from Button widget
				// leave ok = true so that subclasses can do what they need to do
			}
		}
		return ok;
	},

If stopPropagation() was called on the original event, then it should be called on the new event. Likewise for preventDefault().

comment:5 Changed 7 years ago by Douglas Hays

Milestone: tbd1.9
Type: enhancementdefect

Changed 7 years ago by Douglas Hays

Attachment: 15097.patch added

possible fix

comment:6 Changed 7 years ago by Douglas Hays

Paul Christopher, can you please test with the attached patch to see if it behaves as you expect?

Changed 7 years ago by Douglas Hays

Attachment: 15097.2.patch added

refreshed fix

comment:7 Changed 7 years ago by Douglas Hays

I added a refreshed patch that uses bill's suggestion to stop the initial button click and then only deal with the synthetic event on the hidden valueNode. This needs review and testing. I also added automated tests that compare native button click event handling to both Button and Dialog/submit handling.

comment:8 Changed 7 years ago by Paul Christopher

Just tested the new, revised patch with the above test case, and - as far as I can judge - it works perfectly now!

comment:9 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [29962]:

Fixes #15097. Stop the initial click on the visible button-like elements and fire a synthetic click on the hidden typed INPUT so that native event behavior works as expected. Utilize the new event.defaultPrevented to stop calls to _onSubmit for non-form widgets. Added automated tests compare the behavior to native buttons.

comment:10 Changed 7 years ago by bill

Blocked By: 16344 added

Just noting the dependency for the record.

comment:11 Changed 7 years ago by bill

In [29964]:

add comment for 2.0, refs #15097 tangentially, !strict

comment:12 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

Hmm, I'm getting lots of failures after [29962], for example ButtonMixin.html. Failing on IE and FF.

A partial list of the IE8 failures is:

  • robot/Dialog_a11y
  • robot/Toolbar
  • Button (form/test_Button.html?mode=test)
  • Button_a11y
  • CheckBoxMixin (onclick w3)
  • ButtonMixin (events)
Last edited 7 years ago by bill (previous) (diff)

comment:13 Changed 7 years ago by Douglas Hays

In [29994]:

Refs #15097. Wrap valueNode.click() inside defer() so the current event that is being stopped doesn't get mixed up in IE. Fixed several tests to not use fake click events that don't understand defaultPrevented. Fixed several tests to not assume click() processing is synchronous. Added additional mobile tests to the mixin tests.

comment:14 Changed 7 years ago by bill

Thanks for working on that. I'm seeing failures now though in for example in Dialog_mouse.html, because the submit button doesn't close the dialog anymore.

comment:15 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:16 Changed 7 years ago by Douglas Hays

This seems to be a problem with IE specific code within on.js. A global lastEvent is being used and 2 different type==click events are arriving nearly simultaneously on 2 distinct nodes but the on._fixEvent processing is assuming they are the same event. Artificially separating the click events by 1 or more milliseconds seems to work around the issue but I'd rather not do that. The defer() in _ButtonMixin.js should be removed after this is fixed in on.js.

comment:17 Changed 7 years ago by bill

Resolution: fixed
Status: reopenedclosed

In [30051]:

When dojo/on compares the current event to a previously saved event, check if evt.target matches too. This avoids false matches in the case of dijit/form/Button, where a click event on a <span> programatically triggers a click event on another node (an <input type=button>). Fixes #15097 !strict.

Also rolls back [29994] which is no longer needed.

comment:18 Changed 7 years ago by bill

In [30052]:

Fix StackContainer test on IE, refs #15097

comment:19 Changed 7 years ago by Douglas Hays

In [30071]:

Refs #15097. IE < 9 was not correctly processing checkbox onclick event handlers when on() was used with the click() method.

comment:20 Changed 7 years ago by bill

There's still an issue with dojo/on. I split it off to #16543.

comment:21 Changed 7 years ago by bill

In [30281]:

Evt.target is not set for native events on IE6-8, so need to use evt.srcElement instead. Fixes regression in dojo/tests/on/on from [30051], refs #15097 and fixes #16543 !strict.

comment:22 Changed 6 years ago by Bill Keese <bill@…>

In 45637a1860c3cbc5e462511aa8fd94e58fc3ffbf/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:23 Changed 6 years ago by Bill Keese <bill@…>

In f6a11a5a3dd182eb5812fd036032c01eb38ce8d6/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:24 Changed 4 years ago by Bill Keese <bill@…>

In 8a7b57d877c1a5335359bcaacf1cc47bde79e76b/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:25 Changed 4 years ago by Bill Keese <bill@…>

In 4ef225abfcf5fad732501e5c154edd07bb6068a2/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:26 Changed 4 years ago by Bill Keese <bill@…>

In ab2d0ca45bfe35b2433b44ee3c914af141083bf1/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:27 Changed 4 years ago by Bill Keese <bill@…>

In e12545073e958fd81eae0199a10192f0bd8ca287/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:28 Changed 4 years ago by Bill Keese <bill@…>

In 531fe6db1799718ae4f79186795df9d4a6db10db/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:29 Changed 4 years ago by Bill Keese <bill@…>

In a4e7826a8561eed9f5084e1e17796b1f94ff0345/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.