Opened 20 months ago

Closed 13 months ago

Last modified 5 days ago

#15878 closed defect (fixed)

dojo/touch click event normalization and fixes

Reported by: dmandrioli Owned by: edurocher
Priority: undecided Milestone: 1.9
Component: General Version: 1.8.0
Keywords: Cc: cjolif, bill
Blocked by: Blocking:

Description (last modified by bill)

When you click on a ListItem contained in a ScrollableView, the click event is duplicated.

This affects only Android 4.1 and is related to http://code.google.com/p/android/issues/detail?id=36116

Workaround: listen for touchEnd instead of click event or use a View instead of a ScrollableView.

Attachments (4)

15878.patch (960 bytes) - added by edurocher 20 months ago.
Do not fire synthetic click events in ScrollableView for Android >= 4.1 - Eric Durocher (IBM, CCLA)
touch_click.patch (15.8 KB) - added by edurocher 14 months ago.
On touch-enabled devices, generate clicks in dojo/touch instead on relying on the browser, to avoid inconsistent behaviors - Eric Durocher (IBM, CCLA)
touch_click_2.patch (3.2 KB) - added by edurocher 14 months ago.
Fix some regressions on dijit widgets on mobile - Eric Durocher (IBM, CCLA)
click_backtrack.patch (2.8 KB) - added by edurocher 13 months ago.
Backtrack use of dojoClick to replace synthetic clicks in mobile - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (46)

Changed 20 months ago by edurocher

Do not fire synthetic click events in ScrollableView for Android >= 4.1 - Eric Durocher (IBM, CCLA)

comment:1 Changed 20 months ago by edurocher

  • Cc cjolif added

The ScrollableView fires synthetic click events because it prevents/stops touchstart/move events (to disable native browser scrolling), and in that case the OS normally does not fire click events on touchend. However, this is not true in Android 4.1: the click event is still fired even though touchstart/move events are stopped. This explains why we see duplicate click events. The solution is to not fire click events again on Android 4.1.

This will have to be tested again on future Android versions to see if the behavior changes again...

comment:2 Changed 20 months ago by cjolif

In [29617]:

refs #15878. Fixes double click events occuring on Android >= 4.1. Thanks Eric Durocher (IBM, CCLA). !strict.

comment:3 Changed 20 months ago by cjolif

  • Resolution set to fixed
  • Status changed from new to closed

In [29619]:

fixes #15922, #15878. Backport fixes from Eric Durocher (IBM, CCLA) into 1.8 branch. !strict.

comment:4 Changed 18 months ago by bill

  • Milestone changed from tbd to 1.8.1

comment:5 Changed 14 months ago by edurocher

  • Milestone 1.8.1 deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened

Testing the Android version (>= 4.1) does not seem reliable enough, as the click event is apparently not sent on Android 4.1 devices using the Chrome browser (Galaxy Note II). So, I am reopening this and looking for a more general solution not based on version sniffing.

Last edited 14 months ago by edurocher (previous) (diff)

comment:6 Changed 14 months ago by edurocher

This other problem seems similar: #16676. Not really a duplicate since it is not in the same module, but the resolution should be similar.

comment:7 Changed 14 months ago by edurocher

  • Cc bill added

OK, I'd like to propose this new patch as a browser independent solution.

The idea is the following: instead of trying to guess which browser will / will not send a click after a preventDefault() on touch events, we always generate click events ourselves in dojo/touch, and we always stop native click events sent by the browser.

This should solve several different problems:

  • this ticket about dojox/mobile/scrollable, and the similar ticket on mobile Switch: #15922 (with the patch, we can just remove the ugly code that generated the clicks in these widgets)
  • a similar ticket on dojox/gesture: #16676
  • and also the "click delay" problem: #16720

The patch can probably be enhanced, for example we could probably avoid adding new listeners on the document for touchmove and touchend. I added different listeners because we need to always be notified of touchmove/end events, even if the event was stopped in a listener, and the existing listeners in dojo/touch do not use addEventListener(..., true). The existing listeners could probably be changed to use addEventListener(..., true), thus removing the need for the new listeners. Bill?

Also, not sure about the move threshold (40 currently, this seems approximately what iOS does...)

comment:8 Changed 14 months ago by bill

I was also thinking about the click delay, which some people consider a problem. You listed it as #16720 but there's also a lot of prior discussion at http://thread.gmane.org/gmane.comp.web.dojo.devel/17903 and #15110. My key point from #15110 is that the delay is by design from Apple to enable page zoom via double click, so we want to be cautious if we are intentionally breaking that behavior.

