Opened 7 years ago

Closed 7 years ago

#15859 closed defect (fixed)

ListItem doesn't highlight for "quick" touches

Reported by: Jeremiah Orr Owned by: Eric Durocher
Priority: high Milestone: 1.8.1
Component: DojoX Mobile Version: 1.8.0
Keywords: Cc: cjolif, Adrian Vasiliu
Blocked By: Blocking:

Description

As of 1.8.0, if you click/touch a dojox.mobile.ListItem? relatively quickly, it will perform the transition, but will not highlight the button. If you click/touch and hold a little longer, it will highlight correctly.

The attached file shows this behavior. If you switch to 1.7.3, it highlights even with quick clicks/touches.

I have observed this on iOS Safari 5.1.1 and Mac OS X Chrome 21.

Attachments (3)

touchTest.html (2.3 KB) - added by Jeremiah Orr 7 years ago.
patch15859.patch (729 bytes) - added by Adrian Vasiliu 7 years ago.
Fix for the "delayedSelection": now the item is selected before transitioning - Adrian Vasiliu, IBM, CCLA
patch15859-variant-handleSelection.patch (715 bytes) - added by Adrian Vasiliu 7 years ago.
Variant of the fix for the "delayedSelection": now the item is selected before transitioning - Adrian Vasiliu, IBM, CCLA

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by Jeremiah Orr

Attachment: touchTest.html added

Changed 7 years ago by Adrian Vasiliu

Attachment: patch15859.patch added

Fix for the "delayedSelection": now the item is selected before transitioning - Adrian Vasiliu, IBM, CCLA

comment:1 Changed 7 years ago by Adrian Vasiliu

Right. This is a side-effect of changeset 27598 for dojox/trunk/mobile/_ItemBase.js, which introduced the "delayed selection" mechanism, to avoid the unwanted selection of items during scrolling.

A workaround is to set item._delayedSelection at false (in markup, by adding "_delayedSelection:false" to the data-dojo-props of each item).

Attached a candidate patch, which aims to preserve the "delayed selection" mechanism while ensuring the item is selected before transitioning (it is deselected after transition).

Tested this patch with ListItem and IconMenu (another subclass of _ItemBase), on desktop (Chrome, Firefox) and mobile (iOS and Android).

comment:2 Changed 7 years ago by cjolif

Milestone: tbd1.8.1
Priority: undecidedhigh

comment:3 Changed 7 years ago by cjolif

Cc: cjolif added

comment:4 Changed 7 years ago by cjolif

The problem I see with the patch is that is there is a user click action it won't call the defaultClickAction and thus don't see the selected state on the button. I'm not sure this is expected? Why not doing the selected call in onTouchEnd instead?

comment:5 Changed 7 years ago by cjolif

Cc: Adrian Vasiliu added

Changed 7 years ago by Adrian Vasiliu

Variant of the fix for the "delayedSelection": now the item is selected before transitioning - Adrian Vasiliu, IBM, CCLA

comment:6 Changed 7 years ago by Adrian Vasiliu

I'm not sure calling it in _ItemBase._onTouchEnd would be better. In this method, we don't know yet what will happen next. This method delegates the handling of the event to _onClick, which is *not* defined on _ItemBase, instead it is defined and implemented in the subclasses (ListItem, IconMenu). These implementations of _onClick in the subclasses are the ones which call either the user-defined onClick, or the defaultClickAction. It may not be okay to assume that any user-defined onClick would always like this selection to happen. Also, a user-defined onClick, if it wants the selection to be handled similar to the default, has anyway to call by itself _ItemBase.handleSelection, since this is only called by defaultClickAction.

Given that defaultClickAction is already the only one to call handleSelection, it looked consistent to me it's also this method which takes care of this additional selection. But, all in one, I would rather propose to move the call from defaultClickAction to _ItemBase.handleSelection, as in the attached patch15859-variant-handleSelection.patch. This is technically equivalent to the previous variant, but probably semantically better, grouping together selection-related stuff. This way, for a user-defined click action, just calling handleSelection would restore the entire default behavior for the selection (if needed).

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:7 Changed 7 years ago by cjolif

Adrian, can you check something? There is some code in _onTouchMove that is here to "cancel" the selection if the user moves touch during the selection gesture. I fear that your patch might override that behavior. Can you investigate? Thanks.

Last edited 7 years ago by cjolif (previous) (diff)

comment:8 Changed 7 years ago by cjolif

ok found the answer myself, _onTouchMove calls cancel() which removes the touch end listener and so the added code is never called in that case and thus can't cause an issue.

That said even if my code is maybe not "better" what is sure is that it would better correspond to the behavior that existed before the delayed selection mechanism was introduced and would be more consistent between when this mode is one vs not. Actually if the delay is 0, you will see the selection happens whatever is the user-defined onClick method while with your solution if the user-defined onClick returns false then no selection will occur when there is a delayed selection vs one will occur when there is not. That said the right solution is probably wider than that because there are other side problems like the fact the selection is partly handled in always executed code, partly handled in code that is only executed if user-defined onClick returns true. So waiting for the "right" solution managing onClick with false let's do what you propose that is fixing the issue in most cases and where the only potential problem is with a case that anyway needs some rework.

comment:9 Changed 7 years ago by cjolif

In [29583]:

refs #15859. Quick fix for quick selection issue. The selection management with a user-defined onClick might still need work however.Thanks Adrian Vasiliu (IBM, CCLA). !strict.

comment:10 Changed 7 years ago by cjolif

In [29584]:

refs #15859. Quick fix for quick selection issue. The selection management with a user-defined onClick might still need work however.Thanks Adrian Vasiliu (IBM, CCLA). !strict.

comment:11 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.