Your proposed patch is similar to code that's already in dijit/a11yclick.js. The main difference of your code, compared to the code in a11yclick, is that your code is applying this behavior to every node on the page, not just nodes with a11yclick listeners on them.

Your code is better for performance since it just sets up a few listeners on the page rather than listeners on every node, and it possibly fixes problems trying to use event delegation with dijit/a11yclick (ex: putting a dijit/a11yclick handler on a Toolbar instead of the individual buttons). Your code is more dangerous though; developers might not appreciate how merely loading dojo/touch causes side effects on their whole page. A compromise would be to use your design, but only enable that behavior for nodes that have a property (ex: dojoClick) and/or attribute (ex: data-dojo-click-node) or class name set. Perhaps also check if it's set on an ancestor of the node. That's similar to _CssStateMixin's technique. Not sure what's best.

Like the a11yclick code, your code has a bug where zooming the page via doubleclick causes an unexpected click event. Except your bug is presumably worse because it affects doubleclicking anywhere on the page, not just doubleclicking nodes with dijit/a11yclick handlers. Not an issue for pages where zoom is disabled, but it could be an issue for other pages.

Also, you shouldn't be using dojo/_base/event, it's effectively deprecated. Pluswhich, dojo/_base/event::stop() calls evt.stopPropagation() but presumably you want to call evtlstopImmediatePropagation(), in case the clicked node itself has a "click" listener registered.

Last edited 14 months ago by bill (previous) (diff)

comment:9 Changed 14 months ago by bill

PS: The idea behind a synthetic event like dijit/a11yclick (which perhaps should just be called dijit/click) is that app code call on(node, a11yclick, ...) instead of on(node, "click", ...). If code follows that rule, the on.emit() call doesn't have to emit "click", it could emit "dojoclick" etc., similar to how the other code in dojo/touch works. That would be more straightforward, albeit users would need to be trained to do on(node, a11yclick, ...).

The reasons a11yclick is currently emitting a "click" event are:

  1. for native browser behavior. For example, dijit/Checkbox is an actual <input type=checkbox> so we depend on the browser rather than javascript to toggle it.
  2. because the app may be listening for "click" itself, rather than dijit/a11yclick
Last edited 14 months ago by bill (previous) (diff)

comment:10 Changed 14 months ago by edurocher

Yes I thought of a separate dojoclick event too (this could be exposed as touch.click), but did not do it for the same 2 reasons you mention...

About your previous comments, I agree this may be too intrusive and it would probably be safer to explicitly enable this on a specific node/hierarchy only.

comment:11 Changed 14 months ago by edurocher

New version of the patch which

  • also generates clicks on IE10 (where the delay is noticeable too)
  • generates clicks only for nodes that have the dojoClick property set to true (set by dojox/mobile to the whole document by default).
  • replaces similar code in dijit/a11yclick by a dojo/touch require

Note, the old dijit/a11yclick code was not strictly equivalent, since it used a timer to wait for a potential native click fired by the browser. To decide if we really want to do this.

comment:12 Changed 14 months ago by edurocher

New version of patch where dojoClick is set only on some dojox/mobile widgets where the native behavior is problematic (Buttons, scrollable, Switch). Also added a way to customize the move threshold per node.

Changed 14 months ago by edurocher

On touch-enabled devices, generate clicks in dojo/touch instead on relying on the browser, to avoid inconsistent behaviors - Eric Durocher (IBM, CCLA)

comment:13 Changed 14 months ago by edurocher

Yet another version: reverted to setting dojoClick on the whole body, to get consistent click behavior on all mobile widgets.

comment:14 Changed 14 months ago by bill

  • Resolution set to fixed
  • Status changed from reopened to closed

In [30784]:

Put touch click normalization in dojo/touch, rather than in dijit/a11yclick and various mobile modules. Unlike the old dijit/a11yclick code, the new code works by squelching the real click event, and instead always generating a synthetic click event. Limited to nodes that have a dojoClick property set, or an ancestor with the dojoClick property set. Patch from Eric Durocher (IBM, CCLA), fixes #15878 !strict.

Some dijit widgets like dijit/Tree and dijit/TitlePane are currently manually listening for click events and keyboard events; they need to be updated to set the dojoClick property on their nodes instead.

comment:15 Changed 14 months ago by bill

In [30795]:

remove unused dependencies and streamline a11yclick code, refs #15878 !strict

Changed 14 months ago by edurocher

Fix some regressions on dijit widgets on mobile - Eric Durocher (IBM, CCLA)

comment:16 Changed 14 months ago by bill

In [30808]:

Make Tree leverage dijit/a11yclick, which indirectly make openOnClick work for keyboard "click", fixes #16819 !strict. Also makes Tree more responsive on mobile, refs #15878.

Builds on work from [30806] and [30807].

comment:17 Changed 14 months ago by bill

In [30818]:

Leverage ondijitclick in Calendar, to get the normalized touch handling in dojo/touch plus the keyboard click handling in dijit/a11yclick. Refs #15878 !strict.

comment:18 Changed 14 months ago by bill

  • Component changed from DojoX Mobile to General
  • Description modified (diff)
  • Milestone set to 1.9
  • Summary changed from ScrollableView: ListItem click event is duplicated on Android 4.1 to dojo/touch click event normalization and fixes

Changing title since this is mainly a ticket about changes to dojo/touch now (although it does fix the ScrollableView problem).

comment:19 Changed 14 months ago by bill

In [30819]:

Leverage dojo/touch in CheckBox and MultiSelect, to get the normalized touch click handling. These two widgets are special because they are based on native form controls (<input type=checkbox> and <select multiple>), so they need to get a real click event rather than using a synthetic dojoclick type event. Luckily (for this case) this is how dojo/touch is currently working, by emitting a click event. Refs #15878 !strict.

comment:20 Changed 14 months ago by bill

In [30820]:

Use ondijitclick in AccordionContainer, to get the normalized touch click handling. The keyboard click support will never be used in this case, since you can't focus the title of a closed accordion pane, but using ondijitclick as a way to trigger the touch normalization in dojo/touch. Refs #15878 !strict.

comment:21 Changed 14 months ago by bill

In [30821]:

Use ondijitclick in ScrollingTabController, to get the normalized touch click handling on the left/right/menu buttons. The keyboard click support will never be used in this case, since you can't focus those buttons, but using ondijitclick as a way to trigger the touch normalization in dojo/touch. Refs #15878 !strict.

comment:22 Changed 14 months ago by bill

In [30822]:

Use ondijitclick in TitlePane, to get the normalized touch click handling and keyboard "click" handling. Refs #15878 !strict.

comment:23 Changed 14 months ago by bill

In [30827]:

Use touch events in _ListMouseMixin, for more responsive click handling. Refs #15878 !strict.

comment:24 Changed 14 months ago by bill

In [30828]:

Suppress mousedown and mouseup events that occur 300ms after the synthetic click event is fired, since they may confuse client code. Also, do a better job of suppressing the native click event that (may) occur 300ms after the synthetic one; it sometimes appears on the DOMNode behind a popup that was closed in the 300ms interval. Patch from Eric Durocher (IBM, CCLA).

Refs #15878 !strict.

comment:25 Changed 14 months ago by bill

In [30829]:

Use touch events in StackContainer, for more responsive click handling for TabContainer tabs. Refs #15878 !strict. Note that [30821] only fixed the left/right/menu buttons. This fixes the main tab buttons.

comment:26 Changed 13 months ago by bill

In [30858]:

Don't call preventDefault() on the native click event, otherwise on Android text fields cannot be edited anymore. Not sure why, since we are generating a synthetic click event. Similarly, not sure why this doesn't break (native) checkboxes. Seems like they would be getting two clicks now instead of one. But, everything seems to work with this change. Refs #15878 !strict.

comment:27 Changed 13 months ago by bill

In [30880]:

set -webkit-tap-highlight color to avoid late flash on iOS, fixes #16863, refs #15878

comment:28 Changed 13 months ago by edurocher

Synthetic clicks are sent even on disabled elements, patch attached to #16866 :
http://bugs.dojotoolkit.org/attachment/ticket/16866/touch_click_disabled.patch

comment:29 Changed 13 months ago by bill

In [30902]:

Don't generate click events due to touchstart/touchend on disabled <button> nodes. Fixes #16866, refs #15878 !strict. Also renamed "clicked" to "clickTracker".

Changed 13 months ago by edurocher

Backtrack use of dojoClick to replace synthetic clicks in mobile - Eric Durocher (IBM, CCLA)

comment:30 Changed 13 months ago by bill

  • Resolution fixed deleted
  • Status changed from closed to reopened

Having more problems with this approach of generating faux click events, so will try another approach of just generating synthetic events with a different name.

Eric reports that: In dojox/mobile/tests/test_FormControls.html : if I click *exactly* on a RadioButton's check box it works as expected, but if I click on the label, other radios are not deselected, apparently because the widget does not receive the click...

From me: On iPad in dojox/mobile/tests/test_FormControls.html I'm seeing the checkbox toggle instantly (as we want) but then toggling again 300ms later, back to the original value. By my debugging, apparently what's happening is:

  1. the user clicks <label for="cbox">Click me</label>
  2. we emit synth click on that <label>
  3. the click on <label> generates a click event on the associated checkbox <input type="checkbox" ...>. we sort-of block that native event, but not really, due to [30858], so the checkbox toggles.
  4. 300ms later, the real click event on the <label> comes. again we try to suppress it, but we don't, due to [30858], so the checkbox gets another click event, and toggles again.

If I revert [30858] then clicking the label just doesn't do anything.

It seems like we are between a rock and a hard place... [30858] was needed on android because the synthetic click event wouldn't perform the browser default action. But as I worried, it causes repercussions [on iOS]. The other complication is of course that it's hard to suppress the click event from the touchstart/touchend without accidentally suppressing other click events.

comment:31 Changed 13 months ago by bill

Also, the stopNativeEvents() code from [30828] stops phantom click events from appearing on items behind a Menu (that is closed by the click event). However, it's problematic for clicking small things. On dijit/tests/form/mobile.html (and other tests) clicking on the checkboxes or their labels often generates touchstart and touchend events on nearby nodes, such as the containing <fieldset> or it's <legend>. The checkbox or label still gets a click event though. Somehow the browser adjusts for it.

comment:32 Changed 13 months ago by bill

In [30932]:

Rollback change to MultiSelect in [30819]. MulitSelect is a native form control that opens a popup, and there's no delay on click, so using dojoClick doesn't have any effect. Refs #15878 !strict.

comment:33 Changed 13 months ago by bill

In [30936]:

Call evt.preventDefault() on native click event except for INPUT controls. This prevents the double click problem while still allowing keyboard to popup on android. Also, forward clicks on labels to be clicks on the associated nodes. Patch from Eric Durocher (IBM, CCLA). Refs #15878 !strict.

Also removing "target" property to on.emit() hash, since it's already specified as the first argument to on.emit().

Finally, add CSS to avoid flash when clicking Tree.

comment:34 Changed 13 months ago by bill

In [30949]:

Don't call evt.preventDefault() for <textarea>, same as <input type=text>, refs #15878 !strict.

comment:35 Changed 13 months ago by bill

  • Resolution set to fixed
  • Status changed from reopened to closed

In [30958]:

fix regression where mouse clicks not getting through (on devices that support both mouse and touch), fixes #15878 !strict

comment:36 Changed 13 months ago by cjolif

In [31156]:

refs #15878. Fix two problems with synthetic clicks: virtual keyboard not hidden when clicking background + stopImmediatePropagation does not imply stopPropagation on Android 2.x. Thanks Eric Durocher & Sebastien Brunot(IBM, CCLA) (with review from Bill). !strict.

comment:37 Changed 13 months ago by bill

In [31162]:

fix dijit/a11yclick paths to be relative, refs #15878

comment:38 Changed 12 months ago by edurocher

In [31244]:

refs #15878. Let applications set win.doc.dojoClick to false, and generate synthetic clicks like in 1.8 in that case. !strict

comment:39 Changed 12 months ago by edurocher

In [31323]:

refs #15878. Fix regression in mobile test case using native radio buttons.

comment:40 Changed 12 months ago by bill

In [31403]:

remove no-longer used dependency after [30808], refs #15878 !strict

comment:41 Changed 10 months ago by edurocher

Coming back to ticket:15878#comment:5 and the specific problem on Galaxy Note 2: just tested with 1.9, all works OK now on that device. It is true, though, that if we set document.dojoClick = false; to disable the new 1.9 click machinery, Switch widgets for example do not receive click events, so the legacy code that generates click conditionally on some devices seems to fail on Galaxy Note 2. I don't think it is worth fixing as long as the default (new) dojoClick code works fine.

comment:42 Changed 5 days ago by Bill Keese <bill@…>

In a776e1ec8117dccfce8c8ad88e571a50e89db82a/dojo:

Avoid double-click event when onclick handler shows confirm()/alert().
Fixes #17815, refs #15878.

Note: See TracTickets for help on using tickets.